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