Add the disease_variants collection, built from the release file - #225
Merged
Merged
Conversation
The chatbot answers about disease one level above the question. Asked to list the ABCA1 variants in Reactome it names none, and says "Defective ABCA1 causes Tangier Disease" instead; asked which diseases involve PTEN variants it gives the PTEN Loss of Function pathway. Reactome curates six ABCA1 variants and 108 PTEN variants across 86 diseases, and disease_variant_ewas_mapping.tsv has all of them with the residue change, the disease identifiers, and the normal reaction each defective one replaces. It is also the only embeddings work not blocked. The four existing collections are generated from Neo4j and we have no credentials for it; this one is a flat TSV, so it follows the alliance path -- MetaDataCSVLoader and build_embeddings, no database -- and can be built and tested while Release 97 waits on access. Two decisions are left for the team: which columns are embedded rather than filterable metadata, and what to do about the disease field, where a third of rows pack up to nineteen diseases into one pipe-delimited string and turn 443 real diseases into 968 apparent ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Asked to list the ABCA1 variants, the chatbot answered "Defective ABCA1 causes Tangier Disease" and named none of the six. The four existing collections hold pathway- and reaction-level prose about disease, so it talks about disease fluently and has no document for the variant itself. It now answers with C1417R, Q537R, N935S, R587W and S1446L, each with its disease and whether it is a loss of function; PTEN moves from "PTEN Loss of Function in Cancer" to naming Q17*, Q97* and Q171* against endometrial cancer. Both open decisions were delegated back, and one of the spec's own recommendations turned out to be impossible. Chroma accepts only str, int, float or bool as a metadata value, so the disease field cannot be split into a list. Fanning out to one document per variant-disease pair was measured rather than guessed: 6,294 documents become 10,500 and p16INK4A R80* repeats 45 times, which is enough near-identical documents to fill a result set. One document per variant it is, with the pipes rewritten as commas. The other decision was which columns to embed. The loader renders each field as "name: value", so the column names are embedded too -- and the release names are the query paths that produced them, up to 104 characters of entityWithAccessionedSequence_reactionLikeEvent_entityFunctionalStatus_... They are renamed to what a person would call them, identifiers are metadata rather than content because nobody types R-HSA-5682201 at a chatbot, and the variant's own identifier is named st_id because that is the key csv_chroma de-duplicates on. The two sweep expectations match a pattern for a named variant rather than a specific one, since which of the six come back depends on retrieval order, and they skip until the collection is installed rather than turning the deploy gate red for a reason nobody can act on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s them With the collection installed the sweep still failed. Retrieval was not the problem: the ABCA1 question retrieves three named variants, and the RAG chain answers it with five in 2,546 characters. The agent answered in 393 and named none. The difference was the router. `live` is described as answering "whether some specific thing exists in it at all", and "List the ABCA1 variants in Reactome" reads exactly like a question about what the database contains -- so with MCP configured it went to the live services, which answer at the level of the pathway and never see the new documents. Beta always has MCP configured, so the collection would have been dead there while passing every local test. The rule now draws the line at scope versus content: live answers how many, which species, which release and whether a thing exists at all; reactome answers what is curated about a named gene, disease or pathway, including listing it. Checked in both directions -- the two variant questions now pass and the species and release questions still route live and still pass. 13/13 against the running MCP sibling. The prompt is unchanged where `live` is not offered, which FR-007 requires byte for byte, and a test covers that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerating appended instead of replacing. Chroma.from_documents adds to whatever is already in the directory, so a second `make` would have produced 12,588 documents -- every variant twice, retrieved twice in a result set, with no error anywhere. It now removes the collection first, and clears chromadb's cached system client, which otherwise keeps pointing at the deleted sqlite file and fails the next write with "attempt to write a readonly database". `cross_reference` was named `disease_cross_reference`, and it is not all disease: every row carries a Mondo disease id, but 4,239 also carry a COSMIC variant id, plus ClinVar, ClinGen and LOVD. Anyone filtering on that name would have got variant identifiers back. It is `cross_references` now, and the description says what is really in it. The disease identifier proper is `disease_id`, which is DOID throughout -- and that one I did check: it lines up with `disease` position for position on all 6,294 rows. The enumeration rule I added to the reactome prompt was dead weight. I added it on the theory that the model was summarising instead of listing; the real cause was the router sending the question to the live services. Reverted and verified: both variant questions pass without it, so it was costing tokens in every Reactome answer for nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…needed The bundle directory is named Release95 and now holds one collection built from the Release 97 download directory. Nothing recorded that, which is the failure build_embeddings already warns about: a bundle whose contents disagree with its path is unusable in a way nothing reports. Generating now writes provenance.json beside the collections, merged so each keeps its own entry. Spec 009 is Adam's concern, measured. Every collection is searched for every question and contributes a fixed ten documents. Adding the fifth cost 2,376 tokens and 3.4 seconds on a question about CDK5 and Alzheimer disease, which has nothing to do with variants -- +34% and +30%, so three more tables is roughly +7K tokens and +10s on everything. The allocation is also inverted: disease_variants contributed 15% of the context to that CDK5 question and 5% to the ABCA1 question it exists for, because every collection gets ten documents whether or not they are any good and variant documents are short where pathway prose is long. The proposal costs nothing to run: the intent classifier already makes one LLM call per question, and the per-collection descriptions it would need already exist in metadata_info.py, read today only by bin/retrieval_baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`make` has been broken for this bundle. `metadata_columns` asked for a `species` column that the reactions and summations queries never return -- both filter on speciesName = "Homo sapiens" and then do not select it -- so generation died on the first upload with "Metadata column 'species' not found in CSV file". The Release95 bundle predates it, which is why nobody hit it. Adding it to the queries would have been worse: these collections pass no `content_columns`, so every CSV column is embedded, and a line reading "species: Homo sapiens" in all 52,000 documents is noise in every one of them. The second bug shipped. Chroma.from_documents appends to whatever is already in the directory, and the live Release95 bundle holds 33,498 reaction documents for 16,749 CSV rows -- exactly 2.00x, every reaction stored twice, because generation ran twice. Nothing reported it, and since the vector retriever overfetches and then de-duplicates on st_id, half that overfetch was being spent on duplicates. Regeneration now replaces, and clears chromadb's cached system client so the next write does not fail on a deleted sqlite file. Release 97 builds 17,004 reactions for 17,004 rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured each flag rather than guessing: warn_unused_configs and disallow_subclassing_any both report zero errors, so they cost nothing to hold and stop that slippage arriving unnoticed. warn_unused_ignores reported five, all stale "type: ignore" comments in one test file -- a stale ignore is a claim about the code that is no longer true. Left for a per-module ratchet, with the numbers: no_implicit_reexport (10), disallow_untyped_calls (22), disallow_any_generics (52). Those three bite because ignore_missing_imports is on repo-wide, so untyped third-party surfaces leak Any inward; fixing them is about stubs, not annotations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Yesterday's review called this rule dead weight, because both variant questions passed without it. Two runs was not enough evidence for a case this marginal. Against the Release 97 bundle the PTEN question fails without it and passes with it: without the rule: 1 of 3 runs named a variant with the rule: 3 of 3 Retrieval is not the difference -- both bundles return the same seven disease_variants documents with the same PTEN variants in them. The model has the variants in context either way and chooses whether to enumerate them or summarise at the level of the pathway, and the rule is what decides that. The router fix from yesterday was necessary and not sufficient: routing gets the question to the collection, and this gets the answer to name what the collection returned. 13/13 against Release 97 with the live MCP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
no_implicit_reexport: names now come from where they are defined rather than from whichever module imported them first -- ProfileName from agent.profile_names, TriggerEvent from util.config_yml.messages, SharedSystemClient from chromadb.api.shared_system_client. The one deliberate re-export is `logging` from util.logging, now declared in that module's __all__: importing it is what runs dictConfig, so callers take their logger through it on purpose. disallow_untyped_calls: on repo-wide, with three modules exempted. All 22 violations were calls into chainlit's unannotated API or boto3's Session -- nothing fixable by annotating our code -- so exempting those three keeps the rule meaningful everywhere else. bin/chat-chainlit.py carries the exemption as an inline directive because a hyphenated filename cannot be named in an overrides section; warn_unused_configs, enabled earlier today, is what caught that the section was doing nothing. disallow_any_generics stays off, but every bare builtin it flagged is fixed -- nine of them, including `-> tuple` and three `dict[str, list]`. The 43 left are framework generics, and writing Runnable[X, Y] without checking each chain would encode a wrong type rather than an absent one. 317 tests, ruff and mypy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
provenance.json covered disease_variants alone, which implied the other four were unknown rather than simply unrecorded. upload_to_chromadb now records every collection it builds, and the Release97 bundle is backfilled so all five carry their source and row count. Spec 009 gains the decision it was missing: the answer sweep is the pass/fail, retrieval_baseline is captured and its diff reported because Principle II requires the measurement, and no numeric document-loss threshold is set until there is one real measurement to set it from. A dropped document is what this change is for; a wrong answer is the defect. Any threshold chosen now would be invented, and a number that looks rigorous and means nothing is the failure this project keeps finding. Its weakness is written down rather than hidden: thirteen questions cannot cover five collections, so growing the tracked set is part of the work. Four of the five collections currently have no question that fails if routing stops searching them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The chatbot answers about disease one level above the question. Asked to list the ABCA1 variants in Reactome it names none of the six and says "Defective ABCA1 causes Tangier Disease" instead.
The four existing collections hold pathway- and reaction-level prose about disease, so the chatbot talks about disease fluently and has no document for the variant itself. It answers a level up and hedges — a fluent answer that doesn't contain the fact asked for.
Why this one could be done now
It's the only embeddings work that isn't blocked. The four existing collections are generated from Neo4j and we have no credentials; this is a flat TSV, so it follows the
alliancepath —MetaDataCSVLoaderandbuild_embeddings, no database. 6,294 variants, 400 genes, cost in cents.The two decisions, delegated back and decided
Which columns are embedded. The loader renders each field as
name: value, so the column names are embedded too — and the release names are the query paths that produced them, up to 104 characters ofentityWithAccessionedSequence_reactionLikeEvent_entityFunctionalStatus_…. Left alone they'd contribute more tokens than the values, identically in every document. Renamed to what a person would call them:Identifiers are metadata, not content — nobody types
R-HSA-5682201at a chatbot. The variant's own identifier is namedst_idbecause that's the keycsv_chromade-duplicates on; a different name would silently disable de-duplication for this collection.The pipe-delimited
diseasefield. The spec's own recommendation — split it into a list for metadata — turned out to be impossible: Chroma accepts onlystr,int,floatorbooland rejects a list outright. So I measured the alternative rather than guessing: fanning out to one document per variant-disease pair takes 6,294 documents to 10,500, and repeatsp16INK4A R80*45 times — enough near-identical documents to fill an entire result set. One document per variant, pipes rewritten as commas.Verification
Two answer-sweep expectations match a pattern for a named variant (
\bABCA1 [A-Z]\d{2,4}[A-Z*]) rather than a specific one, because which of the six come back depends on retrieval order and pinning one would fail a good answer — the mistake made in #223 and fixed there. Checked against the recorded answers: the pattern doesn't match the old one and does match the new.They carry
needs_collection="disease_variants"and skip loudly until the bundle ships, rather than turning the deploy gate red for a reason nobody can act on. A test covers the direction that matters — that they run once it's installed. The skip summary is now grouped by reason, since it previously said "they need the live service" for every skip.313 tests, ruff and mypy clean.
Installing it
The embeddings tree is root-owned, so installing needs sudo — and using sudo is what keeps it root-owned. It is not the container's doing: the image has run as
appuser(uid 3001) since 2025-04-17 and the bundle was created 2026-09-02.~/fix-embeddings-ownership.shsetsawright:reactomewith world-read, which suits both — the owner manages bundles without sudo, and the container, whose uid isn't a host user and which only reads at runtime, still can.🤖 Generated with Claude Code