feat: implement OCR handling for embedded images and add warnings#2059
Open
ricofurtado wants to merge 5 commits into
Open
feat: implement OCR handling for embedded images and add warnings#2059ricofurtado wants to merge 5 commits into
ricofurtado wants to merge 5 commits into
Conversation
github-actions
Bot
added
backend
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds OCR-disabled image ingestion blocking across document processors, detects embedded images in Docling output, propagates warnings through standard and Langflow ingestion paths, and includes warning-bearing completed files in task listings. ChangesOCR-disabled image handling and embedded-image warnings
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TaskProcessor
participant DoclingService
participant DocumentProcessing
TaskProcessor->>DoclingService: convert_file(ocr=effective_ocr)
DoclingService-->>TaskProcessor: document data
TaskProcessor->>DocumentProcessing: has_embedded_images(doc_dict)
DocumentProcessing-->>TaskProcessor: true/false
TaskProcessor-->>TaskProcessor: attach warning when OCR is disabled
sequenceDiagram
participant Client
participant LangflowFileService
participant DoclingPollingService
participant TaskService
Client->>LangflowFileService: upload_and_ingest_file(ocr_override=false)
LangflowFileService->>DoclingPollingService: poll_until_ready()
DoclingPollingService-->>LangflowFileService: document_json_content
LangflowFileService-->>Client: success result with warning
TaskService-->>Client: tasks/files include warning-bearing completed files
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/unit/test_document_processing_resplit.py (1)
122-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for the reference-based detection path.
The two new tests only exercise the top-level
picturescollection key. The_references_image_collectionhelper has non-trivial recursive logic (string marker matching, dictlabel/typeinspection, list traversal) that is currently untested. Adding a test that provides abodywith#/pictures/references or a dict item withlabel: "picture"would increase confidence in that code path.🤖 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/unit/test_document_processing_resplit.py` around lines 122 - 158, Add coverage for the reference-based image detection path in extract_relevant and its _references_image_collection helper. The current tests only verify the top-level pictures key, so add a test that drives the recursive logic through a body containing `#/pictures/` references and another case with a dict item using label or type set to picture. Keep the assertions focused on has_embedded_images so the helper’s string, dict, and list traversal branches are exercised.tests/unit/test_processors_embedded_image_warning.py (1)
11-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared test setup to reduce duplication.
The two tests share nearly identical mock setup (~70 lines each) differing only in the
ocrvalue and final assertions. A shared fixture or helper function for the common mocks (docling_service, config, embedding client, session_manager, etc.) parameterized byocrwould reduce duplication and prevent the two tests from drifting apart over time.🤖 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/unit/test_processors_embedded_image_warning.py` around lines 11 - 176, The two tests for TaskProcessor.process_document_standard duplicate the same mock and fixture setup, so extract the shared preparation into a reusable helper or fixture and parameterize it by the OCR setting. Keep the common setup for document_service, session_manager, docling_service, models_service, get_openrag_config, get_index_name, chunk_texts_for_embeddings, and the fake embedding client in one place, then have both tests only pass the differing ocr value and assert the expected warning behavior. This will reduce repetition and keep the embedded-image warning coverage in sync.src/models/processors.py (1)
719-724: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the repeated OCR-disabled early-fail block.
This exact 5-line fail-and-return block is duplicated across
DocumentFileProcessor(Line 719),ConnectorFileProcessor(Lines 937 and 999), andLangflowFileProcessor(Line 1410). A small helper would keep the FAILED/error/updated_at/failed_filesside effects consistent if this shape ever changes.♻️ Example helper
def _fail_task_with_ocr_image_error( upload_task: "UploadTask", file_task: "FileTask", error: str ) -> None: file_task.status = TaskStatus.FAILED file_task.error = error file_task.updated_at = time.time() upload_task.failed_files += 1- if image_error := get_ocr_disabled_image_error(original_filename): - file_task.status = TaskStatus.FAILED - file_task.error = image_error - file_task.updated_at = time.time() - upload_task.failed_files += 1 - return + if image_error := get_ocr_disabled_image_error(original_filename): + _fail_task_with_ocr_image_error(upload_task, file_task, image_error) + return🤖 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/models/processors.py` around lines 719 - 724, The OCR-disabled early-fail block is duplicated in multiple processors, so extract the shared FAILED/error/updated_at/failed_files logic into a small helper and reuse it from DocumentFileProcessor, ConnectorFileProcessor, and LangflowFileProcessor. Add a helper such as _fail_task_with_ocr_image_error and call it wherever get_ocr_disabled_image_error() returns a value, keeping the status update, error assignment, timestamp refresh, and failed_files increment in one place.
🤖 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 `@src/services/langflow_file_service.py`:
- Around line 1068-1073: Use the effective OCR state when deciding whether to
attach EMBEDDED_IMAGES_OCR_DISABLED_WARNING, because ocr_override only reflects
an explicit false override and misses config/default-disabled OCR. Update the
warning guard in the relevant block in langflow_file_service to mirror the
effective_ocr logic used in processors.py, and base the check on that computed
state before setting result["warning"].
---
Nitpick comments:
In `@src/models/processors.py`:
- Around line 719-724: The OCR-disabled early-fail block is duplicated in
multiple processors, so extract the shared FAILED/error/updated_at/failed_files
logic into a small helper and reuse it from DocumentFileProcessor,
ConnectorFileProcessor, and LangflowFileProcessor. Add a helper such as
_fail_task_with_ocr_image_error and call it wherever
get_ocr_disabled_image_error() returns a value, keeping the status update, error
assignment, timestamp refresh, and failed_files increment in one place.
In `@tests/unit/test_document_processing_resplit.py`:
- Around line 122-158: Add coverage for the reference-based image detection path
in extract_relevant and its _references_image_collection helper. The current
tests only verify the top-level pictures key, so add a test that drives the
recursive logic through a body containing `#/pictures/` references and another
case with a dict item using label or type set to picture. Keep the assertions
focused on has_embedded_images so the helper’s string, dict, and list traversal
branches are exercised.
In `@tests/unit/test_processors_embedded_image_warning.py`:
- Around line 11-176: The two tests for TaskProcessor.process_document_standard
duplicate the same mock and fixture setup, so extract the shared preparation
into a reusable helper or fixture and parameterize it by the OCR setting. Keep
the common setup for document_service, session_manager, docling_service,
models_service, get_openrag_config, get_index_name, chunk_texts_for_embeddings,
and the fake embedding client in one place, then have both tests only pass the
differing ocr value and assert the expected warning behavior. This will reduce
repetition and keep the embedded-image warning coverage in sync.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dce63426-c54a-4188-9209-fee25ed6c7f6
📒 Files selected for processing (9)
src/models/processors.pysrc/services/docling_polling_service.pysrc/services/langflow_file_service.pysrc/utils/document_processing.pytests/unit/test_document_processing_resplit.pytests/unit/test_langflow_file_service_two_phase.pytests/unit/test_processors_clear_stale_chunks.pytests/unit/test_processors_embedded_image_warning.pytests/unit/test_processors_ocr_image_block.py
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
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
1 participant
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.
This pull request introduces comprehensive support for handling documents with embedded images when OCR is disabled. It ensures that users are notified with clear warnings if embedded images are present but OCR is turned off, preventing silent data loss. The changes add detection for embedded images, propagate warnings through the document processing pipeline, and include tests to verify this behavior.
Embedded image detection and warning propagation:
has_embedded_imagesfield to the output ofextract_relevantindocument_processing.py, using a newhas_embedded_imagesfunction to detect embedded images in Docling JSON exports. [1] [2]EMBEDDED_IMAGES_OCR_DISABLED_WARNINGand logic to attach a warning to processing results if embedded images are found and OCR is disabled, in bothprocess_document_standardandupload_and_ingest_file. [1] [2]Image file handling and error reporting:
is_image_fileandget_ocr_disabled_image_errorhelpers to identify image files and return user-friendly errors if OCR is disabled, preventing ingestion of image files without OCR. [1] [2] [3] [4] [5]Docling polling and result propagation:
DoclingPollResultandDoclingPollingServiceto carry the full document JSON content, enabling downstream checks for embedded images. [1] [2] [3] [4]Testing and validation:
has_embedded_imagesfield is set correctly and that warnings are issued when appropriate, including a new test for the two-phase Langflow ingestion flow. [1] [2]Minor improvements:
Summary by CodeRabbit
New Features
Bug Fixes
Tests