Add the disease_variants collection, built from the release file

bin/chat-chainlit.py

@@ -1,3 +1,7 @@
+# mypy: disallow-untyped-calls=False
+# chainlit ships no annotations for cl.user_session.get/set, cl.Message.send
+# or get_data_layer. Not fixable here; it needs stubs upstream. The file is
+# named with a hyphen, so it cannot be listed in [[tool.mypy.overrides]].
 import os
 
 import chainlit as cl
@@ -9,7 +13,8 @@
 from langchain_community.callbacks import OpenAICallbackHandler
 
 from agent.graph import AgentGraph
-from agent.profiles import ProfileName, get_chat_profiles
+from agent.profile_names import ProfileName
+from agent.profiles import get_chat_profiles
 from agent.profiles.base import OutputState
 from util.chainlit_helpers import (
     PrefixedS3StorageClient,
@@ -19,7 +24,8 @@
     static_messages,
     update_search_results,
 )
-from util.config_yml import Config, TriggerEvent
+from util.config_yml import Config
+from util.config_yml.messages import TriggerEvent
 from util.logging import logging
 from util.orcid_provider import ORCIDOAuthProvider
 from util.secrets import (

bin/embeddings_manager

@@ -276,6 +276,12 @@ if __name__ == "__main__":
         action="store_true",
         help="Force regeneration of CSV files"
     )
+    make_parser.add_argument(
+        "--disease-variant-tsv",
+        help="disease_variant_ewas_mapping.tsv from the release download "
+             "directory. Adds the disease_variants collection, which is built "
+             "from that file rather than from Neo4j.",
+    )
 
     args = parser.parse_args()
     func = args.func

pyproject.toml

@@ -217,9 +217,46 @@ disallow_incomplete_defs = true
 warn_no_return = true
 warn_return_any = true
 extra_checks = true
+# Free: both already pass with zero errors, so they cost nothing to hold and
+# stop the corresponding slippage from arriving unnoticed.
+warn_unused_configs = true
+disallow_subclassing_any = true
+# A stale ignore is a claim about the code that is no longer true. There were
+# five, all in one test file, all obsolete.
+warn_unused_ignores = true
+# Names now come from where they are defined rather than from whichever module
+# happened to import 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, `logging`
+# from util.logging, is declared in that module's __all__ -- importing it is
+# what runs dictConfig, so callers take the logger through it on purpose.
+no_implicit_reexport = true
+# Enabled repo-wide, with three modules exempted below. Every violation is a
+# call into an untyped third-party API rather than anything we could annotate,
+# so exempting those three keeps the rule meaningful everywhere else: new
+# untyped calls cannot appear in ordinary code.
+disallow_untyped_calls = true
+# disallow_any_generics is the one strict setting still off. Every bare builtin
+# it flagged is now parameterised; the 43 left are all framework generics --
+# Runnable (29), StateGraph (9), CompiledStateGraph (3), RunnableLambda and
+# BasePromptTemplate. Turning it on means writing the real input and output
+# type of every chain and graph, and a guess there encodes a type that is
+# wrong rather than absent, which is worse than the bare form. It needs doing
+# per chain, with the chain in front of you.
 # disallow_untyped_decorators stays off: chainlit's @cl.* decorators are
 # untyped upstream, so it only ever fires on bin/chat-chainlit.py.
 
+# Untyped third-party surfaces, not our debt. chainlit ships no annotations for
+# `cl.user_session.get/set`, `cl.Message.send` or `get_data_layer`, and boto3's
+# `Session` is untyped without boto3-stubs. Nothing here is fixable by
+# annotating our own code; it needs stubs upstream or a typed wrapper of our
+# own, which is a change with its own design rather than a lint fix.
+# bin/chat-chainlit.py carries the same exemption as an inline directive: its
+# hyphenated name cannot appear in an overrides section.
+[[tool.mypy.overrides]]
+module = ["util.chainlit_helpers", "util.secrets"]
+disallow_untyped_calls = false
+
 # --- mypy baseline -------------------------------------------------------------
 # Modules that do not yet meet the floor above. Each entry is debt: fix the module,
 # then delete its block. Do not add to this list without a TODO.

specs/008-disease-variant-embeddings/spec.md

@@ -0,0 +1,168 @@
+# Feature Specification: Disease and Variant Embeddings
+
+**Feature Branch**: `spec/disease-variant-embeddings`
+
+**Created**: 2026-09-16
+
+**Status**: Implemented. Both decisions were delegated back and are recorded below,
+with one of the original recommendations withdrawn as impossible.
+
+**Input**: Add disease and variant data to the embeddings, from
+`disease_variant_ewas_mapping.tsv` in the release download directory. Do it as a new
+collection alongside regenerating the existing ones for Release 97.
+
+## Why: the chatbot answers about diseases, but cannot name a variant
+
+Asked on 2026-09-16 against beta running `a4de878`, with live MCP:
+
+> **List the ABCA1 variants in Reactome and the disease each one causes.**
+>
+> "The Reactome database lists the following disease associated with ABCA1 variants:
+> **Defective ABCA1 causes Tangier Disease (TGD)** ... This is the primary disease
+> explicitly mentioned in the context of ABCA1 variants. Other diseases may be
+> associated with ABCA1, bu[t] ..."
+
+Zero variants named. Reactome curates **six** ABCA1 variants, and the file has them:
+`ABCA1 W590S`, `N935S`, `C1417R` and three more, each with its stable ID and the
+residue change in prose ("L-tryptophan 590 replaced with L-serine").
+
+> **Which diseases involve variants of the PTEN gene in Reactome?**
+>
+> "... **PTEN Loss of Function in Cancer**: This pathway highlights ..."
+
+Answered at pathway level. Reactome curates **108 PTEN variants across 86 diseases**.
+
+The pattern is consistent: the four existing collections (`complexes`, `ewas`,
+`reactions`, `summations`) hold pathway- and reaction-level prose about disease, so
+the chatbot says *"defective ABCA1 causes Tangier disease"* fluently. What it has no
+document for is the individual variant entity. It therefore answers a level up from
+the question and hedges, which is the failure mode this project keeps finding: a
+fluent answer that does not contain the fact asked for.
+
+## Why this one is worth doing now
+
+It is the only part of the embeddings work that is **not blocked**.
+
+| | source | needs |
+|---|---|---|
+| `complexes`, `ewas`, `reactions`, `summations` | Neo4j at `bolt://localhost:7687` | credentials we do not have |
+| **disease/variant** | **a flat TSV in the download directory** | **nothing** |
+
+`generate_reactome_embeddings` reads Neo4j. `generate_alliance_embeddings` does not --
+it reads flat files through `MetaDataCSVLoader` and `build_embeddings`. That is the
+precedent to follow, and it means this collection can be built, installed and tested
+today while the Release 97 regeneration waits on access.
+
+## The data
+
+`.../static/download/97/disease_variant_ewas_mapping.tsv`, 5.3 MB, **6,294 rows**,
+26 columns, one row per variant entity (6,294 distinct `stable_id`, so no duplication).
+
+- **400** genes, **462** disease pathways
+- **443** distinct diseases -- *not* the 968 distinct `disease` strings, see D2
+- Best covered: cancer (1,700 variants), Kabuki syndrome (564), acute myeloid
+  leukaemia (126), ornithine carbamoyltransferase deficiency (104)
+
+Each row already carries the chain a user actually asks about: gene, variant display
+name, the residue change in prose, the disease with Mondo/DOID identifiers, the
+reaction the variant takes part in, its functional status, **and the normal reaction
+and pathway it is the defective counterpart of**.
+
+Fill rates are high: 20 of 26 columns are ≥95% populated. Two are effectively empty
+and should be dropped (`normal_reaction_like_event_go_biological_process_*`, 6.3%);
+`entityWithAccessionedSequence_literatureReference_pubMedIdentifier` is 30%.
+
+## Decisions
+
+Both were delegated: *"I think you should decide on the columns ... make your best
+design and go ahead."*
+
+### D1 -- what is embedded, and what is metadata: decided
+
+The loader renders each field as `name: value`, so the **column names are embedded
+too**. The release names are the query paths that produced them, up to 104 characters
+of `entityWithAccessionedSequence_reactionLikeEvent_entityFunctionalStatus_...`. Left
+alone they would contribute more tokens than the values, identically in every
+document. They are renamed to what a person would call them.
+
+Embedded: `gene`, `variant`, `protein`, `residue_change`, `mutation_type`, `disease`,
+`reaction`, `functional_status`, `disease_pathway`, `normal_reaction`,
+`normal_pathway`, `normal_process`. A document reads:
+
+```
+gene: ABCA1
+variant: ABCA1 W590S [plasma membrane]
+residue_change: L-tryptophan 590 replaced with L-serine
+mutation_type: ReplacedResidue
+disease: Tangier disease
+reaction: Defective ABCA1 does not transport CHOL from transport vesicle membrane...
+functional_status: loss_of_function
+normal_reaction: 4xPALM-C-p-2S-ABCA1 tetramer transports CHOL from transport vesicle...
+```
+
+Metadata, not embedded: every identifier -- `st_id`, `uniprot_id`, `disease_id`,
+`disease_cross_reference`, and the four Reactome stable IDs. Nobody types
+`R-HSA-5682201` at a chatbot, and embedding it costs tokens in every document. The
+variant's own identifier is named **`st_id`** because that is the key `csv_chroma`
+de-duplicates on; a different name would silently disable de-duplication here.
+
+Dropped: the two `go_biological_process` columns at 6.3% fill, and
+`first_entitySet`. A column empty in 94% of documents earns nothing and costs a line
+of `name:` in every one.
+
+Median document: 556 characters, about 140 tokens.
+
+### D2 -- the pipe-delimited `disease` field: decided, and the earlier recommendation withdrawn
+
+This spec first recommended splitting the field into a list for metadata. **That is
+not possible**: Chroma accepts only `str`, `int`, `float` or `bool` as a metadata
+value and rejects a list outright.
+
+Fanning out to one document per variant-disease pair was measured rather than
+estimated: 6,294 documents become **10,500** (+67%), and `p16INK4A R80*` would be
+repeated **45 times**. Forty-five near-identical documents can fill an entire result
+set, which is a worse failure than a long disease string.
+
+Decided: **one document per variant**, with `|` rewritten to `, ` so the field reads
+as a list rather than a path. All disease names stay searchable in the content, and
+the metadata value stays a string Chroma will accept.
+
+## Scope
+
+In: one new collection built from this one file; wiring it into the retriever
+alongside the existing four; answer-sweep expectations that fail if it regresses.
+
+Out: `HumanDiseasePathways.txt` and `Reactome2OMIM.txt` -- not needed for this
+(confirmed 2026-09-16); regenerating the four Neo4j-backed collections for Release 97;
+publishing to S3, which is blocked separately -- this host's instance profile is
+`EC2CloudwatchAgentRole` and `head_bucket` on `download.reactome.org` returns 403.
+
+## Result, measured
+
+Built into a scratch bundle and asked through the real retriever:
+
+| | before | after |
+|---|---|---|
+| ABCA1 | *"Defective ABCA1 causes Tangier Disease ... Other diseases may be associated with ABCA1, bu[t]"* -- no variant named | **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 |
+
+Two answer-sweep expectations pin this. They 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 `007` and fixed there. Checked against the recorded answers: the
+pattern does not match the old answer and does match the new one.
+
+They carry `needs_collection="disease_variants"` and skip, loudly, until the bundle
+ships -- otherwise they would turn the deploy gate red for a reason nobody can act
+on. A test covers the direction that matters: that they run once it is installed.
+
+Cost was cents. The collection is a fraction of the 3.4 GB the existing four take.
+
+## 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, so something was
+run under sudo on the host. `~/fix-embeddings-ownership.sh` sets `awright:reactome`
+with world-read, which suits both: the owner can manage bundles without sudo, and the
+container, whose uid is not a host user and which only reads at runtime, still can.

specs/009-collection-routing/spec.md

@@ -0,0 +1,134 @@
+# Feature Specification: Searching Only the Collections a Question Needs
+
+**Feature Branch**: `spec/collection-routing`
+
+**Created**: 2026-09-17
+
+**Status**: Draft. Two decisions (D1, D2) for the team.
+
+**Input**: *"the more tables we add the more tokens we use up and the longer the
+responses take to return, it would be nice to search the disease variant table if
+that is what they are asking about."*
+
+## Clarifications
+
+### Session 2026-09-17
+
+- Q: When routing skips a collection that would have contributed a document, how much recall loss should reject the change? → A: Gate on answers, measure documents. No numeric document-loss threshold is set until there is one real measurement to set it from.
+
+The answer sweep staying green is the pass/fail. `bin/retrieval_baseline` is captured
+before and after and its diff reported, because Principle II requires the measurement
+-- but a changed document set does not by itself block, since dropping documents is
+what this change is for. A wrong answer is the defect; a dropped document is data.
+
+Any threshold chosen today would be invented: there is no evidence yet on how often
+the classifier picks wrong, and a number that looks rigorous and means nothing is the
+failure this project keeps finding.
+
+The weakness in that, stated rather than hidden: thirteen questions is a thin net, and
+routing could break something nobody tracks. So growing the tracked set is part of
+this work rather than a follow-up -- see Success Criteria.
+
+## The cost is real, and it was measured
+
+Every collection in the bundle is searched for every question. `retrieve_documents`
+loops over all of them and each contributes up to `max_documents_per_collection`
+(10), regardless of whether it had anything to say.
+
+Adding `disease_variants` as a fifth collection, asked a question with nothing to do
+with variants -- *"What does CDK5 phosphorylate in Alzheimer disease?"*:
+
+| | docs | context tokens | retrieval |
+|---|---|---|---|
+| 4 collections | 40 | 7,061 | 11.5s |
+| 5 collections | 50 | 9,437 | **14.9s** |
+
+**+2,376 tokens (+34%) and +3.4s (+30%)**, all of it noise for that question. The
+cost is per collection and per question, so three more tables is roughly +7K tokens
+and +10s on every question anyone asks.
+
+## The allocation is also inverted
+
+Worse than linear cost: `disease_variants` contributes **more** to questions it is
+irrelevant to than to the one it exists for.
+
+| question | tokens from `disease_variants` | share of context |
+|---|---|---|
+| What does CDK5 phosphorylate in Alzheimer disease? | 1,427 | **15%** |
+| How does TP53 regulate PTEN transcription? | 1,135 | 8% |
+| List the ABCA1 variants in Reactome | 616 | **5%** |
+
+Each collection gets a fixed ten documents whether or not they are any good, and
+variant documents are short (556 characters median) where pathway prose is long. So
+the fixed allocation spends the most context on the collection with the least to
+contribute, and the least on the one that answers the question.
+
+## The proposal: decide collections in the call that already happens
+
+The intent classifier already runs **one LLM call per question** and returns a
+source (`reactome`, `userguide`, `live`). Having it also name collections adds **no
+latency and no call**.
+
+The descriptions it would need already exist. `reactome_descriptions_info` in
+`src/retrievers/reactome/metadata_info.py` holds a written description of every
+collection and is currently read only by `bin/retrieval_baseline`. It was written
+for exactly this kind of routing and is otherwise unused in the serving path.
+
+`HybridRetriever.retrieve_documents` then loops over the selected collections
+instead of all of them -- a filter on `self.collection_retrievers.items()`.
+
+## Decisions
+
+### D1 -- what happens when the classifier is unsure
+
+Recommendation: **select all collections**. A wrong selection costs recall silently,
+which is the failure this project keeps finding; a wrong *default* costs only what
+we already pay today. So the change can only make things faster, never worse than
+the current behaviour, unless the classifier actively picks a wrong subset.
+
+The alternative -- always include a core set and gate only specialised collections
+-- does not scale: the fifth collection is specialised today, but the reason for
+this spec is that there will be a tenth.
+
+### D2 -- fixed allocation, or relevance-weighted
+
+Routing fixes *which* collections are searched. It does not fix the inversion above,
+which is the fixed ten-per-collection cap.
+
+Option (a): leave the cap alone. Simple, and routing alone removes most of the waste.
+
+Option (b): give the 50-document budget out by score rather than by collection, so a
+collection with nothing relevant contributes nothing even when it is searched.
+
+Recommendation: **(a) first, measured, then (b) separately**. They are independent,
+and doing both at once makes it impossible to say which one moved the numbers.
+
+## How we will know it worked
+
+`bin/retrieval_baseline` exists precisely for this: it captures what each retriever
+returns for a fixed question set and diffs two captures. Measured across two
+identical runs BM25 is byte-identical on 80/80 question-collections, so a diff after
+this change is a real difference in behaviour rather than noise.
+
+- `capture` before and after; the diff names every question whose documents changed
+- the answer sweep must stay at 13/13 -- including the two variant questions, which
+  fail if routing sends them past `disease_variants`, and the species and release
+  questions, which fail if it stops routing to `live`
+- the token and latency numbers above, re-measured
+- the tracked question set grown before the change lands, not after. Thirteen
+  questions cannot cover five collections; each collection needs at least one
+  question that fails if routing stops searching it. `disease_variants` has two
+  already, and the other four have none.
+
+That last one matters: the point of this change is a number going down, and it
+should be reported as one.
+
+## Scope
+
+In: collection selection for the `reactome` source, defaulting to all; the
+measurement above.
+
+Out: the relevance-weighted allocation of D2, which is its own change; `userguide`,
+which has one collection; anything about the four Neo4j-backed collections being
+Release95 while `disease_variants` came from the Release 97 download directory --
+that is recorded in the bundle's `provenance.json` and is spec 008's problem.