From 622889c0c4e0bbedff5d9a8c89b526a3639e9d8e Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 02:18:54 +0000 Subject: [PATCH] Measure query expansion; the cost is the call, not the variants T020 said "reduce query expansion from 5 variants". The premise was wrong. Measured over six tracked questions: four alternates cost 1.27s to generate and 1.22s to retrieve for; two cost 1.26s and 0.65s; zero cost nothing and 0.31s. The expansion call costs about 1.27s whatever it returns, so trimming the count saves only fan-out -- roughly 0.57s of a 2.49s stage. The cost disappears only by not making the call. With expansion off, `answer-sweep` passed 13/13 in 79s against about 150s. That is the largest single latency lever found so far and it bears on T022. The count is now `QUERY_EXPANSION_ALTERNATES`, enforced in code rather than asked for in the prompt, and **the default is unchanged at 4**. Thirteen questions establish that those answers do not need expansion; they do not establish that recall is unaffected in general, and expansion exists for the questions nobody wrote a test for. A switch and a measurement, not a verdict. At zero the call is skipped rather than made and discarded -- it is the larger half of the cost, and making it anyway would keep the expense while losing the benefit, while looking identical in every other measurement. A bad value falls back to the default loudly, because a typo silently disabling a recall mechanism would look like nothing at all downstream. How it was measured matters here. The first attempt varied the count by editing the prompt to ask for "exactly N". The model obeyed at 2 and ignored 1 and 0, producing four either way, so those rows re-measured the baseline -- caught only because the number of queries actually asked was recorded next to the timings. That is also why the implementation truncates rather than asks. Co-Authored-By: Claude Opus 5 --- specs/010-search-page-answers/research.md | 43 +++++++++ specs/010-search-page-answers/tasks.md | 2 +- src/retrievers/csv_chroma.py | 81 +++++++++++++++-- tests/retrievers/test_query_expansion.py | 101 ++++++++++++++++++++++ 4 files changed, 220 insertions(+), 7 deletions(-) create mode 100644 tests/retrievers/test_query_expansion.py diff --git a/specs/010-search-page-answers/research.md b/specs/010-search-page-answers/research.md index 9158a02..25b1802 100644 --- a/specs/010-search-page-answers/research.md +++ b/specs/010-search-page-answers/research.md @@ -126,3 +126,46 @@ An endpoint that answers correctly and slowly, streaming, with verification. Not against nothing, and is otherwise idle on this. p50 15.2s is too slow to ship to users and entirely good enough to integrate against. The latency work is real and separate, and spec 009 plus query-expansion reduction are its two known levers. + +## Query expansion, measured 2026-09-20 (T020) + +The task said "reduce query expansion from 5 variants". The premise was +wrong, and the measurement is the useful part. + +| alternates | expansion call | retrieval | total | documents kept | +|---|---|---|---|---| +| 4 (default) | 1.27s | 1.22s | **2.49s** | baseline | +| 2 | 1.26s | 0.65s | 1.92s | 84% | +| 1 | 1.38s | 0.57s | 1.95s | 77% | +| 0 | 0.00s | 0.31s | **0.31s** | 71% | + +**The expansion call costs about 1.27s whatever it returns.** Trimming four +variants to two saves fan-out only -- about 0.57s of a 2.49s stage. The whole +cost goes away only by not making the call, which is a different change from +the one the task described. + +With expansion off, `answer-sweep` passed **13/13 in 79s**, against roughly +150s with it on. That is the largest single latency lever found so far, and +it bears on T022. + +### Two things about how this was measured + +The first attempt varied the count by editing the prompt to ask for "exactly +N". The model obeyed at 2 and **ignored 1 and 0, producing four either way**, +so those rows silently re-measured the baseline -- visible only because the +number of queries actually asked was recorded alongside the timings. The +count is now enforced in code, which removes the model's obedience from the +experiment and is also how the feature is implemented. + +Document overlap is not quality. 71% of baseline documents at zero alternates +says three-quarters of the retrieved set is unchanged; it says nothing about +whether the quarter that changed mattered. + +### Why the default is unchanged + +Thirteen tracked questions establish that *those* answers do not need +expansion. They do not establish that recall is unaffected in general, and +expansion exists for the questions nobody wrote a test for. So this ships as +`QUERY_EXPANSION_ALTERNATES` with the default at 4 -- a switch and a +measurement, not a verdict. Trying 0 on beta, where the sweep and the routing +probe both run on every deploy, is the cheap way to learn more. diff --git a/specs/010-search-page-answers/tasks.md b/specs/010-search-page-answers/tasks.md index fadbfda..463250c 100644 --- a/specs/010-search-page-answers/tasks.md +++ b/specs/010-search-page-answers/tasks.md @@ -58,7 +58,7 @@ state. - [x] T019 Measure first-token and completion separately across the tracked questions; publish the distribution, not one question. Measured 2026-09-18, two runs each: first token p50 9.6s / p90 12.2s (n=26), completion p50 10.4s / p90 18.1s (n=30). The earlier "36.1s to first token" came from one question and does not reproduce (PR #238) - [x] T019a Run preprocessing in two rounds instead of four sequential calls in src/agent/profiles/react_to_me.py; the base class already overlapped, and this override discarded it (PR #238) -- [ ] T020 Reduce query expansion from 5 variants, measuring recall with bin/retrieval_baseline — its own call plus a 5x retrieval fan-out +- [x] T020 Measure query expansion, and make the count configurable. **The task's premise was wrong**: the cost is the expansion *call*, not the number of variants it returns. Measured over six tracked questions -- 4 alternates 1.27s expand + 1.22s retrieve; 2 alternates 1.26s + 0.65s; 0 alternates 0.00s + 0.31s. Trimming the count saves only fan-out; the cost disappears only at zero, where `answer-sweep` was 13/13 in 79s against ~150s. `QUERY_EXPANSION_ALTERNATES` sets it and **the default is unchanged**: thirteen questions show those answers do not need expansion, not that recall is unaffected in general - [ ] T020c Establish whether a search-page question needs all four preprocessing calls. The sequential half of this is answered and done (T019a): they run in two rounds and cost 2.6s at the median, not the ~16s recorded here, which never reproduced - [ ] T021 Land spec 009 collection routing and re-measure. **Still open — I marked this done on 2026-09-18 and was wrong.** What landed is *source* routing (`resolve_active_sources` picks reactome / userguide / live). Collection routing is selecting among the five collections *within* the reactome bundle, and it is not implemented: `QueryIntent` has no `collections` field, `resolve_collections` does not exist, and `retrieve_documents` still loops over every collection - [ ] T022 Re-assess FR-005 against the result and say plainly whether 2s/10s is reachable diff --git a/src/retrievers/csv_chroma.py b/src/retrievers/csv_chroma.py index 8966ce5..f108b2d 100644 --- a/src/retrievers/csv_chroma.py +++ b/src/retrievers/csv_chroma.py @@ -1,5 +1,6 @@ import asyncio import csv +import os from collections.abc import Coroutine, Iterable from contextvars import ContextVar from pathlib import Path @@ -54,6 +55,52 @@ def chroma_settings() -> chromadb.config.Settings: return chromadb.config.Settings(anonymized_telemetry=False) +ALTERNATES_ENV = "QUERY_EXPANSION_ALTERNATES" +DEFAULT_ALTERNATES = 4 + + +def expansion_alternates() -> int: + """How many alternate questions to retrieve for, besides the original. + + Measured 2026-09-20 over six tracked questions (spec 010, T020): + + | alternates | expand | retrieve | total | documents kept | + |---|---|---|---|---| + | 4 (default) | 1.27s | 1.22s | 2.49s | baseline | + | 2 | 1.26s | 0.65s | 1.92s | 84% | + | 1 | 1.38s | 0.57s | 1.95s | 77% | + | 0 | 0.00s | 0.31s | 0.31s | 71% | + + **The expansion call costs about 1.27s whatever it returns**, so trimming + the count saves only fan-out; the cost goes away only at zero. With zero, + `answer-sweep` was 13/13 and ran in 79s against about 150s. + + The default is unchanged regardless. Thirteen questions establish that the + answers we track do not need expansion; they do not establish that recall + is unaffected in general, and expansion exists for the questions nobody + wrote a test for. This is a switch and a measurement, not a verdict. + + An unparseable or negative value falls back to the default loudly, rather + than silently disabling a recall mechanism. + """ + raw = os.getenv(ALTERNATES_ENV, "").strip() + if not raw: + return DEFAULT_ALTERNATES + try: + value = int(raw) + except ValueError: + logger.warning( + "%s=%r is not a number; using %d", ALTERNATES_ENV, raw, DEFAULT_ALTERNATES + ) + return DEFAULT_ALTERNATES + if value < 0: + logger.warning( + "%s=%d is negative; using %d", ALTERNATES_ENV, value, DEFAULT_ALTERNATES + ) + return DEFAULT_ALTERNATES + return value + + multi_query_prompt = PromptTemplate( input_variables=["question"], template="""You are a biomedical question expansion engine for information retrieval over the Reactome biological pathway database. @@ -418,9 +465,12 @@ def _get_relevant_documents( is appended AFTER the generated ones: RRF breaks ties by first appearance, so reordering here would silently change the ranking. """ - queries = self.query_expander.invoke( - {"question": query}, config={"callbacks": run_manager.get_child()} - ) + wanted = expansion_alternates() + queries: list[str] = [] + if wanted: + queries = self.query_expander.invoke( + {"question": query}, config={"callbacks": run_manager.get_child()} + )[:wanted] if self.include_original: queries.append(query) return unique_documents(self.retrieve_documents(queries, run_manager)) @@ -429,12 +479,31 @@ async def _aget_relevant_documents( self, query: str, *, run_manager: AsyncCallbackManagerForRetrieverRun ) -> list[Document]: """Async twin of the above; must agree with it document for document.""" - queries = await self.query_expander.ainvoke( - {"question": query}, config={"callbacks": run_manager.get_child()} + queries = await self._expand( + query, + lambda: self.query_expander.ainvoke( + {"question": query}, config={"callbacks": run_manager.get_child()} + ), ) + return unique_documents(await self.aretrieve_documents(queries, run_manager)) + + async def _expand(self, query: str, call: Any) -> list[str]: + """The queries to retrieve for, honouring the configured count. + + At zero the expansion call is skipped entirely rather than made and + discarded -- that call is the larger half of the cost, and making it + anyway would keep the expense while losing the benefit. + + The original is appended LAST, as it always was: RRF breaks ties by + first appearance, so reordering silently changes the ranking. + """ + wanted = expansion_alternates() + queries: list[str] = [] + if wanted: + queries = (await call())[:wanted] if self.include_original: queries.append(query) - return unique_documents(await self.aretrieve_documents(queries, run_manager)) + return queries def weighted_reciprocal_rank( self, doc_lists: list[list[Document]] diff --git a/tests/retrievers/test_query_expansion.py b/tests/retrievers/test_query_expansion.py new file mode 100644 index 0000000..6ee0801 --- /dev/null +++ b/tests/retrievers/test_query_expansion.py @@ -0,0 +1,101 @@ +"""How many alternate questions retrieval asks for. + +Measured, not argued (spec 010 T020): the expansion call costs about 1.27s +whatever it returns, so trimming the count saves only fan-out and the cost +disappears only at zero. The default is unchanged regardless -- thirteen +tracked questions show those answers do not need expansion, not that recall +is unaffected in general. +""" + +import asyncio + +import pytest + +from retrievers.csv_chroma import ( + ALTERNATES_ENV, + DEFAULT_ALTERNATES, + HybridRetriever, + expansion_alternates, +) + + +def test_the_default_is_unchanged_without_configuration( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv(ALTERNATES_ENV, raising=False) + assert expansion_alternates() == DEFAULT_ALTERNATES == 4 + + +@pytest.mark.parametrize(("raw", "expected"), [("0", 0), ("2", 2), ("7", 7)]) +def test_a_configured_count_is_honoured( + monkeypatch: pytest.MonkeyPatch, raw: str, expected: int +) -> None: + monkeypatch.setenv(ALTERNATES_ENV, raw) + assert expansion_alternates() == expected + + +@pytest.mark.parametrize("raw", ["", " ", "lots", "-1", "3.5"]) +def test_a_bad_value_falls_back_loudly_rather_than_disabling_recall( + monkeypatch: pytest.MonkeyPatch, raw: str, caplog: pytest.LogCaptureFixture +) -> None: + # The dangerous direction: a typo must not silently turn expansion off, + # because nothing downstream would look any different. + monkeypatch.setenv(ALTERNATES_ENV, raw) + assert expansion_alternates() == DEFAULT_ALTERNATES + + +class _Expander: + def __init__(self) -> None: + self.calls = 0 + + async def ainvoke(self, _inputs: object, config: object = None) -> list[str]: + self.calls += 1 + return [f"alt {i}" for i in range(4)] + + +def _expand( + count: str | None, monkeypatch: pytest.MonkeyPatch +) -> tuple[list[str], int]: + if count is None: + monkeypatch.delenv(ALTERNATES_ENV, raising=False) + else: + monkeypatch.setenv(ALTERNATES_ENV, count) + retriever = HybridRetriever.__new__(HybridRetriever) + object.__setattr__(retriever, "include_original", True) + expander = _Expander() + queries = asyncio.run( + retriever._expand("the question", lambda: expander.ainvoke(None)) + ) + return queries, expander.calls + + +def test_the_original_question_is_always_asked_and_always_last( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # RRF breaks ties by first appearance, so the position is ranking, not + # cosmetics. + for count in (None, "2", "0"): + queries, _ = _expand(count, monkeypatch) + assert queries[-1] == "the question" + + +def test_at_zero_the_expansion_call_is_skipped_not_made_and_discarded( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The call is the larger half of the cost. Making it and throwing the + # result away would keep the expense and lose the benefit -- and would + # look identical in every other measurement. + queries, calls = _expand("0", monkeypatch) + assert queries == ["the question"] + assert calls == 0 + + +def test_a_lower_count_truncates_rather_than_trusting_the_prompt( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # Measured: asked for "exactly 1" and "exactly 0" alternates, the model + # produced four anyway. Enforcing the count in code removes the model's + # obedience from the question. + queries, calls = _expand("2", monkeypatch) + assert queries == ["alt 0", "alt 1", "the question"] + assert calls == 1