fix(indexer): honor preset parsing_strategy, lazy pools, pymupdf markdown, parse timeout - #582
Merged
Merged
Conversation
…timeout - #569 honor a preset's parsing_strategy: ParserDispatcher.for_pdf_strategy() + a parser_factory wired into the pipeline, so pymupdf/docling are reachable per preset instead of always using the global default backend. - #570 build pymupdf in markdown mode with embed_images=False: structured text for the chunker, no base64 bloat / Milvus gRPC overflow. - #571 bound the parse stage with loader.parse_timeout (PARSE_TIMEOUT, default 3600s) so a wedged parse fails that file instead of hanging indexing. Tests cover strategy dispatch, pymupdf markdown mode and the parse-timeout error.
…ackends (#575) A per-preset parsing_strategy can select a backend that isn't the global default, which bootstrap never pre-warmed, so the loader's get-only ray.get_actor failed with 'Failed to look up actor'. Create the pool on first use via get_or_create_actor, with get_if_exists=True for race-safe concurrent creation. Covers DoclingPool and MarkerPool.
📝 WalkthroughWalkthroughThe PR adds a configurable parse timeout, per-preset PDF parser strategy routing, lazy Ray actor creation for Docling and Marker pools, and PyMuPDF markdown parsing that omits embedded images. Tests were added or updated for timeout handling, routing, caching, and loader acquisition. ChangesIndexing parse and PDF routing
Sequence Diagram(s)sequenceDiagram
participant IndexerPool
participant ParserDispatcher
participant _PdfStrategyParser
participant PDFBackend
IndexerPool->>ParserDispatcher: for_pdf_strategy(strategy)
ParserDispatcher-->>IndexerPool: parser_factory
IndexerPool->>_PdfStrategyParser: parse(document)
_PdfStrategyParser->>PDFBackend: parse PDF document
_PdfStrategyParser-->>IndexerPool: parsed result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 `@openrag/core/config/indexation.py`:
- Around line 181-186: The parse_timeout config in the indexing settings should
be validated as strictly positive instead of accepting 0 or negative values.
Update the config model in the parse_timeout field definition so invalid values
are rejected at load time before reaching asyncio.wait_for, keeping the existing
ParseTimeout/indexation settings behavior but preventing immediate failures for
every file.
In `@openrag/services/workers/stages/parse.py`:
- Around line 42-48: The TimeoutError handling in parse() is incorrectly
rewrapping internal parser timeouts and can crash when timeout is None. Update
the try/except around run_with_optional_timeout so it only converts a
TimeoutError into the filename-based message when an actual timeout bound was
applied; if timeout is None, let the parser’s own TimeoutError propagate
unchanged. Use the parse() function and the run_with_optional_timeout call as
the key locations, and avoid formatting timeout with {timeout:g} unless timeout
is guaranteed to be numeric.
In `@tests/unit/services/workers/parsers/test_parser_dispatcher.py`:
- Around line 144-150: The test for ParserDispatcher’s pymupdf backend only
verifies _mode is markdown and misses the no-images contract. Update
test_pymupdf_backend_builds_in_markdown_mode_without_images to also assert the
parser is configured with embed_images disabled, or verify a parse result from
the pymupdf parser keeps images empty. Use ParserDispatcher and the
_get("pymupdf") setup to locate the relevant assertion area.
🪄 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: f37128dd-4a69-4d59-9d89-68c2989dafdc
📒 Files selected for processing (14)
conf/config.yamlopenrag/core/config/indexation.pyopenrag/core/config/loader.pyopenrag/core/indexing/parsers/pdf/pymupdf.pyopenrag/services/workers/bootstrap.pyopenrag/services/workers/indexer_pool.pyopenrag/services/workers/parsers/docling_workers.pyopenrag/services/workers/parsers/marker_workers.pyopenrag/services/workers/parsers/parser_dispatcher.pyopenrag/services/workers/stages/parse.pytests/unit/services/workers/parsers/test_parser_dispatcher.pytests/unit/services/workers/parsers/test_pool_loaders.pytests/unit/services/workers/stages/test_parse.pytests/unit/services/workers/test_indexer_pool.py
Contributor
Author
Testing independentlyUnit: Functional (API/logs, no admin UI needed):
|
It feeds asyncio.wait_for, so 0/negative would fail every parse immediately instead of disabling the bound. Reject at config load (CodeRabbit #582).
…s set
When timeout is None the parse stage applies no asyncio.wait_for, so a
TimeoutError can only be internal to the parser. Re-raise it as-is instead
of relabeling it 'parse timed out after {timeout}s' (which also crashed
formatting {timeout:g} on None). Test covers the timeout=None path.
Addresses CodeRabbit #582.
Strengthen the pymupdf test beyond _mode: build a PDF that contains an image and assert _extract_markdown returns no ImageBlocks and inlines no base64 data URI — catches a regression that re-enables embed_images. (CodeRabbit #582)
…set-parsing-strategy # Conflicts: # openrag/services/workers/bootstrap.py
…cing marker The default indexation preset hardcoded parsing_strategy="marker", and the new per-preset parser_factory routed every PDF through that strategy — overriding the operator's global file_loaders.pdf (PDFLoader) choice. On a pymupdf-configured deployment this forced marker, lazily spinning up the Marker Ray pool and loading models inside the indexer actor; on a GPU-less/CPU runner the parse stage never completes and indexing hangs. Make parsing_strategy optional: None now means "inherit the global PDFLoader". The default preset omits it (so it follows the deployment's configured backend); named presets (legal/finance) keep their explicit marker opt-in. _select_parser defers to the global dispatcher when no strategy is set.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/core/config/indexation_pipeline.py (1)
20-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep PDF parsing strategy values in one place —
PARSING_STRATEGIES, theLiteral[...], andParserDispatcher._PDF_BACKEND_NAMEScan drift independently. Derive one from the other or add an import-time assertion so the accepted values stay aligned.🤖 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 `@openrag/core/config/indexation_pipeline.py` around lines 20 - 30, The PDF parsing strategy values are duplicated across PARSING_STRATEGIES, IndexationPipelineConfig.parsing_strategy, and ParserDispatcher._PDF_BACKEND_NAMES, so they can drift out of sync. Make the accepted strategy list come from a single source of truth by deriving the Literal-backed config and dispatcher names from PARSING_STRATEGIES, or add an import-time assertion that compares IndexationPipelineConfig and ParserDispatcher against it. Use the symbols PARSING_STRATEGIES, IndexationPipelineConfig, parsing_strategy, and ParserDispatcher._PDF_BACKEND_NAMES to keep the values aligned.
🤖 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.
Nitpick comments:
In `@openrag/core/config/indexation_pipeline.py`:
- Around line 20-30: The PDF parsing strategy values are duplicated across
PARSING_STRATEGIES, IndexationPipelineConfig.parsing_strategy, and
ParserDispatcher._PDF_BACKEND_NAMES, so they can drift out of sync. Make the
accepted strategy list come from a single source of truth by deriving the
Literal-backed config and dispatcher names from PARSING_STRATEGIES, or add an
import-time assertion that compares IndexationPipelineConfig and
ParserDispatcher against it. Use the symbols PARSING_STRATEGIES,
IndexationPipelineConfig, parsing_strategy, and
ParserDispatcher._PDF_BACKEND_NAMES to keep the values aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57588816-6f6e-45e4-a041-06dadfd4344d
📒 Files selected for processing (4)
openrag/api/routers/admin/presets.pyopenrag/core/config/indexation_pipeline.pyopenrag/services/orchestrators/preset_service.pyopenrag/services/workers/pipeline_builder.py
Comment on lines
+67
to
+68
| pages = [(chunk.get("text") or "").strip() for chunk in chunks] | ||
| return pages, [] |
Collaborator
There was a problem hiding this comment.
The issue with this implementation is that we're dropping embedded images, even though this parser is capable of handling them. Do we really want to do that?
We can avoid embedding images inside the markdown, but preserve ImageBlocks.
Collaborator
There was a problem hiding this comment.
Since pymupdf image cropping quality has not been validated and compared to other advanced parsers like (docling & marker), we've decided to remove the embedded images until that validation.
Collaborator
|
Found while testing this PR: docling parses run CPU-only on a GPU host Exercising the new per-preset and slow parses. Root cause: It's a pre-existing docling bug that this PR surfaces (docling was rarely reached before per-preset routing). Fixed separately in #584, which makes |
Ahmath-Gadji
approved these changes
Jun 29, 2026
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
Collaborator
There was a problem hiding this comment.
LGTM.
I've tested it and it works
This was referenced Jun 29, 2026
Closed
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.
Makes the indexing pipeline honor a partition preset's
parsing_strategyand hardens the PDF parse stage. Four related parser fixes (entangled in shared files, so one PR; #575 is a separate commit).parsing_strategy:ParserDispatcher.for_pdf_strategy()+ a parser_factory wired into the pipeline, so pymupdf/docling are reachable per preset instead of always using the global default.get_or_create_actor+get_if_exists) so a per-preset backend works even when it isn't the global default and bootstrap didn't pre-warm it.embed_images=False: structured text for the chunker, no base64 bloat / Milvus gRPC overflow.loader.parse_timeout(PARSE_TIMEOUT, default 3600s) so a wedged parse fails that file instead of hanging indexing.Tests: strategy dispatch, lazy pool creation, pymupdf markdown mode, parse-timeout error. Refs #569 #570 #571 #575.
Summary by CodeRabbit
parse_timeout(default 3600s) to bound per-file parsing, withPARSE_TIMEOUTenvironment override support.