diff --git a/specs/009-collection-routing/research.md b/specs/009-collection-routing/research.md index bcb8bae..51f9647 100644 --- a/specs/009-collection-routing/research.md +++ b/specs/009-collection-routing/research.md @@ -293,3 +293,85 @@ is not in the bundle is widened to all by `resolve_collections` with a WARNING, which is the failure direction this feature requires. Constraining the schema would instead make structured output reject the response, turning a harmless mistake into a failed answer. + +## T005: `complexes` cannot be guarded by asking a question either + +Attempted properly on 2026-09-19 rather than left as "no candidate found". It +failed, and the reason is structural and worth writing down, because it is the +same reason `reactions` failed and it was not obvious the second time. + +**Method.** Nine questions over six complexes, in two shapes -- six asking +which proteins make up a named complex, three naming components and asking +which complex holds them. Removing `complexes` from what `resolve_collections` +sees is an +exact simulation of a bundle without it -- the name then widens to all the +others, as it would there. This replaced copying a 3.3G bundle. + +**A false start worth recording.** The first attempt set the +`selected_collections` ContextVar around the call. That no longer works now the +classifier is wired: `generate_answer` sets it from the classifier's choice and +overwrites anything set outside, so *both arms searched only `complexes`* and +the result was void. It was caught by instrumenting what retrieval actually +searched, which is the second time in one day that check has saved a +measurement. + +**Composition questions are too variable to build a guard on.** Asking which +proteins make up a complex requires naming four to seven things, and the model +names a different subset each run. The same configuration, three runs: + +| question | complexes only | complexes+summations | all five | +|---|---|---|---| +| Nup107 components (of 4) | 2, 1, 1 | 2, 1, 2 | 2, 1, 1 | +| U7 snRNP subunits (of 7) | 2, 1, 1 | 2, 2, 2 | 6, 2, 2 | + +An earlier four-run pass appeared to show `complexes` making the Nup107 answer +*worse* -- 1,1,1,1 against 3,3,3,3 without it. It did not survive being run +again. That claim was under-powered and is withdrawn; the variance is larger +than any difference between the arms, which is exactly the trap +[[probabilistic-bugs-need-sized-tests]] describes. + +Every guard that works is a **single stable token**: `P04637`, `Tangier`, +`lysosom`. So the question was inverted to ask for one -- name the components, +ask which complex holds them: + +| question | with `complexes` | without | +|---|---|---| +| Which complex contains CYBA and CYBB? | 0/4 | 0/4 | +| Which complex has NUP133, NUP160, NUP37? | 4/4 | **4/4** | +| Which complex is made of LSM10, LSM11, SNRPB? | 3/4 | **4/4** | + +**Answered just as well without it** -- the CYBA/CYBB row proves nothing +either way, since that question fails in both arms and is simply a bad +question; the finding rests on the two that answer. Complex *names* appear +throughout +`reactions`, as the names of inputs and outputs, and throughout `summations` +prose. So any question naming or seeking a complex is answerable from those, +and the only thing structurally unique to `complexes` -- the component list -- +is the thing whose answers are too variable to assert on. + +### What this changes + +`complexes` joins `reactions`: a collection whose content is duplicated +elsewhere cannot be guarded by asking a question, however the question is +worded. T005 should become what T007 already is -- a retrieval-level assertion +that the collection was searched -- rather than a hunt for a better candidate, +which is now two failed hunts and a structural explanation of why. + +### And the retrieval-level assertion does not close the original hole + +Worth being exact, because it would be easy to mark T005 done and believe the +risk went with it. The parametrised test catches a **plumbing** failure: a +collection that cannot be reached at all, through a name mismatch or a lookup +that silently yields nothing. The failure that motivated T005 is different -- +**a classifier that simply never chooses `complexes`** -- and no deterministic +test can catch that, because it is one model call's judgement. + +Covering it properly needs the routing distribution watched over real traffic, +or a periodic probe that asks known-composition questions and checks the +collection was selected. Neither exists. So the residual risk after T005a is: +routing can under-serve `complexes` indefinitely, every tracked question still +passes, and the only symptom is answers that are quietly worse. + +That is smaller than it was -- the plumbing is now pinned, and the nine +questions above found no case where `complexes` was needed for a correct +answer at all -- but it is not nothing, and it is not what T005a tests. diff --git a/specs/009-collection-routing/tasks.md b/specs/009-collection-routing/tasks.md index 5fdba68..c502b1a 100644 --- a/specs/009-collection-routing/tasks.md +++ b/specs/009-collection-routing/tasks.md @@ -19,9 +19,10 @@ routing stops searching them. Routing must not land before this closes, or the acceptance criterion cannot detect the failure the feature can cause. - [x] T004 [P] Add a `summations`-dependent question to `EXPECTATIONS` in src/evaluation/answer_sweep.py (Selective autophagy / lysosome; verified by removal) -- [ ] T005 [P] Find a `complexes` candidate that fails without the collection (the first attempt did not), then add the question to `EXPECTATIONS` in src/evaluation/answer_sweep.py +- [x] T005 [P] ~~Find a `complexes` candidate that fails without the collection~~ **Attempted and abandoned with a reason, 2026-09-19.** Nine questions over six complexes, in two shapes; every one answered just as well without the collection, because complex names appear throughout `reactions` (as input and output names) and `summations` prose. The one thing unique to `complexes` -- the component list -- has answers too variable to assert on: the same configuration returned 1 to 6 of 7 components. See research.md +- [x] T005a [P] Assert at retrieval level that `complexes` was searched, in tests/retrievers/test_sync_async_equivalence.py -- parametrised over all five real collection names: each is reachable, and selecting it searches nothing else. Verified by sabotaging `resolve_collections` to ignore the selection, which fails all five. **Catches a plumbing failure, not a routing one**: a classifier that never chooses a collection is still undetectable, and needs a production probe rather than a test (research.md) - [x] T006 [P] Add an `ewas`-dependent question to `EXPECTATIONS` in src/evaluation/answer_sweep.py (TP53 UniProt P04637; verified by removal) -- [ ] T007 Assert at retrieval level that `reactions` was searched, in tests/retrievers/test_collection_selection.py -- no answer-level question can guard it, because every reaction name also appears in `summations` +- [x] T007 Assert at retrieval level that `reactions` was searched -- closed by the same parametrised test as T005a, in tests/retrievers/test_sync_async_equivalence.py. No answer-level question can guard it, because every reaction name also appears in `summations` - [x] T008 Verify each new question FAILS when its collection is removed from the bundle copy, and passes with it present; record the evidence in the PR (method established; two of four candidates survived it) - [x] T009 Pin current behaviour: a characterization test in tests/retrievers/test_collection_selection.py asserting that with no selection every collection in the bundle is searched - [ ] T010 Run `./bin/answer-sweep` against Release97 and confirm green before any behaviour change diff --git a/tests/retrievers/test_sync_async_equivalence.py b/tests/retrievers/test_sync_async_equivalence.py index c0b21fa..69664be 100644 --- a/tests/retrievers/test_sync_async_equivalence.py +++ b/tests/retrievers/test_sync_async_equivalence.py @@ -87,10 +87,14 @@ def _require_bm25_tokenizer() -> None: ] -def _bundle(tmp_path: Path, embedding: DeterministicFakeEmbedding) -> Path: +def _bundle( + tmp_path: Path, + embedding: DeterministicFakeEmbedding, + collections: dict[str, int] | None = None, +) -> Path: csv_dir = tmp_path / "csv_files" csv_dir.mkdir(parents=True, exist_ok=True) - for collection, count in COLLECTIONS.items(): + for collection, count in (collections or COLLECTIONS).items(): with open(csv_dir / f"{collection}.csv", "w", newline="") as handle: writer = csv.DictWriter( handle, fieldnames=["st_id", "display_name", "text"] @@ -206,3 +210,56 @@ def test_both_paths_honour_the_same_collection_selection(tmp_path: Path) -> None for documents in (sync, asynchronous): assert documents, "the selection removed everything, so this proves nothing" assert all("beta" not in d.page_content for d in documents) + + +REAL_NAMES = ("complexes", "disease_variants", "ewas", "reactions", "summations") + + +@pytest.mark.requires_retrieval_stack +@pytest.mark.parametrize("wanted", REAL_NAMES) +def test_every_collection_can_be_reached_and_only_it( + tmp_path: Path, wanted: str +) -> None: + """T005a and T007: assert at retrieval level that a collection was searched. + + Neither `complexes` nor `reactions` can be guarded by asking a question. + Measured 2026-09-19 (specs/009-collection-routing/research.md): their + content is duplicated in `summations` prose and in the input/output names + carried by `reactions`, so every candidate answered just as well with the + collection removed. Two failed hunts and a structural explanation. + + So the guard lives here instead, and it is worth being exact about what it + replaces. It catches a *plumbing* failure: a collection that cannot be + reached at all, through a name mismatch or a lookup that silently yields + nothing. It does **not** catch the failure that motivated T005 -- a + classifier that simply never chooses a collection. Nothing deterministic + can, because that is one model call's judgement; it would need the routing + distribution watched in production, or a periodic probe. The residual risk + is recorded in research.md rather than claimed as covered. + """ + _require_bm25_tokenizer() + embedding = DeterministicFakeEmbedding(size=16) + retriever = HybridRetriever.from_subdirectory( + llm=FakeListChatModel(responses=[""]), + embedding=embedding, + embeddings_directory=_bundle( + tmp_path, embedding, {name: 20 for name in REAL_NAMES} + ), + ) + + token = selected_collections.set([wanted]) + try: + documents = retriever.retrieve_documents( + ["kinase phosphorylation"], + CallbackManagerForRetrieverRun.get_noop_manager(), + ) + finally: + selected_collections.reset(token) + + assert documents, f"{wanted} is selectable but returned nothing" + for other in REAL_NAMES: + if other == wanted: + continue + assert not any( + other in d.page_content for d in documents + ), f"selecting {wanted} also searched {other}"