Skip to content

Add the disease_variants collection, built from the release file - #225

Merged
adamjohnwright merged 10 commits into
mainfrom
spec/disease-variant-embeddings
Sep 17, 2026
Merged

adamjohnwright merged 10 commits into
mainfrom
spec/disease-variant-embeddings

Conversation

@adamjohnwright

@adamjohnwright adamjohnwright commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

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.

before after
ABCA1 "Defective ABCA1 causes Tangier Disease… Other diseases may be associated with ABCA1, bu[t]" C1417R, Q537R, N935S, R587W, S1446L, each with its disease and loss-of-function status
PTEN "PTEN Loss of Function in Cancer" — a pathway Q17*, Q97*, Q171* named against endometrial cancer

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 alliance path — MetaDataCSVLoader and build_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 of entityWithAccessionedSequence_reactionLikeEvent_entityFunctionalStatus_…. Left alone they'd contribute more tokens than the values, identically in every document. Renamed to what a person would call them:

gene: ABCA1
variant: ABCA1 W590S [plasma membrane]
residue_change: L-tryptophan 590 replaced with L-serine
mutation_type: ReplacedResidue
disease: Tangier disease
functional_status: loss_of_function
normal_reaction: 4xPALM-C-p-2S-ABCA1 tetramer transports CHOL from transport vesicle...

Identifiers are metadata, not content — nobody types R-HSA-5682201 at a chatbot. The variant's own identifier is named st_id because that's the key csv_chroma de-duplicates on; a different name would silently disable de-duplication for this collection.

The pipe-delimited disease field. The spec's own recommendation — split it into a list for metadata — turned out to be impossible: Chroma accepts only str, int, float or bool and 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 repeats p16INK4A 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.sh sets awright:reactome with 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

adamjohnwright and others added 2 commits September 16, 2026 19:32
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>
@adamjohnwright adamjohnwright changed the title Spec the disease and variant collection Add the disease_variants collection, built from the release file Sep 16, 2026
adamjohnwright and others added 8 commits September 16, 2026 21:06
…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>
@adamjohnwright
adamjohnwright merged commit 8ed71db into main Sep 17, 2026
10 checks passed
@adamjohnwright
adamjohnwright deleted the spec/disease-variant-embeddings branch September 17, 2026 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant