Ai4rag ocr - #128
Open
Medokins wants to merge 8 commits into
Open
Conversation
Force consistency_level="Strong" on Milvus vector/hybrid search so a query immediately following an add_documents() upsert can't race Milvus's default Bounded-staleness read and return zero hits against a collection that does have matching data. The added latency is negligible for these small, short-lived, per-evaluation collections. - Simplify the Milvus and pgvector storage schemas: stop duplicating chunk text inside a JSON blob (chunk_content / document) alongside the dedicated content / content_text column; store only metadata as JSON and read text from its own column. - Reject BenchmarkData correct_answers records with zero correct answers, preventing a downstream unitxt TokenOverlap crash on max() of an empty iterable. - Narrow the default chroma search space to drop the "window" retrieval method. Assisted-by: Claude Code Signed-off-by: Jakub Walaszczyk <jwalaszc@redhat.com> Signed-off-by: Nikodem Szwast <nikodemszwast@gmail.com>
Add optional RapidOCR integration via Docling for processing image formats. Merge OCR feature branch with main's MaaS refactoring. ### Added - Optional RapidOCR via Docling (`do_ocr`, `ocr_lang`, custom ONNX model paths) - Image format support (.jpg, .jpeg, .png, .tif, .tiff) in SUPPORTED_EXTENSIONS - OCR model validation and fail-fast error handling ### Changed - Updated text extraction to support OCR-capable document formats - Integrated MaaS refactoring (OpenAI-compatible client replacing OGX) ### Fixed - Fail fast with instructions when RapidOCR ONNX models are missing - Ensure `ensure_ascii=False` in JSON output for document discovery Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Nikodem Szwast <nikodemszwast@gmail.com>
Signed-off-by: Nikodem Szwast <nikodemszwast@gmail.com>
Branch/tag protection rules restricting who can trigger workflows apply repo-wide and can't be scoped to this workflow alone. Add a check-permissions job that looks up the triggering actor's repo role via the GitHub API and fails the run early unless they hold admin/maintain permission, before any build or publish jobs run. Signed-off-by: Jakub Walaszczyk <jwalaszc@redhat.com> Assisted-by: Claude Code Signed-off-by: Nikodem Szwast <nikodemszwast@gmail.com>
Signed-off-by: Nikodem Szwast <nikodemszwast@gmail.com>
Restores the v0.11.1 and v0.11.0 changelog entries that were inadvertently removed. These tagged releases exist in the repository history and must be preserved in the changelog to maintain the historical record and follow semantic versioning principles. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Nikodem Szwast <nikodemszwast@gmail.com>
* feat(evaluator): add RAGAS evaluator with llm_judge_mode selector Adds RagasEvaluator (+ ragas adapters) as a regular dependency, an llm_judge_mode selector (base/ragas/all/none) on run_rag_optimization, a shared build_aggregate_metric helper, evaluator-aware metric resolution, and docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * test(evaluator): add functional tests for RAGAS evaluator Cover the two surfaces the RAGAS unit tests deliberately skip: - Drive the real ragas.evaluate machinery through the ai4rag adapters with local fake models (no network), asserting the pipeline runs end-to-end, delegates to both the LLM and embedding adapters, and returns well-formed results in the unit range. This catches RAGAS version drift in the dataset schema, evaluate() signature, result columns, and BaseRagasLLM/BaseRagasEmbeddings interfaces. - Wire a RagasEvaluator into a full AI4RAGExperiment run (real Chroma, mocked search-space models) with only the scoring step stubbed, verifying RAGAS metrics are routed by evaluator type into results. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * fix(unitxt): exclude records without references instead of crashing Reference-based unitxt metrics (faithfulness, answer_correctness) delegate to TokenOverlap, which crashes with "max() iterable argument is empty" when a record has no references (empty contexts/ground_truths). One such record aborted the whole evaluation via the broad except -> EvaluationError. Evaluate each reference-based metric only over the records that carry its references: unevaluable records contribute no per-question score and are left out of the mean/CI. Metrics that share an evaluable-row set still run in a single evaluate() call, so the healthy case is unchanged in behaviour and cost. Add real-unitxt regression tests reproducing the production crash. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * fix(experiment): treat unscored optimization metric as failed iteration When the optimization metric is produced but has a None mean (now reachable for reference-based unitxt metrics whose records all lack references), the score lookup used next((mean ...), None), which conflated "metric absent" with "metric present but unscored" and raised a misleading 'not found' error that aborted the whole optimization run. Split the two cases in a new _resolve_optimization_score helper: a genuinely absent metric (wrong name/evaluator) still raises RAGExperimentError with an evaluator-qualified message, while a produced-but-None metric returns None so the optimizer records a failed — not fatal — iteration (matching the float|None contract and gam_opt's None handling). Also disambiguate the optimization_metric flag in the streamed payload by name AND evaluator, so a colliding name (unitxt vs ragas 'faithfulness') only flags the actual target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * refactor(evaluator): drop ragas adapter factory functions Call the AI4RAGRagasLLM / AI4RAGRagasEmbeddings constructors directly instead of wrapping them in make_ragas_llm / make_ragas_embeddings factories that added nothing over the constructors themselves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * Revert "fix(unitxt): exclude records without references instead of crashing" This reverts commit b5f0369. The empty retrieved contexts were caused by a Milvus read-after-write consistency issue (documents fetched too soon after indexing), not by the metrics themselves, and that root cause is being fixed separately. Restore the simpler single evaluate() call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * refactor(experiment): require RAGMetric instances for metrics Both the metrics and optimization_metric setters now expect RAGMetric instances selected from Metrics and reject bare name strings, which are ambiguous because a name (e.g. "faithfulness") is shared across the unitxt and RAGAS evaluators. The optimization pipeline resolves its configured metric name to a concrete RAGMetric before constructing the experiment, binding ambiguous names to the unitxt variant under the assumption that only unitxt metrics (plus the custom overall_score) drive optimization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * fix(leaderboard): disambiguate colliding metric names by evaluator The leaderboard keyed aggregate scores by metric name only, so when both the unitxt and RAGAS evaluators emit "faithfulness" the RAGAS score silently overwrote the unitxt one (last-wins), conflating two distinct metrics into a single column that could show the wrong evaluator's value. Key aggregate scores by a collision-free key: unitxt/custom metrics keep their bare name, other evaluators are prefixed (e.g. "ragas_faithfulness") so each variant gets its own leaderboard column. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * docs(evaluator): reference ragas adapter classes not removed factories The mkdocstrings directives still pointed at make_ragas_llm / make_ragas_embeddings, which were removed; point them at the AI4RAGRagasLLM / AI4RAGRagasEmbeddings classes so the strict docs build passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> * docs: reflect RAGMetric-only metric API and evaluator-aware matching Update the evaluation guide and data-flow doc after tightening the metrics / optimization_metric API to reject bare name strings: drop the string-based examples and note that a RAGMetric instance from Metrics is required. Also fix the optimization-score pseudocode to match on both name and evaluator, as the code does for colliding names. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: “Piotr <phelm@redhat.com> --------- Signed-off-by: “Piotr <phelm@redhat.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Nikodem Szwast <nikodemszwast@gmail.com>
jakub-walaszczyk
requested changes
Aug 20, 2026
jakub-walaszczyk
left a comment
jakub-walaszczyk
left a comment
Collaborator
There was a problem hiding this comment.
Proper PR name is missing.
There is no description in the PR, even though the template is prepared.
Some things are not described on whjy we make things the way we make. Maybe we should consider some quick demo before processing further to introduce some more context for the review.
Additionally, please provide some more information on what happens with the language handling. It is kind of limited here, hence a little bit error prone and misaligned with what we actually try to support?
| ".qmd", | ||
| ".rmd", | ||
| ".xhtml", | ||
| # Image formats for OCR-capable ingestion (RapidOCR via Docling). |
Collaborator
There was a problem hiding this comment.
Please remove this unnecessary comment
Comment on lines
+97
to
+102
| do_ocr: bool = False, | ||
| ocr_lang: Sequence[str] | str | None = None, | ||
| ocr_det_model_path: str | None = None, | ||
| ocr_cls_model_path: str | None = None, | ||
| ocr_rec_model_path: str | None = None, | ||
| ocr_rec_keys_path: str | None = None, |
Collaborator
There was a problem hiding this comment.
Don't we want to expect instance of DoclingExtractionConfig as a parameter here? pipelines-components might construct this and pass prepared instance
|
|
||
| def extract_text( # pylint: disable=too-many-locals,too-many-arguments,too-many-positional-arguments | ||
| @dataclass(frozen=True) | ||
| class DoclingExtractionConfig: |
Collaborator
There was a problem hiding this comment.
This would be nice to have docstring here describing each parameter "what" and "why".
|
|
||
| s3_creds = _resolve_s3_credentials(s3_endpoint, s3_access_key, s3_secret_key, s3_region) | ||
| artifacts_path = _resolve_artifacts_path(docling_artifacts_path) | ||
| pipeline_config = DoclingExtractionConfig( |
Collaborator
There was a problem hiding this comment.
as suggested in the class signature comment, double-handling these parameters might not have much rationale. We can pass ready instance of this dataclass to the function while calling it.
Comment on lines
+422
to
+435
| _ARTIFACTS_RAPIDOCR_ENGLISH = ( | ||
| "onnx/PP-OCRv4/det/en_PP-OCRv3_det_mobile.onnx", | ||
| "onnx/PP-OCRv4/cls/ch_ppocr_mobile_v2.0_cls_mobile.onnx", | ||
| "onnx/PP-OCRv4/rec/en_PP-OCRv4_rec_mobile.onnx", | ||
| ) | ||
| _ARTIFACTS_RAPIDOCR_CHINESE = ( | ||
| "onnx/PP-OCRv4/det/ch_PP-OCRv4_det_mobile.onnx", | ||
| "onnx/PP-OCRv4/cls/ch_ppocr_mobile_v2.0_cls_mobile.onnx", | ||
| "onnx/PP-OCRv4/rec/ch_PP-OCRv4_rec_mobile.onnx", | ||
| ) | ||
| # Optional small models that some rapidocr wheels still ship under ``rapidocr/models/``. | ||
| _BUNDLED_RAPIDOCR_DET = "PP-OCRv6_det_small.onnx" | ||
| _BUNDLED_RAPIDOCR_CLS = "ch_ppocr_mobile_v2.0_cls_mobile.onnx" | ||
| _BUNDLED_RAPIDOCR_REC = "PP-OCRv6_rec_small.onnx" |
Collaborator
There was a problem hiding this comment.
Any constants should be elevated to the top of the file. The question is, why do we have such relative paths here? Shouldn't we store such paths in the pipelines-components after all? Question, not assumption. Let's discuss on why
Comment on lines
+448
to
+451
| try: | ||
| import rapidocr | ||
| except ImportError: | ||
| return None |
Collaborator
There was a problem hiding this comment.
is there a case where we do not install rapidocr? I do not think so. It is most likely installed with docling's deps (btw I do not see the added dependencies, so where does this come from?)
Additionally, when we catch error on import and we silently return None this is completely not informational. How are we supposed to debug it later in the prod when all we have are some logs.
| _logger.addHandler(handler) | ||
|
|
||
| DOWNLOAD_MAX_THREADS = 8 | ||
| DEFAULT_OCR_LANG: tuple[str, ...] = ("english",) |
Collaborator
There was a problem hiding this comment.
What are the languages and how is the language detection handled? What is the process?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Added support for OCR