From 507ad5cbaa9c8080715ea7f7b857504eef2a6667 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 8 Sep 2026 19:54:32 +0000 Subject: [PATCH] Plan the retriever rewrite in three stages Stages the work so a structural change and a behaviour change are never in the same commit: 1 BaseRetriever with vendored RRF. Query expansion and SelfQuery both stay. This changes how the code is arranged, not what it returns, so the exit criterion is that retrieval_baseline shows ZERO difference. 2 D1: plain semantic search replaces SelfQuery. Retrieval drops to one LLM call per message. Differences must be confined to the vector side. 3 D2/D3: the budget and the bundle become arguments, which removes the B008 suppression and the four retrievers.*.rag mypy baseline entries. Each stage ends by answering a real question through the full chain via both invoke and ainvoke -- not by testing the retriever in isolation, which is the failure mode that recurred most this week and is Article I of the constitution. Deliberately not generated: research.md, data-model.md, contracts/. There are no unresolved unknowns (D1-D4 are settled with evidence in the spec) and the feature introduces no data model or external contract, so the scaffolding would be ceremony. The constitution says Spec Kit is for decisions, not paperwork. Complexity tracking records four rejected simplifications and why, including keeping metadata_info.py: it is not dead, because the evaluator and the baseline harness still construct SelfQuery, and deleting it would break the tool that measures stage 2. Every factual claim was checked against the code: userguide is already a plain BaseRetriever, the four collections hold 121,291 documents, RRF is weight/(rank+60) counting from 1 and de-duplicating on page_content, and there are four mypy baseline entries rather than three. Co-Authored-By: Claude Opus 5 --- specs/001-retriever-rewrite/plan.md | 152 ++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 specs/001-retriever-rewrite/plan.md diff --git a/specs/001-retriever-rewrite/plan.md b/specs/001-retriever-rewrite/plan.md new file mode 100644 index 00000000..f183ea34 --- /dev/null +++ b/specs/001-retriever-rewrite/plan.md @@ -0,0 +1,152 @@ +# Implementation Plan: Retriever Rewrite + +**Branch**: `plan/retriever-rewrite` | **Date**: 2026-09-08 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `/specs/001-retriever-rewrite/spec.md` + +## Summary + +Replace `HybridRetriever`, which subclasses `MultiQueryRetriever` and reaches into +LangChain internals in five places, with a plain `BaseRetriever` that owns its own +fusion. Swap `SelfQueryRetriever` for plain semantic search (D1), keep BM25, and +make the context budget an argument rather than a module constant (D2, D3). + +The work is deliberately staged so that a behaviour change and a structural change +are never in the same commit. Stage 1 is a pure refactor that must produce +byte-identical retrieval; only Stage 2 changes what the model sees. + +## Technical Context + +**Language/Version**: Python 3.12 + +**Primary Dependencies**: `langchain-core` (BaseRetriever, Document), `langchain-chroma`, +`langchain-community` (BM25Retriever), `rank-bm25`, `nltk`. The point of the rewrite +is to depend on `langchain-core` contracts only. + +**Storage**: Chroma collections on disk, from an installed embeddings bundle; +sibling `csv_files/*.csv` for BM25. + +**Testing**: pytest. `tests/retrievers/test_hybrid_retriever.py` already pins the +fusion behaviour, including the tie-break and the `[1/n]*n` weighting that cannot +affect ordering. `bin/retrieval_baseline` for before/after measurement. + +**Target Platform**: Linux server (container), and developer machines. + +**Project Type**: Library within a single application repository. + +**Performance Goals**: Retrieval makes exactly one LLM call per message, down +from 21 (FR-009). No wall-clock target; the LLM answer dominates. + +**Constraints**: Sync and async must agree (FR-007). Behaviour must be measurable +with the committed question set before and after (FR-008). + +**Scale/Scope**: 4 Reactome collections, ~121k documents total; `userguide` has its +own retriever and is out of scope. + +## Constitution Check + +*GATE: must pass before implementation, re-checked after.* + +| Article | How this plan satisfies it | +|---|---| +| I — verify the user path | Every stage ends with the full RAG chain answering a real question through **both** `invoke` and `ainvoke`, not just the retriever in isolation. This is the failure mode that recurred most. | +| II — measure, don't argue | `bin/retrieval_baseline capture` before Stage 1 and after each stage; Stage 1 must show **zero** difference. | +| III — characterization tests pin behaviour | The existing retriever tests must pass unchanged through Stage 1. Any test that must change in Stage 2 changes together with the code and says why. | +| IV — fail loudly | Missing bundle raises with an actionable message rather than passing `None` into a `Path` (FR-006), removing the `B008` suppression and four mypy baseline entries. | +| V — derive from the source of truth | Budget and over-fetch stay single named parameters; the harness keeps importing them from the module rather than re-declaring. | +| VI — bias to doing over filing | Stages are small enough to land individually; nothing here is deferred to an issue that could be done now. | + +**No violations.** The one judgement call is Stage 3 (budget as an argument), which +could be deferred — it is kept because the interface should be settled while the +file is open rather than bolted on later, per spec User Story 2. + +## Project Structure + +### Documentation (this feature) + +```text +specs/001-retriever-rewrite/ +├── spec.md # what and why, with D1–D4 settled +└── plan.md # this file +``` + +No `research.md`, `data-model.md` or `contracts/` are generated. There are no +unresolved unknowns — D1–D4 are decided and the evidence is in the spec — and the +feature introduces no new data model or external contract. Generating empty +scaffolding would be ceremony. + +### Source Code + +```text +src/retrievers/ +├── csv_chroma.py # HybridRetriever -> plain BaseRetriever; owns RRF +├── reactome/rag.py # bundle becomes an explicit argument +├── uniprot/rag.py # same +├── plantreactome/rag.py # same +└── userguide/ # already a plain BaseRetriever; the model to follow + +tests/retrievers/ +└── test_hybrid_retriever.py # extended, not rewritten + +bin/retrieval_baseline # already mirrors the pipeline +``` + +## Implementation Stages + +### Stage 1 — Structural only, zero behaviour change + +Replace the `MultiQueryRetriever` subclass with a `BaseRetriever` that implements +`_get_relevant_documents` / `_aget_relevant_documents`, and vendor Reciprocal Rank +Fusion into this repository (FR-001, FR-002). + +Query expansion stays (D4) and `SelfQueryRetriever` stays for now — this stage +changes *how the code is arranged*, not what it returns. + +RRF is copied from `EnsembleRetriever.weighted_reciprocal_rank` as it behaves +today: `weight / (rank + 60)` with rank counted from 1, de-duplicated on +`page_content`, ties resolved by first appearance. Those properties are already +pinned by tests. + +**Exit criteria**: existing tests pass unchanged; `retrieval_baseline compare` +shows **zero** differences; the full chain answers a real question through both +`invoke` and `ainvoke`. + +### Stage 2 — D1: plain semantic search replaces SelfQuery + +Swap `SelfQueryRetriever` for `vectordb.as_retriever(...)` (FR-009). Retrieval +drops to one LLM call per message. + +`metadata_info.py` stays: `evaluator.py` and `bin/retrieval_baseline` still +construct SelfQuery for comparison, so deleting it would break the tool that +measures this change. + +**Exit criteria**: `retrieval_baseline compare` shows differences confined to the +vector side; a test asserts retrieval makes exactly one LLM call; the chain +answers real questions in both paths. + +### Stage 3 — D2/D3: the caller supplies the budget + +`MAX_DOCUMENTS_PER_COLLECTION` becomes a constructor argument with today's value +as the default (FR-005). The bundle becomes an explicit argument, removing the +`B008` suppression and the four `retrievers.*.rag` mypy baseline entries (FR-006, +SC-006). + +**Exit criteria**: two callers with different budgets in one process get different +amounts of context; the four baseline entries are deleted; gates green. + +## Complexity Tracking + +| Decision | Simpler alternative rejected | Why | +|---|---|---| +| Vendor RRF (~15 lines) | Keep calling `EnsembleRetriever.weighted_reciprocal_rank` | The method is public, but reaching it requires `EnsembleRetriever(retrievers=[])` — an empty retriever built solely to borrow a method. Ranking is the product's core quality; a library upgrade should not be able to reorder results silently. | +| Three stages | One commit | A behaviour change and a structural change in the same diff cannot be attributed when results move. Same reason the rewrite precedes the LangChain upgrade. | +| Keep `metadata_info.py` | Delete 339 dead lines | It is not dead: the evaluator and the baseline harness still construct SelfQuery. Deleting it would break the tool that measures Stage 2. | +| Keep multi-query expansion | Remove it too | D4. Removing two things at once makes any quality change unattributable. Measured separately afterwards. | + +## Out of Scope + +- The LangChain upgrade itself. This unblocks it; it does not perform it. +- Answer quality evaluation (ragas). This plan changes what reaches the model; + whether answers improve is measured separately. +- `userguide` retriever — already a plain `BaseRetriever`. +- Token-based budgeting (D3 caveat) and removing expansion (D4 follow-up).