fix(indexer): resolve named embedder and VLM endpoints - #564
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesMulti-model endpoint registry hydration and factory wiring
Sequence Diagram(s)sequenceDiagram
participant process_file as IndexerPool.process_file
participant freshness as _ensure_registry_fresh
participant decision as _reload_decision
participant reload as _reload_registry
participant embedder_factory as _build_embedder_factory
participant vlm_factory as _build_vlm_factory
participant pipeline as IndexingPipeline
process_file->>freshness: required endpoint names
freshness->>decision: embedder/llm/vlm map
decision->>decision: check registries and fallbacks
decision-->>freshness: reload result
freshness->>reload: refresh registries
process_file->>embedder_factory: build embedder client
process_file->>vlm_factory: build VLM client
process_file->>pipeline: pass factories
pipeline->>pipeline: select VLM with default fallback
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/services/workers/pipeline_builder.py (1)
148-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVLM selection logic is correct and matches the fail-loud/fallback contract.
Explicit
config.vlmpropagatesKeyError(fail loudly), while the unset case falls back toself.vlm. This aligns with thevlm_factorycontract inindexer_pool.pyand the two regression tests.One small consistency note:
_select_contextualizer(Line 184) and_select_topic_tagger(Line 199) emit alogger.warningwhen they swallowKeyError, but the silent"default"fallback here logs nothing. A debug-level line would make the bypass observable without changing behavior.♻️ Optional: log the default-VLM fallback
try: return self.vlm_factory("default") - except KeyError: + except KeyError as exc: + logger.debug(f"No default VLM in registry; using preconfigured VLM instance ({exc})") return self.vlm🤖 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/services/workers/pipeline_builder.py` around lines 148 - 155, The default VLM fallback in PipelineBuilder is currently silent, unlike _select_contextualizer and _select_topic_tagger which log when they catch KeyError. Update the VLM selection branch that calls self.vlm_factory("default") to emit a debug-level log before or when falling back so the bypass is observable, while keeping the existing behavior of returning self.vlm on KeyError and preserving the explicit config.vlm fail-loud path.
🤖 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/services/workers/pipeline_builder.py`:
- Around line 148-155: The default VLM fallback in PipelineBuilder is currently
silent, unlike _select_contextualizer and _select_topic_tagger which log when
they catch KeyError. Update the VLM selection branch that calls
self.vlm_factory("default") to emit a debug-level log before or when falling
back so the bypass is observable, while keeping the existing behavior of
returning self.vlm on KeyError and preserving the explicit config.vlm fail-loud
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4878dcf4-7381-4e13-817f-231079b5d900
📒 Files selected for processing (6)
openrag/services/orchestrators/model_endpoint_service.pyopenrag/services/workers/indexer_pool.pyopenrag/services/workers/pipeline_builder.pytests/unit/services/orchestrators/test_model_endpoint_service.pytests/unit/services/workers/test_indexer_pool.pytests/unit/services/workers/test_pipeline_builder.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/services/workers/test_model_endpoint_registry_e2e.py (1)
195-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
image_captioningparameter is ignored at pipeline-build time.The
image_captioningargument (defaultFalse) only flows intoenable_image_captioningon Line 200, whilebuild_indexing_pipelineis always called with a hardcodedimage_captioning=Trueon Line 195. Tests still pass because captioning policy is gated by the config, but a caller passingimage_captioning=Falsewould not get a captioning-disabled pipeline as the name implies. Consider threading the parameter through for consistency.♻️ Proposed change
- image_captioning=True, + image_captioning=image_captioning,🤖 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/services/workers/test_model_endpoint_registry_e2e.py` around lines 195 - 200, The image_captioning argument is being ignored when constructing the indexing pipeline, since build_indexing_pipeline is always called with a hardcoded True while enable_image_captioning is set from the parameter later. Update the pipeline setup in test_model_endpoint_registry_e2e.py so the build_indexing_pipeline call uses the image_captioning variable consistently, and keep the related enable_image_captioning wiring in sync so callers can actually disable captioning through the parameter.
🤖 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 `@tests/unit/services/workers/test_model_endpoint_registry_e2e.py`:
- Around line 195-200: The image_captioning argument is being ignored when
constructing the indexing pipeline, since build_indexing_pipeline is always
called with a hardcoded True while enable_image_captioning is set from the
parameter later. Update the pipeline setup in
test_model_endpoint_registry_e2e.py so the build_indexing_pipeline call uses the
image_captioning variable consistently, and keep the related
enable_image_captioning wiring in sync so callers can actually disable
captioning through the parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b911fb4-dea0-4149-8715-4123259ed4bf
📒 Files selected for processing (1)
tests/unit/services/workers/test_model_endpoint_registry_e2e.py
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Manual end-to-end verification
I exercised this branch against a live deployment built from the PR (real OpenAI-compatible inference backend) to confirm named embedder/VLM resolution behaves as intended at index time. Endpoints/models are anonymized below as placeholders.
Setup
Registered endpoints in the DB-backed model-endpoint registry, then created three partitions, each pointed at a different embedder:
default— partition uses the default embedder.embedder-A— partition pointed at a valid named endpoint (distinct registry entry).embedder-bogus— partition pointed at a named endpoint whose URL is intentionally unreachable (http://127.0.0.1:1/v1), everything else valid.
For each partition I indexed a small document and checked which endpoint the embedding request actually targeted, per file.
Results
- Named resolution works. Partitions (1) and (2) resolved their configured embedder from the registry and indexed successfully. This also confirms the indexer lazily hydrates
cfg.modelsfrom the DB — the registry starts empty in the indexer process, which is precisely the gap this PR closes. - Routing is correct (decisive check). Partition (3)'s embedding request went to its own configured (unreachable) endpoint and failed there — it did not silently fall back to the working default. Under the pre-fix behavior (inert embedder factory → silent default fallback), this partition would have wrongly succeeded against the default endpoint. It didn't, which is the proof the fix routes per-partition correctly.
- Fail-loud on misconfiguration. A partition pointed at an unregistered embedder name fails the file loudly instead of indexing with the wrong model; a named endpoint with a bad credential fails at the embedding call (auth error) rather than silently substituting another model. This matters because a wrong embedding model corrupts a partition's vectors invisibly.
- VLM path. Image captioning resolved the configured VLM endpoint and made a real call only when a document actually contained an image; text-only files correctly made no VLM call.
Principle of the test
Because a correctly-routed call and a silently-misrouted call look identical when the named endpoint and the default point at the same backend, the routing check is only meaningful when they diverge. So the key partition deliberately uses an endpoint that must fail if (and only if) it is actually used — confirming the indexer contacts each partition's own configured endpoint, not the default.
Note: OpenRAG uses one shared Milvus collection with a single embedding dimension (partition =
partition_keyrow tag), so a named embedder must match the collection's dimension. Worth documenting, but independent of this PR.
Net: named embedder/VLM endpoint resolution works as intended — each partition indexes through exactly its configured endpoint (hydrated from the registry), and fails loud on a bad/missing endpoint instead of silently using the default.
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
This is good for merge
2 participants
Context
This fixes the remaining model-endpoint registry gap in the refactored indexer. After #560, named LLM endpoints were resolved correctly for contextualization and topic tagging, but named embedders and VLMs could still be missed at index time.
Why this matters
The embedder case is especially risky because indexing can appear successful while using the wrong vector model. That leaves retrieval silently degraded or incompatible with the partition configuration. VLM selection had a similar Admin UI mismatch for image captioning.
What changed
The indexer now refreshes the hydrated model registry for embedder and VLM requirements too. Named embedders and named VLMs are resolved dynamically from the live registry, cached by full endpoint identity, and rebuilt when endpoint config changes. Explicit named endpoint misses fail loudly instead of falling back to another model.
Default env/config fallback remains available for deployments that have not configured named endpoints. Seeded default endpoints now preserve the configured timeout and embedder batch size so the DB-backed default stays faithful to the running environment.
Validation
uv run --no-env-file pytest tests/unit -quv run --no-env-file ruff check openrag/services/orchestrators/model_endpoint_service.py openrag/services/workers/indexer_pool.py openrag/services/workers/pipeline_builder.py tests/unit/services/orchestrators/test_model_endpoint_service.py tests/unit/services/workers/test_indexer_pool.py tests/unit/services/workers/test_pipeline_builder.pygit diff --checkCloses #562
Summary by CodeRabbit
New Features
Bug Fixes
batch_sizeandtimeoutvalues for embedder/LLM/VLM/reranker.Tests