Add configurable hybrid/auto/dense retrieval to the service query path#2319
Add configurable hybrid/auto/dense retrieval to the service query path#2319mahikaw wants to merge 3 commits into
Conversation
Introduce a server-owned vectordb.retrieval_mode (dense | hybrid | auto) for the service /v1/query endpoint, keeping dense as the default for backward compatibility. Hybrid combines LanceDB vector search with BM25/FTS; auto inspects the table and selects dense or hybrid based on available indexes. Evidence responses now report semantic for dense and semantic + lexical for hybrid. MCP inherits this since it proxies to the same route. - Add vectordb.retrieval_mode to service config, bundled retriever-service.yaml, and Helm (serviceConfig.vectordb.retrievalMode) - Extend vectordb_app with --retrieval-mode and mode-aware search via the existing LanceDB.retrieval() path; build FTS indexes on write when hybrid/auto is configured - Wire retrieval mode through the vectordb deployment and note in the MCP query tool that retrieval mode is server-owned - Cover dense/hybrid/auto in service vectordb tests
Greptile SummaryThis PR adds configurable
|
| Filename | Overview |
|---|---|
| nemo_retriever/src/nemo_retriever/service/vectordb_app.py | Core change: adds hybrid/auto retrieval via VectorDBState.retrieval_mode, _ensure_hybrid_indexes, and resolve_effective_retrieval_mode. Two defects: (1) if FTS build raises during init, the outer catch silently swallows it while _table_exists is already True; (2) bare except Exception: return in _ensure_hybrid_indexes drops open-table failures with no logging. |
| nemo_retriever/src/nemo_retriever/service/config.py | Adds ServiceVectorRetrievalMode Literal type and retrieval_mode field to VectorDbConfig with a Pydantic Field and sensible default — clean, backward-compatible addition. |
| nemo_retriever/tests/test_service_vectordb_hybrid_integration.py | New integration test file with @pytest.mark.integration on all six tests — exercises real LanceDB I/O for FTS index building and end-to-end hybrid/auto query paths. |
| nemo_retriever/tests/test_service_vectordb_app.py | Adds unit tests for retrieval mode config, health reporting, hybrid strategy propagation, and 422 on missing FTS index — good coverage of new branches. |
| nemo_retriever/helm/templates/configmap.yaml | Threads retrievalMode into the Helm-rendered YAML configmap with a |
| nemo_retriever/helm/templates/deployment-vectordb.yaml | Adds --retrieval-mode CLI arg to the vectordb container spec with a sensible dense default. |
| nemo_retriever/helm/values.yaml | Adds retrievalMode: dense with inline comments explaining the three options — clean values addition. |
| nemo_retriever/src/nemo_retriever/service/cli.py | Adds --vectordb-retrieval-mode CLI override; Pydantic validation in load_config will reject invalid values. |
| nemo_retriever/src/nemo_retriever/service/mcp_server.py | One-line doc update clarifying that retrieval mode is server-owned — no functional change. |
| nemo_retriever/src/nemo_retriever/service/routers/ingest.py | Minimal summary string update to the /query endpoint — no functional change. |
Comments Outside Diff (1)
-
nemo_retriever/src/nemo_retriever/service/vectordb_app.py, line 152-160 (link)FTS build failure silently poisons startup state
When
retrieval_mode == "hybrid",_table_existsis set toTrueon line 154 before_ensure_hybrid_indexes()is called on line 158. If_ensure_hybrid_indexes()raises — for example becauseinspect_lancedb_table_objectorvdb.write_to_indexthrows — the outerexcept Exception:swallows the exception and logs "LanceDB table does not exist yet", even though_table_existsis alreadyTrue. The service then starts in a broken state: health reportstable_exists=True, hybrid queries immediately return 422 ("both a vector column and FTS index are required"), and the startup log gives no actionable clue. Moving_ensure_hybrid_indexes()outside the "table doesn't exist" try/except, or at minimum re-raising on FTS failure, would prevent this silent corruption of startup state.Prompt To Fix With AI
This is a comment left during a code review. Path: nemo_retriever/src/nemo_retriever/service/vectordb_app.py Line: 152-160 Comment: **FTS build failure silently poisons startup state** When `retrieval_mode == "hybrid"`, `_table_exists` is set to `True` on line 154 before `_ensure_hybrid_indexes()` is called on line 158. If `_ensure_hybrid_indexes()` raises — for example because `inspect_lancedb_table_object` or `vdb.write_to_index` throws — the outer `except Exception:` swallows the exception and logs "LanceDB table does not exist yet", even though `_table_exists` is already `True`. The service then starts in a broken state: health reports `table_exists=True`, hybrid queries immediately return 422 ("both a vector column and FTS index are required"), and the startup log gives no actionable clue. Moving `_ensure_hybrid_indexes()` outside the "table doesn't exist" try/except, or at minimum re-raising on FTS failure, would prevent this silent corruption of startup state. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
nemo_retriever/src/nemo_retriever/service/vectordb_app.py:152-160
**FTS build failure silently poisons startup state**
When `retrieval_mode == "hybrid"`, `_table_exists` is set to `True` on line 154 before `_ensure_hybrid_indexes()` is called on line 158. If `_ensure_hybrid_indexes()` raises — for example because `inspect_lancedb_table_object` or `vdb.write_to_index` throws — the outer `except Exception:` swallows the exception and logs "LanceDB table does not exist yet", even though `_table_exists` is already `True`. The service then starts in a broken state: health reports `table_exists=True`, hybrid queries immediately return 422 ("both a vector column and FTS index are required"), and the startup log gives no actionable clue. Moving `_ensure_hybrid_indexes()` outside the "table doesn't exist" try/except, or at minimum re-raising on FTS failure, would prevent this silent corruption of startup state.
Reviews (3): Last reviewed commit: "fixes" | Re-trigger Greptile
Summary
Adds configurable hybrid retrieval to the NeMo Retriever service query path while keeping dense as the default for backward compatibility.
Service /v1/query (and MCP, which proxies to it) can now run in dense, hybrid, or auto mode via vectordb.retrieval_mode. Hybrid combines LanceDB vector search with BM25/FTS; auto inspects the table and selects dense or hybrid based on available indexes. Evidence responses report semantic for dense and semantic + lexical for hybrid.
Changes
Add vectordb.retrieval_mode to service config, bundled retriever-service.yaml, and Helm (serviceConfig.vectordb.retrievalMode)
Extend vectordb_app with --retrieval-mode and mode-aware search using the existing LanceDB.retrieval() path
Build FTS indexes on write when hybrid/auto is configured
Wire retrieval mode through the vectordb deployment and document MCP behavior (retrieval mode is server-owned)
Description
Checklist