Retriever seam + shared QueryEngine + eval convergence (issue #16, step 4)#21
Open
elkaix wants to merge 5 commits into
Open
Retriever seam + shared QueryEngine + eval convergence (issue #16, step 4)#21elkaix wants to merge 5 commits into
elkaix wants to merge 5 commits into
Conversation
Step 4a of issue #16. Introduce a runtime-checkable Retriever Protocol (retrieve(query, top_k) -> list[SearchResult]) with adapters: - DenseRetriever wraps ChromaVectorStore (the default retrieval strategy) - BM25HybridRetriever conforms directly - RerankingRetriever / MultiQueryRetriever compose an inner Retriever (over-fetch+rerank, and fan-out+dedup respectively) Promote the four eval-proven levers (bm25_hybrid, reranker, query_rewriter, refusal_handler) from src/eval/ to a core src/retrieval/ package via git mv, so production can activate them by configuration without importing the eval package -- the same dependency-direction fix as ADR 0003. Update eval import sites and rename the moved modules' tests to test_retrieval_*. Behavior-preserving: production does not consume the seam yet (that is 4b). Full suite green (311 passed).
Step 4b of issue #16. Introduce src/query_engine/ — a deep module owning retrieve->generate for both the sync and streaming query paths: - prompt.py: the single Markdown answer prompt (the sync path adopts it, so both paths issue identical instructions), the streaming planning prompt, and a filename-prefixed context builder. - telemetry.py: StageTelemetry assembly from provider-reported Usage, in one place (was duplicated across four backend sites). - engine.py: QueryEngine.ask (sync) and ask_stream (streaming) share those helpers; the reasoning pass stays stream-only so sync keeps its single LLM call. An optional refusal gate is off by default. The Retriever is injected via a build_retriever factory selected by a new config field RETRIEVER_STRATEGY (default "dense", behaviour-preserving; "reranked" wired; "hybrid"/"multi_query" recognised but deferred). RAGBackend now delegates query/query_with_telemetry/stream_query to self.query_engine and owns only conversation persistence, deferred to the stream's terminal result event so the empty-store path persists nothing (as before). Behaviour-preserving: test_backend.py and test_backend_telemetry.py pass unchanged. Full suite green (325 passed). Engine unit tests assert sync and streaming produce identical answer instructions.
Step 4c of issue #16. EvalPipeline now delegates retrieve->generate to a QueryEngine instead of re-implementing it: the levers (hybrid/dense, rewrite, rerank) compose into one Retriever behind the seam, and the refusal gate, prompt, context, and telemetry come from production's modules. Eval measures the shipped pipeline. - Delete eval's divergent answer prompt and bare-join context; the engine's single Markdown prompt + filename-prefixed context are the only ones now. - EvalConfig chunk/overlap/top-k/model/judge defaults derive from src/config.py (single source of truth): chunk defaults move 512/64 -> 500/50. - Translate the engine's StageTelemetry to eval's dict; timings collapse from per-lever stages to retrieve/generate; the dead rewriter_cost_usd field is dropped (verified zero readers). - Reorder the engine's refusal gate before the no-documents branch so an empty retrieval is an answerability signal (production unchanged — gate off). - Add ADR 0004, glossary entries, and an eval<->production parity test that pins eval to the shipped prompt + context builders. Full suite green (327 passed). Production backend tests unchanged.
Standards: - Split QueryEngine below the 250-line ceiling: StreamResult, the event alias, and the streaming helpers move to src/query_engine/streaming.py. - Drop the two `Any`s: ask_stream/_stream_reasoning yield a typed StreamEvent (str | StreamResult); the facade narrows with isinstance and an invariant assert. Tighten list[dict] -> list[dict[str, str]]. - Extract _source_dict() so the two query paths stop duplicating the source-citation shape (the sync path adds chunk_index on top). - Name the rewriter expansion unpack instead of a magic [0]. Spec: - Unify chunking config: production ingestion now reads CHUNK_SIZE/ CHUNK_OVERLAP from src/config.py (set to production's actual 512/64), and the eval harness already derives from the same source -- so "baseline" eval measures production's chunking. Behaviour-preserving. - Fix a stale WHY comment in _get_sliding_window (persistence moved to the terminal result event, not "before streaming"). - Correct the query_rewriter cost note: 4c dropped the dead rewriter_cost_usd (zero readers), it is not "deferred". Full suite green (327 passed); production backend tests unchanged.
Follow-up honesty notes on ADR 0004: - The chunking config unification has NO behaviour delta on either side — production and eval both stay 512/64; only the previously-eval-only config.CHUNK_SIZE constant was realigned to production's real values. - Note the one remaining latent eval change: with an empty index and no refusal handler, an eval query now returns the no-documents sentinel instead of generating from empty context (eval always ingests first).
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
1 participant
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.
Step 4 of the RAG architecture deepening spec (#16): introduce a Retriever seam, extract a shared QueryEngine that both production and the eval harness call, and converge the eval pipeline onto it. Lands in leaf-first, behaviour-preserving sub-phases.
What & why
The system had no seam between retrieval and generation, so eval-proven levers (hybrid/rerank/rewrite/refusal) couldn't ship, and three diverged answer prompts meant eval measured a pipeline that wasn't the one served.
src/retrieval/) — a runtime-checkableRetrieverProtocol (retrieve(query, top_k) -> list[SearchResult]). Dense and hybrid conform directly; reranking and multi-query compose an inner Retriever. The four levers were promoted fromsrc/eval/to core so production can activate them by configuration (same dependency-direction fix as ADR 0003).src/query_engine/) — one deep module owns retrieve→generate for both the sync (ask) and streaming (ask_stream) paths: a single Markdown answer prompt for every path, filename-prefixed context, telemetry assembled once, and an optional (off-by-default) refusal gate.RAGBackenddelegates to it and keeps only conversation persistence.EvalPipelinedelegates retrieve→generate to aQueryEngine; its divergent prompt/context copies are deleted, andEvalConfigdefaults derive fromsrc/config.py. A parity test pins eval to the shipped prompt + context builders.Behaviour preservation
tests/test_backend.pyandtests/test_backend_telemetry.pypass unchanged — the facade contract (query/query_with_telemetry/stream_query, the streaming event protocol, the empty-store path) is intact. Full suite: 327 passed (was 304; +23 new tests). Production chunking is unchanged (512/64, now single-sourced from config).Key decisions (see
docs/adr/0004-retriever-seam-and-query-engine.md)hybridandmulti_queryare recognised in the production factory but wiring is deferred (they raise a clear error, no silent fallback) — hybrid needs a live BM25 corpus synced to ingestion. Both are exercised in eval via the seam.retrieve/generate(the engine assembles it once); the deadrewriter_cost_usdfield is dropped (verified zero readers).Testing
TDD at pre-agreed seams: adapter contract tests → engine tests with a fake Retriever + fake LLM (asserting sync and streaming issue identical instructions) → eval↔production parity test. ruff + mypy clean on all changed files.
Run:
CI_LLM_MOCK=1 python -m pytest tests/ -vFollow-ups (later spec steps)
RAGBackendintoConversationStore+Evaluatorbehind the thin facade.src/query_engine/streaming.pymodule is the seam its event enum lands in).