feat(presets): expose parsing strategies via /presets/options - #519
Conversation
The admin UI hardcoded the parsing-strategy dropdown (marker, pymupdf) and so drifted from IndexationPipelineConfig's Literal, which also accepts docling. Add parsing_strategies to PresetOptionsResponse, derived from the validated Literal via typing.get_args() so the exposed list can never drift from what the config accepts. The UI can now drive the dropdown from this like the chunking/ retrieval/reranker options. Closes #518
|
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 (3)
📝 WalkthroughWalkthroughThe ChangesExpose parsing_strategies on /presets/options
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Read parsing_strategies from the options endpoint (#519) instead of a hardcoded marker/pymupdf list, so the choices match the backend (incl. docling) and can't drift. Falls back to [marker, pymupdf] when the backend doesn't yet return the field, so it stays correct until that change deploys.
There was a problem hiding this comment.
LGTM — clean and correctly scoped. Verified the derivation resolves to ["pymupdf", "marker", "docling"] and that deriving from IndexationPipelineConfig's Literal (rather than parser_registry.list_registered()) is the right source of truth, since the registry also holds non-PDF/auto-dispatched parsers (docx, image, eml, audio, …). CI is green and the new test covers it.
Verified live end-to-end
Against a running instance I exercised the full create → attach → resolve flow:
GET /presets/options→parsing_strategies: ["pymupdf", "marker", "docling"]✅POST /presets/with{"name":"pymupdf-fast","preset_type":"indexation","config":{"parsing_strategy":"pymupdf"}}→201PATCH /partition/{p}with{"indexation_preset":"pymupdf-fast"}→200GET /partition/{p}/config→indexation_preset: pymupdf-fast, resolvedindexation_pipeline.parsing_strategy: pymupdf✅
So the option flows correctly through preset creation, partition assignment, and resolved config. (Step 4 stops at the resolved config on purpose — see the gap below for why an actual parse wouldn't yet reflect it.)
One known gap (out of scope for this PR) — described against this branch (refactor/hexagonal)
The parsing_strategy this endpoint now exposes is not yet honored at parse time on this branch. The per-partition indexation_config is plumbed all the way into the pipeline, and the chunker and embedder already respect it (their factories are wired). The parser is the exception:
IndexerPoolbuilds a singleparser = DocSerializerBridgeParser(config=cfg)and callsbuild_indexing_pipeline(parser=..., chunker_factory=..., embedder_factory=...)without aparser_factory(openrag/services/workers/indexer_pool.py).- Because no
parser_factoryis wired,_select_parser(config)(openrag/services/workers/pipeline_builder.py) never reaches itsconfig.parsing_strategybranch and just returns that single bridge parser. Thatconfig.parsing_strategyline is, in fact, the only place the field is read in the whole parse path on this branch — and it's currently dead. - The bridge then selects the concrete loader by file extension / mimetype from
get_loader_classes(config), which reads the globalconfig.loader.file_loadersmap. The per-file override (_legacy_loader_config) only toggles image captioning — it never touches the PDF loader.
Net effect: a partition set to pymupdf still parses PDFs with whatever the global file_loaders.pdf is. Exposing the option here is still correct and useful — it just isn't wired into actual parsing yet.
Why the fix waits for #513
#513 replaces DocSerializerBridgeParser with the new ParserDispatcher, but keeps the same shape: IndexerPool still builds one global dispatcher and passes no parser_factory, and the dispatcher resolves the PDF backend from the global config.loader.file_loaders.pdf. So the gap carries straight over to the consolidated parser path. Fixing it on this branch would mean wiring the now-legacy bridge, only to rewrite it once #513 lands.
A separate PR will therefore wire the partition's parsing_strategy into the parse path after #513 is merged, using the existing (currently-unused) parser_factory / _select_parser seam with the global loader as fallback. It must also address the operational side: marker/docling require provisioned GPU pools, so the follow-up should validate the selected strategy against available backends (or document the provisioning requirement).
Approving so this can merge; the wiring is tracked as the follow-up above.
Read parsing_strategies from the options endpoint (#519) instead of a hardcoded marker/pymupdf list, so the choices match the backend (incl. docling) and can't drift. Falls back to [marker, pymupdf] when the backend doesn't yet return the field, so it stays correct until that change deploys.
What
Exposes
parsing_strategiesonGET /presets/optionsso the admin preset editor can drive its parsing-strategy dropdown from the backend instead of a hardcoded list.Closes #518.
Why
The UI hardcoded
marker+pymupdfand had drifted fromIndexationPipelineConfig.parsing_strategy = Literal["pymupdf", "marker", "docling"]—doclingwas missing. Chunking/retrieval/reranker options already come from this endpoint; parsing didn't.Change
PresetOptionsResponsegainsparsing_strategies: list[str]./optionshandler populates it fromget_args(IndexationPipelineConfig.model_fields["parsing_strategy"].annotation)— derived from the validatedLiteral, so the option list can never drift from what the config accepts.Test
test_preset_options_crud_and_renamenow assertsparsing_strategies == {pymupdf, marker, docling}. Ruff clean; response model verified to build with the derived list.Follow-up
Frontend switch (read
parsing_strategiesfrom options instead of the hardcoded<SelectItem>s) lands separately on the admin-UI branch.Summary by CodeRabbit
New Features
Tests