From 8db1834e6188b3943bac28ac5288320ef936178b Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 17 Sep 2026 13:41:52 +0000 Subject: [PATCH 1/2] Cover the retrieval path the application actually serves with HybridRetriever implements retrieval twice -- retrieve_documents and aretrieve_documents, the second gathering coroutines so collections are queried concurrently. Nothing exercised the async one, and that is the one the application serves through. bin/retrieval_baseline, the tool the constitution names for measuring retrieval changes, drives the sync one. If they drift, every measurement is of a path no user takes. They do not drift today: same documents, same order, checked across three query shapes. Two things this test had to be argued out of claiming. It first reported the paths returning different document sets -- that was FakeEmbeddings returning a fresh random vector per call, so the same query embedded differently on the second run. DeterministicFakeEmbedding fixes it, and the near-miss is recorded in the docstring because the false result is more instructive than the true one. And it does not cover the per-collection cap. Deterministic fake vectors carry no relation to the text, so every query retrieves much the same set and fusion lands on exactly the cap however many queries are given -- changing the cap on one path alone stays invisible. Tried at 12 and 60 documents per collection with one, three and seven queries; ten per collection every time. That limit is written into the docstring rather than left as apparent coverage, and covering it needs real embeddings, which is retrieval_baseline's job. Co-Authored-By: Claude Opus 5 --- .../retrievers/test_sync_async_equivalence.py | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 tests/retrievers/test_sync_async_equivalence.py diff --git a/tests/retrievers/test_sync_async_equivalence.py b/tests/retrievers/test_sync_async_equivalence.py new file mode 100644 index 00000000..23303c3c --- /dev/null +++ b/tests/retrievers/test_sync_async_equivalence.py @@ -0,0 +1,149 @@ +"""The async retrieval path must return what the sync one does. + +`HybridRetriever` implements retrieval twice: `retrieve_documents` and +`aretrieve_documents`, the second gathering coroutines so the collections are +queried concurrently. They are separate implementations of the same fusion, and +nothing exercised the async one at all. + +That asymmetry matters beyond the usual duplication argument: the application +serves through the async path, while `bin/retrieval_baseline` -- the tool the +constitution names for measuring retrieval changes -- drives the sync one. If +they drift, every measurement is of a path no user takes, and Principle II's +before-and-after comparison measures the wrong thing. + +Uses DeterministicFakeEmbedding rather than FakeEmbeddings: the latter returns a +fresh random vector per call, so the same query embeds differently on the second +run and the two paths appear to disagree when they do not. That false result is +what prompted this test. + +What this does NOT cover, measured rather than assumed: the per-collection cap. +Deterministic fake vectors carry no relation to the text, so every query retrieves +much the same set, fusion lands on exactly `max_documents_per_collection` however +many queries are given, and changing the cap on one path alone stays invisible. +Tried at 12 and 60 documents per collection and with one, three and seven +queries; fusion produced ten per collection every time. Covering the cap needs +real embeddings and therefore an installed bundle, which is what +`bin/retrieval_baseline` is for. +""" + +import asyncio +import csv +from pathlib import Path + +import pytest + +pytest.importorskip("langchain_chroma") + +from langchain_chroma import Chroma # noqa: E402 +from langchain_core.callbacks import ( # noqa: E402 + AsyncCallbackManagerForRetrieverRun, + CallbackManagerForRetrieverRun, +) +from langchain_core.documents import Document # noqa: E402 +from langchain_core.embeddings import DeterministicFakeEmbedding # noqa: E402 +from langchain_core.language_models.fake_chat_models import ( # noqa: E402 + FakeListChatModel, +) + +from retrievers.csv_chroma import HybridRetriever, chroma_settings # noqa: E402 + +# Large enough that the per-collection cap actually binds. At twelve it did +# not: fusion produced fewer documents than the cap, so a test that changed +# the cap on one path only still passed. +COLLECTIONS = {"alpha": 60, "beta": 60} + +# Varied on purpose. With near-identical text BM25 and vector search return the +# same ten documents, fusion never exceeds the per-collection cap, and a test +# that changes the cap on one path only still passes -- which this one did. +WORDS = [ + "kinase phosphorylation cascade", + "cholesterol transport vesicle", + "ubiquitin ligase complex", + "mitochondrial respiratory chain", + "DNA mismatch repair", + "interferon signalling", + "collagen assembly", +] + + +def _bundle(tmp_path: Path, embedding: DeterministicFakeEmbedding) -> Path: + csv_dir = tmp_path / "csv_files" + csv_dir.mkdir(parents=True, exist_ok=True) + for collection, count in COLLECTIONS.items(): + with open(csv_dir / f"{collection}.csv", "w", newline="") as handle: + writer = csv.DictWriter( + handle, fieldnames=["st_id", "display_name", "text"] + ) + writer.writeheader() + for i in range(count): + writer.writerow( + { + "st_id": f"{collection}-{i}", + "display_name": f"{collection} item {i}", + "text": WORDS[i % len(WORDS)] + f" {collection} {i}", + } + ) + Chroma.from_documents( + documents=[ + Document( + page_content=( + f"st_id: {collection}-{i}\n" + f"text: {WORDS[i % len(WORDS)]} {collection} {i}" + ), + metadata={"st_id": f"{collection}-{i}"}, + ) + for i in range(count) + ], + embedding=embedding, + persist_directory=str(tmp_path / collection), + client_settings=chroma_settings(), + ) + return tmp_path + + +@pytest.mark.requires_retrieval_stack +@pytest.mark.parametrize( + "queries", + [ + ["kinase phosphorylation"], + ["nothing matches this at all"], + # Enough distinct queries that fusion overflows the per-collection cap, + # so a change to the cap on one path only is visible. With one query the + # two retrievers return largely the same documents, fusion stays under + # the cap, and such a change passes unnoticed -- as it did here. + [ + "kinase phosphorylation cascade", + "cholesterol transport vesicle", + "ubiquitin ligase complex", + "mitochondrial respiratory chain", + "DNA mismatch repair", + "interferon signalling", + "collagen assembly", + ], + ], +) +def test_async_returns_exactly_what_sync_returns( + tmp_path: Path, queries: list[str] +) -> None: + embedding = DeterministicFakeEmbedding(size=16) + retriever = HybridRetriever.from_subdirectory( + # Never called: these tests drive retrieve_documents directly, below the + # query-expansion step. It is here because the constructor requires one. + llm=FakeListChatModel(responses=[""]), + embedding=embedding, + embeddings_directory=_bundle(tmp_path, embedding), + ) + + sync = retriever.retrieve_documents( + queries, CallbackManagerForRetrieverRun.get_noop_manager() + ) + asynchronous = asyncio.run( + retriever.aretrieve_documents( + queries, AsyncCallbackManagerForRetrieverRun.get_noop_manager() + ) + ) + + # Order, not just membership: RRF resolves ties by first appearance, so a + # reordering is a ranking change and the top documents are the ones that + # reach the model. + assert [d.page_content for d in asynchronous] == [d.page_content for d in sync] From 5230621b7b0b423ed4fa6bc8791ada804f265aa2 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 17 Sep 2026 13:51:38 +0000 Subject: [PATCH 2/2] Give CI the NLTK data the image already installs The new equivalence test failed on GitHub and passed here. BM25 tokenises with word_tokenize(..., language="english"), which needs punkt_tab; the Dockerfile installs it at build time and the CI test job never did. So the suite passed on any machine where a developer had downloaded it once and failed on a clean one -- the classic shape, and it was my test that exposed it rather than caused it. Any future test that drives retrieval would have hit the same wall. CI now fetches the same resource the Dockerfile does, so the async path is genuinely covered there rather than skipped. The test also skips with a message naming the download command when the resource is absent, so a developer without it gets that instead of a LookupError raised from inside nltk. Checked by patching nltk.data.find to raise. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 7 +++++++ .../retrievers/test_sync_async_equivalence.py | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9ef33fd3..c443ce34 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,6 +49,13 @@ jobs: - name: Set up Python and Poetry uses: ./.github/actions/install_python_poetry + # The same data the Dockerfile installs. BM25 tokenises with + # word_tokenize(..., language="english"), so any test that drives + # retrieval needs punkt_tab -- without it the suite passes locally, + # where a developer has downloaded it, and fails here. + - name: Fetch NLTK data used by BM25 + run: poetry run python -m nltk.downloader punkt_tab + - name: Run tests run: poetry run pytest diff --git a/tests/retrievers/test_sync_async_equivalence.py b/tests/retrievers/test_sync_async_equivalence.py index 23303c3c..b0020705 100644 --- a/tests/retrievers/test_sync_async_equivalence.py +++ b/tests/retrievers/test_sync_async_equivalence.py @@ -47,9 +47,26 @@ from retrievers.csv_chroma import HybridRetriever, chroma_settings # noqa: E402 + # Large enough that the per-collection cap actually binds. At twelve it did # not: fusion produced fewer documents than the cap, so a test that changed # the cap on one path only still passed. +def _require_bm25_tokenizer() -> None: + """BM25 tokenises with nltk's word_tokenize, which needs punkt_tab. + + CI installs it, matching the Dockerfile. A developer who has not downloaded + it should get a skip saying so rather than a LookupError from inside nltk. + """ + import nltk + + try: + nltk.data.find("tokenizers/punkt_tab") + except LookupError: + pytest.skip( + "nltk punkt_tab not downloaded: python -m nltk.downloader punkt_tab" + ) + + COLLECTIONS = {"alpha": 60, "beta": 60} # Varied on purpose. With near-identical text BM25 and vector search return the @@ -125,6 +142,7 @@ def _bundle(tmp_path: Path, embedding: DeterministicFakeEmbedding) -> Path: def test_async_returns_exactly_what_sync_returns( tmp_path: Path, queries: list[str] ) -> None: + _require_bm25_tokenizer() embedding = DeterministicFakeEmbedding(size=16) retriever = HybridRetriever.from_subdirectory( # Never called: these tests drive retrieve_documents directly, below the