feat: allow caller-controlled ORT loading - #1
Conversation
📝 WalkthroughWalkthroughThe OCR feature graph now separates native ORT support from dynamic loading. Rust, FFI, Python, examples, documentation, and OCR tests use ChangesOCR feature split
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/python.rs (1)
2351-2364: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign disabled-path guidance with the
ocr/ocr-ortfeature split.
src/python.rs#L2351-L2364: explain that theocrpackage extra enablesocr-ort, while direct source builds can selectocr-ort.examples/ocr_scanned_pdf.rs#L44-L48: document bothocrdynamic loading andocr-ortcaller-managed linking, with matching commands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/python.rs` around lines 2351 - 2364, The disabled OCR path guidance in src/python.rs lines 2351-2364 must explain that the ocr package extra enables ocr-ort, while direct source builds may select ocr-ort; update the documentation without changing the extraction logic. Also update examples/ocr_scanned_pdf.rs lines 44-48 to document both ocr dynamic loading and ocr-ort caller-managed linking, including matching commands.src/extractors/auto.rs (1)
1027-1051: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale feature documentation.
These implementations now use
ocr-ort, but nearby docs still say prefetching and OCR are controlled byocr. Standaloneocr-ortbuilds therefore receive incorrect setup guidance. Update those references, mentioningocronly as the compatibility alias if appropriate.Suggested wording update
-/// ... no-op without the `ocr` feature ... +/// ... no-op without the `ocr-ort` feature ... -/// With `ocr` the ONNX ... +/// With `ocr-ort` the ONNX ... -/// was compiled with the `ocr` feature ... +/// was compiled with the `ocr-ort` feature ... -/// attempted only with the `ocr` feature ... +/// attempted only with the `ocr-ort` feature ...Also applies to: 1069-1069, 1212-1213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/extractors/auto.rs` around lines 1027 - 1051, Update the nearby feature documentation associated with the OCR model prefetch/setup code and the additional references around the later affected sections to identify `ocr-ort` as the controlling feature. Mention `ocr` only as a compatibility alias where needed, and ensure standalone `ocr-ort` builds receive correct setup guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_ccitt_decoder_diagnosis.rs`:
- Line 2: Isolate the failing test_ccitt_decoder_diagnosis module instead of
enabling it through the broad ocr-ort feature gate. Either correct its
argument-type mismatch or add a separate exclusion/ignore so the native OCR test
lane remains unaffected when ocr-ort is enabled.
---
Outside diff comments:
In `@src/extractors/auto.rs`:
- Around line 1027-1051: Update the nearby feature documentation associated with
the OCR model prefetch/setup code and the additional references around the later
affected sections to identify `ocr-ort` as the controlling feature. Mention
`ocr` only as a compatibility alias where needed, and ensure standalone
`ocr-ort` builds receive correct setup guidance.
In `@src/python.rs`:
- Around line 2351-2364: The disabled OCR path guidance in src/python.rs lines
2351-2364 must explain that the ocr package extra enables ocr-ort, while direct
source builds may select ocr-ort; update the documentation without changing the
extraction logic. Also update examples/ocr_scanned_pdf.rs lines 44-48 to
document both ocr dynamic loading and ocr-ort caller-managed linking, including
matching commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f68977b7-0f78-437a-8031-32863593bc20
📒 Files selected for processing (32)
Cargo.tomldocs/OCR_GUIDE.mddocs/getting-started-rust.mdexamples/ocr_scanned_pdf.rssrc/document.rssrc/error.rssrc/extractors/auto.rssrc/ffi.rssrc/lib.rssrc/ocr/backend.rssrc/ocr/error.rssrc/python.rstests/test_alice_wonderland.rstests/test_auto_hybrid_text_image_520.rstests/test_auto_ocr_image_519.rstests/test_auto_ocr_multilang_519.rstests/test_ccitt_decoder_diagnosis.rstests/test_ccitt_edge_cases.rstests/test_ccitt_params_extraction.rstests/test_full_extraction.rstests/test_image_filters.rstests/test_ocr.rstests/test_ocr_inference.rstests/test_ocr_integration.rstests/test_ocr_module.rstests/test_ocr_page_detection.rstests/test_ocr_scanned_document.rstests/test_ocr_with_models.rstests/test_pdf_ccitt_params.rstests/test_prefetch_models_519.rstests/test_pride_prejudice.rstests/test_stream_filters.rs
| @@ -1,5 +1,5 @@ | |||
| #![allow(warnings)] | |||
| #[cfg(feature = "ocr")] | |||
| #[cfg(feature = "ocr-ort")] | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Isolate the known failing CCITT test before enabling it here.
The PR notes that test_ccitt_decoder_diagnosis still has an argument-type failure. This gate now includes that failing module whenever ocr-ort is enabled, preventing the native OCR test lane from passing. Fix the mismatch or exclude/ignore this diagnostic test separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_ccitt_decoder_diagnosis.rs` at line 2, Isolate the failing
test_ccitt_decoder_diagnosis module instead of enabling it through the broad
ocr-ort feature gate. Either correct its argument-type mismatch or add a
separate exclusion/ignore so the native OCR test lane remains unaffected when
ocr-ort is enabled.
|
Superseded by #2. This first branch was based on upstream main and would have imported 43 unpublished commits beyond v0.3.74. The replacement maintenance PR is based directly on the peeled v0.3.74 release commit and contains only the loader-policy change. |
1 participant
Summary
Validation
A broad upstream target still encounters the existing fax argument-type failure in test_ccitt_decoder_diagnosis under both the new and legacy configurations; this PR intentionally leaves that unrelated test untouched.
Summary by CodeRabbit
New Features
ocr-ortfeature for native ONNX Runtime OCR with caller-controlled linking and loader settings.Documentation