From 65ad279c298777e18a2c8e2caab5bbd4d776c86d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 9 Sep 2026 14:49:10 +0000 Subject: [PATCH] Spec 002: choosing the default answering model; close out spec 001 Two pieces of design tracking that were owed. Spec 002 records the gpt-5.6-luna decision. #186 made the model usable; this is about whether it becomes the default, which is not a dependency bump: it changes what every user reads, trades determinism for capability, doubles latency, moves cost in an unmeasured direction, and will be asked again for chat-alongside-search and analysis summarisation. None of that is recoverable from the diff. It carries the measurements taken so far and is explicit about which of them are too weak to decide on -- the answer-quality observation is three answers with no rubric, and the determinism check is four inputs, which cannot rule out the rare flip it is meant to rule out. Price per token could not be read from any endpoint, so the premise that started this ("just as cheap") is still unverified. It also names the blocker behind the blocker: evaluator.py runs the right ragas metrics but builds its own SelfQueryRetriever + EnsembleRetriever + MergerRetriever rather than the shipping pipeline. Stage 2 removed SelfQuery, so it now measures a configuration that does not exist, and running it would produce numbers that look like an answer and are not. Fixing that is the spec's P1 because it is owed anyway: the four retrieval changes from 2026-09-04 went in unevaluated. Three decisions are left to the team rather than assumed -- flip before or after evaluating, whether 2x latency is acceptable on chat, and what luna actually costs. Spec 001 is marked complete with its outcome, including the two predictions in its plan that were wrong: "Stage 1 must show zero difference" was unfalsifiable against Chroma's ANN noise floor, which already differed on 2 of 8 question-collections between identical runs; and "Stage 2 becomes deterministic" did not happen, because removing the LLM from the vector side removed only one of the two sources of variance. --- specs/001-retriever-rewrite/plan.md | 4 + specs/001-retriever-rewrite/spec.md | 51 +++- .../checklists/requirements.md | 50 ++++ specs/002-default-llm-choice/spec.md | 275 ++++++++++++++++++ 4 files changed, 379 insertions(+), 1 deletion(-) create mode 100644 specs/002-default-llm-choice/checklists/requirements.md create mode 100644 specs/002-default-llm-choice/spec.md diff --git a/specs/001-retriever-rewrite/plan.md b/specs/001-retriever-rewrite/plan.md index f183ea34..c695ca3d 100644 --- a/specs/001-retriever-rewrite/plan.md +++ b/specs/001-retriever-rewrite/plan.md @@ -2,6 +2,10 @@ **Branch**: `plan/retriever-rewrite` | **Date**: 2026-09-08 | **Spec**: [spec.md](./spec.md) +**Status**: Executed. See [Outcome](./spec.md#outcome-2026-09-09) in the spec — +two of the exit criteria below turned out to be unfalsifiable or wrong, and +which ones is recorded there. + **Input**: Feature specification from `/specs/001-retriever-rewrite/spec.md` ## Summary diff --git a/specs/001-retriever-rewrite/spec.md b/specs/001-retriever-rewrite/spec.md index 0bba52f9..2dc222b7 100644 --- a/specs/001-retriever-rewrite/spec.md +++ b/specs/001-retriever-rewrite/spec.md @@ -4,7 +4,7 @@ **Created**: 2026-09-08 -**Status**: Decisions settled — D1 by Helia, D2–D4 recorded below. Ready for `/speckit-plan`. +**Status**: **Complete.** All three stages shipped 2026-09-09 (#182, #183, #185). Outcome recorded at the end of this file, including two predictions that were wrong. **Input**: Rewrite the Reactome retriever: replace the `HybridRetriever` that subclasses `MultiQueryRetriever` with a plain `BaseRetriever`, decide whether `SelfQueryRetriever` is replaced by plain semantic search, and make the context budget caller-supplied. @@ -230,3 +230,52 @@ calls. - BM25 stays. Helia's proposal keeps it, it is deterministic, and it costs no LLM call. - Answer quality is judged by the ragas work, not by this spec. This spec changes *what reaches the model*; whether that improves answers is measured separately. - The rewrite happens before the LangChain upgrade, so that a behaviour change and an upgrade change cannot be confused for one another. + + +--- + +## Outcome (2026-09-09) + +All three stages shipped. `HybridRetriever` is a plain `BaseRetriever`; +`SelfQueryRetriever` is gone from the pipeline; the bundle and the context budget +are constructor arguments. Retrieval makes **one LLM call per message, down from +21**. The five reaches into LangChain internals are zero, so the upgrade is +unblocked — which was the point. + +Suppressions deleted rather than annotated: four `retrievers.*.rag` mypy baseline +entries and four `B008` noqa comments. + +| stage | PR | what landed | +|---|---|---| +| 1 | #182 | plain `BaseRetriever`, RRF vendored, byte-identical results | +| 2 | #183 | plain semantic search replaces SelfQuery | +| 3 | #185 | `require_dir` for the bundle, budget as a constructor argument | + +### Two predictions in the plan were wrong + +Recorded because the point of this file is that the next person does not repeat +them. + +1. **"Stage 1 must show *zero* difference"** was unfalsifiable as written. Chroma's + ANN search has a noise floor — two identical runs already differed on 2 of 8 + question-collections — so "zero" could never have been observed, and a real + regression of that size would have been invisible against it. An exit criterion + has to be stated against measured noise, not against an ideal. + +2. **"Stage 2 becomes deterministic"** did not happen. Removing the LLM from the + vector side removed *that* source of variance; the ANN variance underneath it + stayed. Removing one of two causes does not make an effect go away, and the plan + asserted it would without checking which cause dominated. + +### One thing this rewrite did not do + +It changed what reaches the model four times over — over-fetch, de-duplication, +genuine BM25/vector fusion, and plain semantic search — and **none of those changes +has been evaluated for answer quality**. The plan said so under Out of Scope, which +made it a known gap rather than an oversight, but it is still a gap. + +`src/evaluation/evaluator.py` is the tool for it and cannot be used as it stands: it +builds its own `SelfQueryRetriever` + `EnsembleRetriever` + `MergerRetriever` rather +than the shipping pipeline, so after Stage 2 it measures a configuration that no +longer exists. Carried into [spec 002](../002-default-llm-choice/spec.md) as its P1, +where the same fix serves both. diff --git a/specs/002-default-llm-choice/checklists/requirements.md b/specs/002-default-llm-choice/checklists/requirements.md new file mode 100644 index 00000000..af972e58 --- /dev/null +++ b/specs/002-default-llm-choice/checklists/requirements.md @@ -0,0 +1,50 @@ +# Specification Quality Checklist: Choosing the Default Answering Model + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-09-09 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +Three deviations from the template, each deliberate: + +1. **Model and file names appear in the spec.** `gpt-5.6-luna`, `evaluator.py`, + `resolve_temperature`. The subject of this decision *is* a named third-party + model and a named drifted file; writing around them would make the document + unusable for the three people who have to act on it. + +2. **The evidence section leads.** The template puts user stories first. Here the + measurements are the substance and the stories follow from them — and two of + the measurements are explicitly labelled as too weak to decide on, which is the + most important thing on the page. + +3. **No [NEEDS CLARIFICATION] markers, but three open questions.** They are not + gaps in the specification — the spec is complete and actionable as written. They + are decisions reserved to the team, recorded in "Decisions for the team" so they + are answered once and in the open rather than assumed. The P1 work (FR-001..004) + proceeds regardless of how they are answered. diff --git a/specs/002-default-llm-choice/spec.md b/specs/002-default-llm-choice/spec.md new file mode 100644 index 00000000..e50c2bfc --- /dev/null +++ b/specs/002-default-llm-choice/spec.md @@ -0,0 +1,275 @@ +# Feature Specification: Choosing the Default Answering Model + +**Feature Branch**: `spec/default-llm-choice` + +**Created**: 2026-09-09 + +**Status**: Evidence gathered, decision open. Three questions for the team below. + +**Input**: `gpt-4o-mini` is today's default. `gpt-5.6-luna` is now usable (#186). Does it become the default, on which surfaces, and what evidence settles it? + +## Why this is a specification and not a dependency bump + +The constitution reserves Spec Kit for "design work with real, unmade decisions" and +excludes "dependency bumps, where the ceremony costs more than the fix". Changing the +answering model looks like a bump and is not one: + +- it changes **what every user reads**, on every surface, immediately +- it trades **determinism** for capability — an unavoidable consequence, not a setting +- it changes **latency** by roughly a factor of two on a chat interface +- it changes **cost**, in a direction nobody has yet measured +- and the same choice will be made again for chat-alongside-search and analysis + summarisation, which have different tolerances + +None of that is recoverable from the diff. That is what this file is for. + +## What has already been settled + +`gpt-5.6-luna` **works end to end**. #186 removed the two things that stopped it: + +| blocker | resolution | +|---|---| +| `temperature=0.0` hardcoded; the gpt-5.5/5.6/6 families accept only their default of 1 | `resolve_temperature()` per model family, `LLM_TEMPERATURE` to override | +| the three graders used `function_calling`, which gpt-5.6 refuses on `/v1/chat/completions` | `method="json_schema"`, verified on **both** models so the path cannot rot | + +Switching is now one environment variable: `LLM_MODEL=gpt-5.6-luna`. **This +specification is about whether to set it, not how.** + +## Evidence gathered + +Measured through `AgentGraph.ainvoke` on the React-to-Me profile — the entry point +`bin/chat-chainlit.py` uses — against the Release95 reactome bundle. + +### Latency and shape (3 questions, averaged) + +| | seconds/question | LLM calls/question | input tokens | +|---|---|---|---| +| gpt-4o-mini | 22.5 | 6 | ~2685 | +| gpt-5.6-luna | 41.2 | 6 | ~3238 | + +Output tokens are omitted deliberately: the OpenAI callback undercounts streamed +completions, and inconsistently between the two models, so the numbers it gave +would have looked authoritative and been wrong. + +### Determinism (10 runs each) + +`temperature=1` is not a preference; it is the only value these models accept. The +concern is the graders, which gate the whole conversation. + +| input | gpt-4o-mini @ 0.0 | gpt-5.6-luna @ 1.0 | +|---|---|---| +| science question → intent | `reactome` ×10 | `reactome` ×10 | +| how-to question → intent | `userguide` ×10 | `userguide` ×10 | +| benign text → safety | `true` ×10 | `true` ×10 | +| prompt injection → safety | `false` ×10 | `false` ×10 | + +Stable on four inputs. That is evidence, not a guarantee — four inputs is a smoke +test, and the failure mode being ruled out is a rare flip, which is precisely what +a small sample cannot rule out. + +### Answer character (qualitative, n=3, no rubric) + +On *"Which complexes contain EGFR?"* luna named four specific complexes with +Reactome links; gpt-4o-mini gave a general description of EGFR signalling naming +none. Luna's answers cite the retrieved records ("In the supplied Reactome +records...", "Reactome describes..."); gpt-4o-mini's read as recalled background. + +This is the observation that makes the decision interesting, and it is the one with +the weakest evidence behind it. **A read of three answers is an anecdote.** + +### What could not be measured + +- **Price per token.** No endpoint reports it. The premise "just as cheap" is + unverified here and has to come from the pricing page or from a billing period. +- **Answer quality at any scale.** See below — the harness that would do it does + not currently measure the pipeline that ships. + +## The blocker behind the blocker + +`src/evaluation/evaluator.py` runs ragas with `faithfulness`, `answer_relevancy`, +`context_recall` and `context_utilization` over a golden set — exactly the metrics +that would settle the answer-quality question, and `faithfulness` is exactly the +axis on which luna appeared to differ. + +It cannot be used as it stands. It builds its own retriever — `SelfQueryRetriever` ++ `EnsembleRetriever` + `MergerRetriever` — rather than calling +`create_bm25_chroma_ensemble_retriever`. Stage 2 removed `SelfQueryRetriever` from +the shipping pipeline, so the evaluator now measures a configuration that no longer +exists. Running it today would produce numbers that look like an answer and are not. + +This is the same drift `bin/retrieval_baseline` had and had fixed: a measurement +tool that has quietly stopped measuring the thing. + +It also blocks work already owed: the four retrieval changes from the week of +2026-09-04 (#169, #170, over-fetch, D1) went in **unevaluated for answer quality**. +Pointing the evaluator at the real pipeline pays for both. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 — The evaluator measures what ships (Priority: P1) + +A developer changes the model, or the retriever, and runs one command that reports +faithfulness, answer relevancy, context recall and context utilization for the +pipeline a user actually talks to. + +**Why this priority**: Nothing else here can be decided without it, and it is owed +regardless of which model wins. It is the only item on this page whose value does +not depend on the outcome of the decision. + +**Independent Test**: Run the evaluator against `main` twice with no change in +between; the metrics agree within noise. Then swap the retriever's document budget +and watch context utilization move. + +**Acceptance Scenarios**: + +1. **Given** the evaluator, **When** it constructs the chain, **Then** it calls the + same factory `bin/chat-chainlit.py` reaches, so a change to retrieval cannot + alter the product without altering the measurement. +2. **Given** a golden question set, **When** the evaluator runs on two models, + **Then** it emits a per-metric comparison rather than two unrelated reports. +3. **Given** the evaluator, **When** `SelfQueryRetriever` no longer exists in the + pipeline, **Then** the evaluator does not construct one. + +--- + +### User Story 2 — The default model is chosen on evidence (Priority: P2) + +The team picks the default from a table of measured differences, not from a +recollection of three answers. + +**Why this priority**: This is the actual question. It is P2 only because it is +gated on P1. + +**Independent Test**: Both models are run over the golden set; the report shows +faithfulness, relevancy, recall, utilization, latency and token counts side by +side. + +**Acceptance Scenarios**: + +1. **Given** both models evaluated, **When** faithfulness differs by less than the + run-to-run noise, **Then** the difference is reported as "not measurable", not + as a win. +2. **Given** a chosen default, **When** it is committed, **Then** the reason and + the numbers are recorded here, so the next person does not re-derive them. + +--- + +### User Story 3 — Different surfaces may choose differently (Priority: P3) + +Chat, chat-alongside-search-results, and analysis summarisation each pick a model +suited to their tolerance for latency. + +**Why this priority**: Only chat exists today. Recording it now costs a paragraph; +discovering later that the model is a global constant costs a refactor — which is +the same mistake the context budget made, fixed in Stage 3. + +**Acceptance Scenarios**: + +1. **Given** a surface that must answer inside a search-results page, **When** it + builds its chain, **Then** it can request a faster model without changing what + the chat surface uses. + +### Edge Cases + +- **A model is added to a fixed-temperature family that the prefix table has not + met.** It returns a 400 on the first user question, not at startup. `LLM_TEMPERATURE` + is the escape hatch; the table is matched on prefix so dated snapshots are covered. +- **The evaluator's own judge model.** ragas uses an LLM to score. If the judge is + the same model being evaluated, the comparison is biased. The judge must be pinned + and stated, and must not change between the two runs being compared. +- **Latency on a streamed interface.** 41s to a complete answer is not 41s of + silence if tokens stream; the perceived cost depends on time-to-first-token, which + has not been measured and is the number a user actually experiences. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The evaluator MUST build its chain from the same factory the + application uses, so that retrieval changes cannot alter the product without + altering the measurement. +- **FR-002**: The evaluator MUST accept the model under test as an argument and + report metrics per model, so two models can be compared in one run. +- **FR-003**: The evaluator MUST pin and report the judge model separately from the + model under test. +- **FR-004**: The evaluator MUST report run-to-run variance, so a difference smaller + than noise is not read as a result. +- **FR-005**: The default model MUST remain selectable per deployment without a code + change. +- **FR-006**: A model whose temperature requirement is unknown MUST fail with a + message naming the environment variable that fixes it. +- **FR-007**: The chosen default and the numbers behind it MUST be recorded in this + specification when the decision is made. + +### Key Entities + +- **Model under test**: the model that answers, and that the graders use. +- **Judge model**: the model ragas uses to score. Independent of the above, pinned. +- **Golden set**: `tests/golden/questions.txt`, 28 questions, already committed and + already used by `bin/retrieval_baseline`. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: One command reports answer quality for the pipeline a user talks to, + for a named model, against the golden set. +- **SC-002**: Two consecutive runs of that command on unchanged code differ by less + than the threshold the command itself reports as noise. +- **SC-003**: The default-model decision is recorded with a per-metric comparison + covering both candidates. +- **SC-004**: Switching the default requires changing one environment variable, and + reverting it requires changing it back — no rebuild, no code edit. +- **SC-005**: The four retrieval changes from 2026-09-04 have an answer-quality + measurement attached, closing the gap left open by spec 001. + +## Assumptions + +- The golden set is representative enough to compare two models. It was assembled + for retrieval measurement, not answer grading; if it proves too narrow that is a + finding, not a reason to skip the measurement. +- `gpt-4o-mini` remains available. Nothing here is a migration forced by deprecation. +- Beta is the place to observe a model change before production. Both pin an image + tag, so the model can differ between them by environment alone. +- Cost is a real constraint but not the binding one at current volume; latency and + answer quality are what the team will notice first. + +## Decisions for the team + +Not gaps in the specification — the P1 work proceeds however these are answered. +They are recorded so they are answered once, in the open. + +### D1 — Does the default flip before or after the quality evaluation? + +| option | what it means | +|---|---| +| **A. Evaluate first** (recommended) | Fix the evaluator (P1), run both models over the golden set, then decide. Slower; the decision is defensible afterwards. | +| B. Flip beta now, evaluate alongside | Beta users see luna immediately. Real questions are better than golden ones, but nothing is being recorded, so "it seems better" stays an impression. | +| C. Flip both now | Fastest. Trades a measurable 2x latency increase on production for an answer-quality improvement supported by three answers. | + +**Recommendation: A**, and it costs less than it sounds — the evaluator fix is owed +anyway for the four unevaluated retrieval changes (SC-005). B is defensible if the +grounding difference matters more than the latency; C is not, on this evidence. + +### D2 — Is ~2x latency acceptable on the chat surface? + +22.5s → 41.2s per question, measured to the complete answer. Chainlit streams, so +what a user feels is time-to-first-token, which has not been measured. If the answer +is "no", that alone settles the default for chat and D1 becomes moot for that +surface — though luna may still suit analysis summarisation, where nobody is +watching a cursor. + +### D3 — What is the actual price per token? + +The premise for trying luna was "just as cheap". No endpoint reports pricing, so +this could not be checked here. If luna is materially more expensive per token, the +larger answers it produces multiply that, and cost becomes a first-order input +rather than the third one. + +## Out of Scope + +- The LangChain upgrade. Unblocked by spec 001, unrelated to this. +- Fine-tuning, or any change to the prompts. Changing prompt and model together + would make the comparison unattributable — the same reason spec 001 was staged. +- Streaming behaviour and time-to-first-token, beyond noting above that it is the + number a user feels. Worth its own measurement.