feat(ocr): pluggable engine architecture + LLM vision OCR + configurable threshold#548
Conversation
c6efc36 to
82f223d
Compare
e9d1ee0 to
c8b3bd3
Compare
Replace hard-coded QwenVisionOCR with a plugin-based engine that discovers OCR backends via Python entry_points. Ships with: - LLMDocumentOCR: multi-modal LLM vision (Qwen/OpenAI/Gemini) - RapidDocumentOCR: offline RapidOCR adapter - Extensible base class for custom engines Includes comprehensive test coverage, env-schema config for engine selection, and updated doc-reader skill documentation. Signed-off-by: shadowinlife <shadowinlife@gmail.com>
c8b3bd3 to
28b0575
Compare
Implement the backward compatibility layer mandated by Issue HKUDS#547 (design-decision table): - engine.py: add _CHOICE_ALIASES table ("qwen-vl" -> "llm-vision") applied in get_ocr_engine() before engine lookup, emitting a deprecation warning. - env_schema.py: add Pydantic model_validator(mode="before") on OcrConfig that reads legacy VIBE_TRADING_OCR_QWEN_MODEL, forwards to the new VIBE_TRADING_OCR_LLM_MODEL field, and emits a deprecation warning. New env var takes precedence when both are set. - llm_vision_ocr.py: restrict the "ollama" placeholder api_key to the ollama provider only. Unknown providers now get empty string so they no longer silently misconfigure (previously any unknown provider without OPENAI_API_KEY was given "ollama" as api_key, corrupting the OpenAI client for non-ollama providers). - test_ocr_engine.py: TestBackwardCompatAliases (6 new tests): alias mapping, deprecation warning emission, env-var precedence, new-takes-precedence over legacy, ollama-only fallback, and unknown-provider empty-key behavior. Tests: 30 passed, 1 skipped (rapidocr_onnxruntime not installed). Full suite: 5412 passed, 4 failed (pre-existing tushare E2E auth, unrelated). Part of HKUDS#547. Signed-off-by: shadowinlife <shadowinlife@gmail.com>
shadowinlife
left a comment
There was a problem hiding this comment.
Code Review — pluggable OCR engine architecture
Reviewed the full diff against the linked Issue #547 and the current main integration points (doc_reader_tool.py, env_schema.py, provider_env_names). No clone was needed — everything was resolvable from the diff plus surrounding source. Skipping pure formatting/lint/import-ordering nits (left to tooling).
What's done well
🎉 The privacy red line is modeled cleanly as data (is_cloud) rather than scattered if choice == "qwen-vl" checks. _select_first_local() enforces it uniformly, and even third-party plugins inherit the guarantee. Well covered by test_auto_never_selects_cloud_engine and friends.
🎉 The backward-compat layer is solid: both the engine-name alias (_CHOICE_ALIASES) and the env-var alias (Pydantic model_validator) emit deprecation warnings and are backed by regression tests (TestBackwardCompatAliases), including new-takes-precedence semantics.
🎉 Keeping RapidOcrEngine.__init__ cheap with an explicit comment about _select_first_local() probing every engine shows good awareness of the lazy-init contract.
Decision: 💬 Comment (recommend changes before merge)
Architecture direction is right, the privacy design is excellent, and tests/backward-compat are thorough. No blocking crash/data-loss issues. But three [important] items are worth resolving before merge — the quality_flag misreport is highest priority because it directly undermines this PR's M2 goal (agent quality awareness). Inline comments below.
…stants, nit - Track ocr_attempted_pages to distinguish "OCR attempted but empty" from "no OCR needed"; map former to quality_flag="degraded" - Promote inline magic numbers to named module constants with rationale comments (_OCR_MAX_TOKENS, _OCR_TEMPERATURE, _OCR_JPEG_QUALITY) - Replace no-op round(len(text) / 1, 1) with float(len(text)) Signed-off-by: shadowinlife <shadowinlife@gmail.com> AI-Contributed/Feature: 0/40 AI-Contributed/UT: 0/0
…nfidence(), deduplicate import - Only retry transient errors (timeout/connection/5xx) in LLM vision OCR; deterministic 4xx failures now raise immediately without delay - Remove unused confidence() method that hid a redundant OCR call - Remove duplicate model_validator import in env_schema.py Signed-off-by: shadowinlife <shadowinlife@gmail.com> AI-Contributed/Feature: 0/46 AI-Contributed/UT: 0/0
|
Thanks @shadowinlife — merged. The provider-agnostic |
Summary
Part of Issue #547 — Milestone M0 + M1 + M2
Delivers OCR engine plugin infrastructure (M0), threshold parameterization (M1), and quality metrics with agent awareness (M2) in a single focused change set.
entry_pointsplugin discovery (replaces hardcoded_VALID_ENGINESfactory)LlmVisionOcrEnginefor any OpenAI-compatible provider (replaces DashScope-onlyQwenVisionOcrEngine)min_text_per_page) + quality metrics (ocr_qualityfield) + agent-aware SKILL.md updatesVIBE_TRADING_OCR_ENGINE=qwen-vlandVIBE_TRADING_OCR_QWEN_MODELalias with deprecation warningsNot in this PR (subsequent PRs in the #547 series):
output_formatparam) — separate feature issuePart of #547
Why
The
strategy-dev-managerskill (#455, merged via #457) introduced a pluggable OCR engine interface with two built-in engines. Five problems remained unresolved:rapidocr_onnxruntimenot declared as a dependency — fresh installs silently fail on scanned PDFsQwenVisionOcrEnginehardcoded to DashScope — users with OpenAI/Anthropic/Google cannot use their provider_VALID_ENGINEStuple — third parties cannot publish OCR pluginsThis PR resolves #1–#5. The plugin scaling concern (Issue #547 main motivation) is addressed by the
entry_pointsmechanism, which allows any third-party package to publish OCR engines without modifying core.Changes
Core architecture (Issue #547 M0)
agent/src/tools/ocr/engine.py— Rewritten:OcrEngineProtocol now requiresis_cloudandinstall_hintfields. Replaced_VALID_ENGINEStuple +_try_rapid()/_try_qwen_vl()factory functions with_BUILTIN_ENGINESregistry +_discover_plugins()viaimportlib.metadata.entry_points(group="vibe_trading.ocr_engines"). Added_select_first_local()that filtersis_cloud=Falseengines forautomode. Added_CHOICE_ALIASESfor backward-compat engine name mapping with deprecation warnings (Issue [Feature] OCR Enhancement: Pluggable Engine Architecture with entry_points Discovery #547). Added optionalconfidence()method to Protocol. Plugin discovery cached via@lru_cache(maxsize=1).agent/src/tools/ocr/llm_vision_ocr.py— New:LlmVisionOcrEnginewithis_cloud=True. Uses_resolve_provider_config()to reuse existingprovider_env_names()— no separate provider mapping table. Converts images to base64 JPEG (quality=85) for API upload. 1 retry for transient network errors. Detailed_OCR_SYSTEM_PROMPTspecifying output format (Markdown/HTML tables/LaTeX formulas).agent/src/tools/ocr/qwen_vision_ocr.py— Deleted. Users withVIBE_TRADING_OCR_ENGINE=qwen-vlare transparently aliased tollm-visionwith a deprecation warning (see Backward Compatibility section).agent/src/tools/ocr/rapid_ocr.py— Addedis_cloud=False,install_hint, optionalconfidence()method (mean RapidOCR confidence), and self-registration viaregister_builtin().Threshold parameterization (Issue #547 M1)
agent/src/tools/doc_reader_tool.py— Addedmin_text_per_pageparameter toread_document()(default 50, previously hardcoded_MIN_TEXT_PER_PAGE). Parameterized through_read_pdf(). Addedocr_engineandocr_qualityfields to PDF and image responses.agent/src/config/env_schema.py— RenamedVIBE_TRADING_OCR_QWEN_MODEL->VIBE_TRADING_OCR_LLM_MODELinOcrConfig. Added Pydanticmodel_validatorthat reads the legacy env var, forwards it to the new field, and emits a deprecation warning (new env var takes precedence when both are set).agent/.env.example— AddedVIBE_TRADING_OCR_ENGINEandVIBE_TRADING_OCR_LLM_MODELdocumentation with engine descriptions.Quality metrics & agent awareness (Issue #547 M2)
agent/src/tools/doc_reader_tool.py—ocr_qualityis a dict withquality_flag(good/degraded/no_ocr_engine/no_ocr_needed),ocr_pages, andtext_density(chars/page). Returned for both PDF and image paths.agent/src/skills/doc-reader/SKILL.md— Added OCR Configuration section: threshold, engine selection, response fields documentation.agent/src/skills/strategy-dev-manager/SKILL.md— Added OCR Quality Check step afterread_documentcalls (flag interpretation +text_density < 100warning).Tests
agent/tests/test_ocr_engine.py— Rewritten:TestEngineSelection(7 tests: privacy guarantee, fallback behavior, explicit cloud choice),TestRegistry(5 tests: builtin registration, plugin override, cache reset),TestInstallHints(3 tests),TestProviderConfigResolution(5 tests: provider env vars, fallback chains, model override),TestRecognizeNoAdvisory(1 test: no advisory warnings on explicit choice),TestBackwardCompatAliases(6 tests: engine-name alias, deprecation warning emission, env-var alias, precedence, ollama-only fallback, unknown-provider empty-key).agent/tests/test_ocr_integration.py— New: synthetic scanned PDF ->_read_pdf()-> OCR -> text verification. Requiresrapidocr_onnxruntime(skipped if not installed).Affected Areas
agent/src/tools/ocr/— complete rewrite of OCR engine factoryagent/src/tools/doc_reader_tool.py— new parameters and response fields (PDF + image paths only)agent/src/config/env_schema.py— one field rename + backward-compat validator inOcrConfigagent/src/skills/doc-reader/SKILL.md+strategy-dev-manager/SKILL.md— documentationagent/tests/test_ocr_engine.py+agent/tests/test_ocr_integration.py— test rewriteBackward Compatibility
All backward compatibility requirements from Issue #547's design-decision table are implemented:
VIBE_TRADING_OCR_ENGINE=autoVIBE_TRADING_OCR_ENGINE=rapidVIBE_TRADING_OCR_ENGINE=qwen-vlllm-vision+ deprecation warning_CHOICE_ALIASESinengine.py, applied inget_ocr_engine()before engine lookupVIBE_TRADING_OCR_ENGINE=noneVIBE_TRADING_OCR_QWEN_MODELVIBE_TRADING_OCR_LLM_MODEL+ deprecation warning_alias_legacy_env_varsPydanticmodel_validator(mode="before")inOcrConfig; new env var takes precedenceDesign divergence from Issue #547
_VISION_MODEL_KEYWORDSheuristic (40+ models)is_available()checks API key onlyVIBE_TRADING_OCR_ENGINE=llm-vision) is the strongest signal. A failed API call is clearer feedback than a heuristic refusal that second-guesses the user. Documented inllm_vision_ocr.pymodule docstring.Out of Scope
output_formatparam. Separate feature issue.rapidocr_onnxruntimeremains optional; engine fails gracefully with a clear install hint.Test Plan
pytest --ignore=agent/tests/e2e_backtest --ignore=agent/tests/test_e2e_harness_v2.py --tb=short -q— 5412 passed, 4 failed (all 4 pre-existing tushare E2E auth failures, unrelated to OCR)rapidocr_onnxruntime)rapidengine path preserved,automode privacy guarantee strengthenedos.getenvcalls use# noqa: env-gate)Risk Assessment
os.getenv()with# noqa: env-gate, matching codebase convention. No new credential paths.is_clouddata field ensuresautomode never sends pages to cloud. ExplicitVIBE_TRADING_OCR_ENGINE=llm-visionrequired for cloud OCR.doc_reader_tool.entry_pointsdiscovery loads installed packages. Same trust model aspip install.Rollback
git revert <merge-commit-sha>. Additive plugin system — no DB migrations or persistent state.VIBE_TRADING_OCR_ENGINE=rapidto use only RapidOCR (local). Engine name is"rapid", not"rapid-ocr".VIBE_TRADING_OCR_ENGINE=qwen-vlcontinues to work (aliased tollm-visionwith log warning).VIBE_TRADING_OCR_QWEN_MODELcontinues to work (forwarded to new field with log warning). Migration optional but recommended.Milestone Roadmap (Issue #547 series)
Checklist
src/agent/,src/session/,src/providers/) without prior discussionSigned-off-by:DCO trailer