refactor(parsers): remove legacy loaders and doc-serializer shims - #513
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves the Ray actor-based document serialization stack ( ChangesIn-process Parser Stack Replacing Ray-backed Legacy Loaders
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over ConversionService,ParserFileSerializer: New in-process path (replaces Ray DocSerializer)
end
participant ConversionService
participant ParserFileSerializer
participant ParserDispatcher
participant ConcreteParser
ConversionService->>ParserFileSerializer: serialize(file_path, metadata)
ParserFileSerializer->>ParserFileSerializer: asyncio.to_thread(read file bytes)
ParserFileSerializer->>ParserFileSerializer: Document.detect_content_type(filename)
ParserFileSerializer->>ParserDispatcher: parse(document)
ParserDispatcher->>ParserDispatcher: _resolve_backend(content_type, filename)
ParserDispatcher->>ParserDispatcher: _get(backend_name) → _build if cache miss
ParserDispatcher->>ConcreteParser: parse(document)
ConcreteParser-->>ParserDispatcher: ProcessedDocument
ParserDispatcher-->>ParserFileSerializer: ProcessedDocument
opt VLM configured and image_captioning enabled
ParserFileSerializer->>ParserFileSerializer: caption_images(vlm, text_blocks)
end
ParserFileSerializer-->>ConversionService: joined text blocks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
coderabbitai
Bot
added
breaking-change
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/content/docs/documentation/env_vars.md`:
- Line 25: The PDFLoader documentation has two issues to fix: First, remove the
duplicate `PyMuPDFLoader` entry from the available options list in the table row
so each loader (PyMuPDFLoader, MarkerLoader, and DotsOCRLoader) is listed only
once. Second, fix the subject-verb disagreement in the description by changing
"PyMuPDFLoader are lightweight" to "PyMuPDFLoader is a lightweight" to match the
singular noun form of the class name.
In `@openrag/services/workers/bootstrap.py`:
- Around line 67-77: The match statement in the get_marker_pool() function only
handles the "DoclingLoader" case, but the _PDF_BACKENDS dictionary in
parser_dispatcher.py also includes "DoclingLoader2" mapped to the "docling"
backend. This creates a contract mismatch where runtime config using
"DoclingLoader2" will cause the dispatcher to request a DoclingPool actor that
bootstrap never creates. Add a match case in get_marker_pool() to handle
"DoclingLoader2" that returns the same actor as "DoclingLoader", or
alternatively remove "DoclingLoader2" from _PDF_BACKENDS if it's no longer
supported per the deferred integration decision.
In `@openrag/services/workers/parsers/parser_dispatcher.py`:
- Around line 22-25: The logger import at lines 22-25 is using
core.utils.logging instead of the standardized openrag/utils/logger utility.
Change the import statement to use get_logger from openrag/utils/logger instead.
Additionally, the logging calls at lines 129 and 145 in the parser_dispatcher
module need to bind contextual information (backend, extension, and when
available file_id and partition) to the log messages to maintain queryability
and consistency with the repository's structured logging guidelines. Update each
logger call to include this bound context.
🪄 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: 2940d1b0-efa4-4119-8d36-0aa27a5c8544
📒 Files selected for processing (46)
README.mddocs/content/docs/documentation/env_vars.mddocs/content/docs/getting_started/quickstart.mdxopenrag/api/main.pyopenrag/api/routers/admin/tools.pyopenrag/core/indexing/serializer.pyopenrag/core/models/document.pyopenrag/di/container.pyopenrag/services/orchestrators/conversion_service.pyopenrag/services/workers/bootstrap.pyopenrag/services/workers/indexer_actor.pyopenrag/services/workers/indexer_pool.pyopenrag/services/workers/parsers/doc_serializer.pyopenrag/services/workers/parsers/doc_serializer_adapter.pyopenrag/services/workers/parsers/doc_serializer_bridge.pyopenrag/services/workers/parsers/file_serializer.pyopenrag/services/workers/parsers/legacy_loaders/CustomDocLoader.pyopenrag/services/workers/parsers/legacy_loaders/CustomHTMLLoader.pyopenrag/services/workers/parsers/legacy_loaders/__init__.pyopenrag/services/workers/parsers/legacy_loaders/audio/__init__.pyopenrag/services/workers/parsers/legacy_loaders/audio/local_whisper.pyopenrag/services/workers/parsers/legacy_loaders/audio/openai.pyopenrag/services/workers/parsers/legacy_loaders/base.pyopenrag/services/workers/parsers/legacy_loaders/doc.pyopenrag/services/workers/parsers/legacy_loaders/docx.pyopenrag/services/workers/parsers/legacy_loaders/eml_loader.pyopenrag/services/workers/parsers/legacy_loaders/image.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/__init__.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/docling.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/docling2.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/dotsocr.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/marker.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/openai.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/pymupdf.pyopenrag/services/workers/parsers/legacy_loaders/pptx_loader.pyopenrag/services/workers/parsers/legacy_loaders/txt_loader.pyopenrag/services/workers/parsers/parser_dispatcher.pytests/unit/services/workers/parsers/legacy_loaders/audio/test_openai.pytests/unit/services/workers/parsers/legacy_loaders/test_base_loader.pytests/unit/services/workers/parsers/legacy_loaders/test_customdocloader.pytests/unit/services/workers/parsers/legacy_loaders/test_doc_loader.pytests/unit/services/workers/parsers/legacy_loaders/test_docx_loader.pytests/unit/services/workers/parsers/legacy_loaders/test_eml_recursion.pytests/unit/services/workers/parsers/test_doc_serializer_bridge.pytests/unit/services/workers/parsers/test_parser_dispatcher.pytests/unit/services/workers/test_indexer_worker.py
💤 Files with no reviewable changes (30)
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/init.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/openai.py
- tests/unit/services/workers/parsers/legacy_loaders/test_customdocloader.py
- openrag/services/workers/parsers/legacy_loaders/docx.py
- tests/unit/services/workers/parsers/legacy_loaders/test_base_loader.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/pymupdf.py
- openrag/services/workers/parsers/legacy_loaders/base.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/docling2.py
- openrag/services/workers/parsers/legacy_loaders/image.py
- openrag/services/workers/parsers/legacy_loaders/txt_loader.py
- openrag/services/workers/parsers/legacy_loaders/CustomDocLoader.py
- openrag/services/workers/parsers/legacy_loaders/CustomHTMLLoader.py
- tests/unit/services/workers/parsers/test_doc_serializer_bridge.py
- openrag/services/workers/parsers/legacy_loaders/pptx_loader.py
- openrag/services/workers/parsers/legacy_loaders/init.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/docling.py
- openrag/services/workers/parsers/legacy_loaders/audio/init.py
- openrag/services/workers/parsers/doc_serializer_adapter.py
- openrag/services/workers/parsers/legacy_loaders/eml_loader.py
- tests/unit/services/workers/parsers/legacy_loaders/test_eml_recursion.py
- openrag/services/workers/parsers/doc_serializer_bridge.py
- openrag/services/workers/parsers/legacy_loaders/audio/openai.py
- openrag/services/workers/parsers/legacy_loaders/doc.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/dotsocr.py
- tests/unit/services/workers/parsers/legacy_loaders/test_docx_loader.py
- tests/unit/services/workers/parsers/legacy_loaders/audio/test_openai.py
- openrag/services/workers/parsers/doc_serializer.py
- openrag/services/workers/parsers/legacy_loaders/audio/local_whisper.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/marker.py
- tests/unit/services/workers/parsers/legacy_loaders/test_doc_loader.py
…file serializer Introduce ParserDispatcher, which routes each Document to a concrete parser in the core/indexing/parsers stack by content type, resolving the PDF and audio backends from config.loader.file_loaders so behaviour matches the GPU pools bootstrap provisions. Add ParserFileSerializer, an in-process FileSerializer that runs the dispatcher for the extractText path. Extend DocumentType detection with svg/gif/webp/bmp image extensions and update the FileSerializer port docstring to describe the in-process implementation.
…dispatcher Wire IndexerPool and the conversion service to the parser dispatcher and the in-process file serializer, and build the captioning VLM from config to inject into the indexing pipeline. Require metadata['file_id'] in _load_document so chunks persist under the caller's file id instead of a random uuid. Drop the DocSerializer actor from bootstrap and refresh the now-stale docstrings.
Delete the legacy_loaders stack and the DocSerializer actor / adapter / bridge shims, now that indexing and extractText run entirely on the parser dispatcher, together with their unit tests.
Ahmath-Gadji
force-pushed
the
refactor/remove-parser-shims
branch
from
June 18, 2026 12:26
18a8383 to
e2cc7cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/unit/services/workers/parsers/test_parser_dispatcher.py`:
- Around line 42-56: The parametrized test matrix in the
`@pytest.mark.parametrize` decorator only includes a single test case for
DocumentType.IMAGE with the .png extension, lacking coverage for the newly
supported image formats. Add four additional test cases to the parametrize
matrix for the newly supported image suffixes (.svg, .gif, .webp, and .bmp),
each paired with DocumentType.IMAGE as the content_type and "image" as the
expected_backend value, positioned alongside the existing .png case to ensure
comprehensive routing test coverage for all supported image formats.
🪄 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: 3b8daf5d-3820-4de4-831f-135770cfc93e
📒 Files selected for processing (46)
README.mddocs/content/docs/documentation/env_vars.mddocs/content/docs/getting_started/quickstart.mdxopenrag/api/main.pyopenrag/api/routers/admin/tools.pyopenrag/core/indexing/serializer.pyopenrag/core/models/document.pyopenrag/di/container.pyopenrag/services/orchestrators/conversion_service.pyopenrag/services/workers/bootstrap.pyopenrag/services/workers/indexer_actor.pyopenrag/services/workers/indexer_pool.pyopenrag/services/workers/parsers/doc_serializer.pyopenrag/services/workers/parsers/doc_serializer_adapter.pyopenrag/services/workers/parsers/doc_serializer_bridge.pyopenrag/services/workers/parsers/file_serializer.pyopenrag/services/workers/parsers/legacy_loaders/CustomDocLoader.pyopenrag/services/workers/parsers/legacy_loaders/CustomHTMLLoader.pyopenrag/services/workers/parsers/legacy_loaders/__init__.pyopenrag/services/workers/parsers/legacy_loaders/audio/__init__.pyopenrag/services/workers/parsers/legacy_loaders/audio/local_whisper.pyopenrag/services/workers/parsers/legacy_loaders/audio/openai.pyopenrag/services/workers/parsers/legacy_loaders/base.pyopenrag/services/workers/parsers/legacy_loaders/doc.pyopenrag/services/workers/parsers/legacy_loaders/docx.pyopenrag/services/workers/parsers/legacy_loaders/eml_loader.pyopenrag/services/workers/parsers/legacy_loaders/image.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/__init__.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/docling.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/docling2.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/dotsocr.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/marker.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/openai.pyopenrag/services/workers/parsers/legacy_loaders/pdf_loaders/pymupdf.pyopenrag/services/workers/parsers/legacy_loaders/pptx_loader.pyopenrag/services/workers/parsers/legacy_loaders/txt_loader.pyopenrag/services/workers/parsers/parser_dispatcher.pytests/unit/services/workers/parsers/legacy_loaders/audio/test_openai.pytests/unit/services/workers/parsers/legacy_loaders/test_base_loader.pytests/unit/services/workers/parsers/legacy_loaders/test_customdocloader.pytests/unit/services/workers/parsers/legacy_loaders/test_doc_loader.pytests/unit/services/workers/parsers/legacy_loaders/test_docx_loader.pytests/unit/services/workers/parsers/legacy_loaders/test_eml_recursion.pytests/unit/services/workers/parsers/test_doc_serializer_bridge.pytests/unit/services/workers/parsers/test_parser_dispatcher.pytests/unit/services/workers/test_indexer_worker.py
💤 Files with no reviewable changes (30)
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/init.py
- openrag/services/workers/parsers/doc_serializer.py
- tests/unit/services/workers/parsers/legacy_loaders/test_customdocloader.py
- openrag/services/workers/parsers/legacy_loaders/init.py
- openrag/services/workers/parsers/legacy_loaders/CustomDocLoader.py
- tests/unit/services/workers/parsers/legacy_loaders/test_base_loader.py
- openrag/services/workers/parsers/legacy_loaders/audio/local_whisper.py
- openrag/services/workers/parsers/legacy_loaders/CustomHTMLLoader.py
- openrag/services/workers/parsers/legacy_loaders/docx.py
- openrag/services/workers/parsers/legacy_loaders/audio/openai.py
- openrag/services/workers/parsers/doc_serializer_adapter.py
- tests/unit/services/workers/parsers/test_doc_serializer_bridge.py
- openrag/services/workers/parsers/legacy_loaders/audio/init.py
- openrag/services/workers/parsers/legacy_loaders/base.py
- tests/unit/services/workers/parsers/legacy_loaders/test_docx_loader.py
- tests/unit/services/workers/parsers/legacy_loaders/test_doc_loader.py
- openrag/services/workers/parsers/legacy_loaders/pptx_loader.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/docling.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/marker.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/pymupdf.py
- openrag/services/workers/parsers/legacy_loaders/txt_loader.py
- openrag/services/workers/parsers/legacy_loaders/image.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/openai.py
- openrag/services/workers/parsers/legacy_loaders/doc.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/docling2.py
- openrag/services/workers/parsers/doc_serializer_bridge.py
- openrag/services/workers/parsers/legacy_loaders/pdf_loaders/dotsocr.py
- tests/unit/services/workers/parsers/legacy_loaders/test_eml_recursion.py
- tests/unit/services/workers/parsers/legacy_loaders/audio/test_openai.py
- openrag/services/workers/parsers/legacy_loaders/eml_loader.py
✅ Files skipped from review due to trivial changes (7)
- README.md
- openrag/api/routers/admin/tools.py
- openrag/api/main.py
- docs/content/docs/documentation/env_vars.md
- openrag/core/indexing/serializer.py
- openrag/services/orchestrators/conversion_service.py
- docs/content/docs/getting_started/quickstart.mdx
🚧 Files skipped from review as they are similar to previous changes (8)
- openrag/core/models/document.py
- openrag/services/workers/parsers/file_serializer.py
- openrag/di/container.py
- openrag/services/workers/indexer_pool.py
- tests/unit/services/workers/test_indexer_worker.py
- openrag/services/workers/bootstrap.py
- openrag/services/workers/indexer_actor.py
- openrag/services/workers/parsers/parser_dispatcher.py
The parser migration removed the PyMuPDF4LLMLoader backend; update the README, env-vars reference, and quickstart so PyMuPDFLoader is the only lightweight PDF option listed.
Ahmath-Gadji
force-pushed
the
refactor/remove-parser-shims
branch
from
June 18, 2026 13:05
e2cc7cc to
40dcac6
Compare
…GE_CAPTIONING The legacy ImageLoader captioned standalone image files unconditionally — IMAGE_CAPTIONING only ever gated images *embedded* in other documents. Routing indexing through the parser dispatcher collapsed both into a single gate (build_caption_vlm returned None when captioning was off), so uploading an image with IMAGE_CAPTIONING=false produced no text and zero indexed documents (regressing the SVG indexing API test). Decouple VLM availability from caption policy: - build_caption_vlm builds the VLM whenever a VLM endpoint is configured. - IndexingPipeline._should_caption() always captions standalone image files, and gates embedded images by the global image_captioning flag plus the per-partition enable_image_captioning setting. - ParserFileSerializer (extractText) applies the same policy.
Lock the parser-migration contract that these extensions resolve to the image backend (the IMAGE content-type mapping added in this PR).
Ahmath-Gadji
force-pushed
the
refactor/remove-parser-shims
branch
from
June 18, 2026 13:21
1d66da5 to
b3e3faa
Compare
hedhoud
reviewed
Jun 19, 2026
Collaborator
There was a problem hiding this comment.
I reviewed this PR and found three points worth addressing before merge.
1. Nested .eml attachments are no longer parsed
.eml means an email file. Before, if an indexed email had another email attached inside it, OpenRAG could parse the attached email too.
With this PR, .eml attachments are excluded from the parser dispatcher. So for this kind of structure:
Email A
└── attached Email B
└── body text
Before, Email B body was indexed. Now, Email B body may be skipped and only the attachment header/metadata may remain.
This is fine if intentional, but then it should be documented. Otherwise, .eml parsing should be added back with a recursion limit so we avoid infinite nested emails.
2. Original filename is lost before parser dispatch
In indexer_actor.py, the document filename is set from the internal file_id. The problem is that file_id may not contain the real filename or extension.
Example:
real file: audio.flac
file_id: 123456789
The parser then sees 123456789, so it cannot know this was a .flac file. This is risky for audio/video files because parser selection can depend on extensions such as .mp4, .flac, or .ogg.
Better behavior would be to keep both concepts separate:
Document.id= OpenRAG internal file idDocument.filename= original uploaded filename, or the saved path filename
3. Large file reads block the async actor
The indexer actor is async, but the file content is read synchronously with read_bytes(). For small files this is not a big issue, but for large PDFs it can block the actor event loop and reduce parallelism.
This matters because even if Ray actor concurrency is configured, one large blocking file read can slow down other in-flight tasks. The read should be moved to a thread so the async actor can continue handling other work.
Merge note
Overall, I think the PR is directionally good, but the .eml regression and filename handling are important to clarify or fix before merge.
Resolve the parser-shim branch against the latest hexagonal work, restore bounded nested email attachment parsing, preserve original filenames for parser dispatch, and avoid blocking the async indexer actor on large file reads.
Collaborator
|
Quick update: I resolved the conflict with I’m not merging this myself. @Ahmath-Gadji could you review it when you have a moment? |
…eanup Resolves the indexer_actor.py conflict between the parser refactor and #529: keeps the async, file_id-required _load_document from the refactor and unions in #529's shared indexed_at threading (process_file reads row['indexed_at'] after pipeline.run and passes it to _write_catalog_record, which forwards it to both the add/update catalog writes). All other #529 changes (store stage, vector_store, milvus_store, document_repo, schema, migration) apply cleanly.
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
2 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.
What
Migrates document parsing entirely onto the new
core/indexing/parsersstack and removes the transitional shims, so there is a single parser path for both indexing and theextractTexttool.Before this PR, two parsing stacks coexisted: the legacy
legacy_loaders/(BaseLoaderregistry) reached through theDocSerializerRay actor + adapter + bridge, and the newDocumentParserregistry. This consolidates on the latter.Commits (reviewable in order)
ParserDispatcherroutes eachDocumentto a concrete parser bycontent_type, resolving the PDF/audio backends fromconfig.loader.file_loaders(so behaviour matches the GPU poolsbootstrapprovisions).ParserFileSerializerruns the dispatcher in-process forextractText. Addssvg/gif/webp/bmpimage detection.IndexerPooland the conversion service to the new stack, builds the captioning VLM from config and injects it into the indexing pipeline, and requiresmetadata['file_id']in_load_documentso chunks persist under the caller's file id.legacy_loaders/**and theDocSerializeractor / adapter / bridge, plus their tests.PyMuPDF4LLMLoader.Not included (separate PRs, by design)
file_idfor thedata_indexerdev scriptVerification
uv run ruff checkcleantest_seed_defaults_preserves_endpoint_api_keys) is pre-existing and environment-driven (a global API key overrides the per-endpoint seed defaults), unrelated to these changes.Summary by CodeRabbit
Release Notes
Documentation
PDFLoaderexamples/options to prioritizePyMuPDFLoader.New Features
image_captioningoption to control captioning for embedded images (standalone images can still be captioned when available).Refactoring
Bug Fixes
svg,gif,webp, andbmp.file_id, and internal metadata keys no longer appear in document metadata.