From 245163422d668455d117c44a5a272b27bf6d6ab5 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 20:29:37 +0000 Subject: [PATCH 1/2] Spec 005: live Reactome data and analysis The three MCP PRs from @GovindhKishore, and the fact that most of what they are justified by is a cheaper problem. Their motivation is that MCP tools query live APIs "regardless of when embeddings were built". True, and it conflates two problems. The bundle is two releases behind -- installed Release95, reactome.org reports 97 -- and it was already two behind when it was built eight days ago, so this is not drift from age. Nothing rebuilt it against a current release. That needs bin/embeddings_manager and a schedule, not an MCP server, a subprocess and a second router. What MCP alone can do is analysis: enrichment, traversal, entity lookup. None of that is similarity search over stored text, and no amount of rebuilding produces it. That is the honest reason to take the work, and it is the smaller-sounding half. Costs, all verified rather than assumed: reactome-mcp describes itself as "just a prototype for now" and was last pushed 2026-07-01; #127 spawns the server as a subprocess with stdio pipes, in a container that now runs as non-root and spawns nothing today; and #142 adds a second LLM classifier beside the intent classifier that already routes, in a pipeline where cutting LLM calls from 21 to one was the point of spec 001. Recommendation is to rebuild the bundle first and independently, which removes staleness from MCP's justification and leaves the real case, then adopt #127 and #137 behind a flag with routing folded into the existing classifier rather than #142's sibling. Recorded in the checklist: nobody has run reactome-mcp from this repository, so its tools, latency and failure behaviour are taken from a PR description rather than observed. Co-Authored-By: Claude Opus 5 --- .../checklists/requirements.md | 57 +++++ specs/005-mcp-live-data/spec.md | 203 ++++++++++++++++++ 2 files changed, 260 insertions(+) create mode 100644 specs/005-mcp-live-data/checklists/requirements.md create mode 100644 specs/005-mcp-live-data/spec.md diff --git a/specs/005-mcp-live-data/checklists/requirements.md b/specs/005-mcp-live-data/checklists/requirements.md new file mode 100644 index 0000000..b68c49a --- /dev/null +++ b/specs/005-mcp-live-data/checklists/requirements.md @@ -0,0 +1,57 @@ +# Specification Quality Checklist: Live Reactome Data and Analysis + +**Purpose**: Validate specification completeness and quality before planning +**Created**: 2026-09-10 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details beyond what the contributed PRs already fix +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Adversarial review of this specification + +Every load-bearing claim was checked rather than reasoned about, after spec 004 +shipped a wrong one. + +| claim | how it was checked | verdict | +|---|---|---| +| "the bundle is two releases behind" | `cat embeddings/current` → Release95; `reactome.org/ContentService/data/database/version` → 97 | **verified** | +| "it was already stale when built" | bundle mtime 2026-09-02, eight days ago | **verified** — so this is not drift from age | +| "`reactome-mcp` is a prototype" | its own repository description: *"This is just a prototype for now"*, last push 2026-07-01 | **verified** | +| "#127 spawns a subprocess" | `asyncio.create_subprocess_exec` with stdio pipes, in its diff | **verified** | +| "#142 duplicates the existing router" | it adds `create_query_router`; `create_intent_classifier` already exists and routes reactome/userguide | **verified** | + +### The claim this specification does not make + +That MCP fixes staleness. It would, but so would rebuilding the bundle, and the +rebuild needs no dependency, no subprocess and no router. Stating it that way is the +whole contribution of this document: the PRs' own justification is mostly a cheaper +problem wearing the expensive problem's clothes. + +### Not verified, and worth knowing + +Nobody has run `reactome-mcp` from this repository. Its 53 tools, their latency and +their behaviour under failure are taken from #127's description, not observed. Any +plan built on this should start by running it once. + +## Notes + +Deviations from the template, deliberate: + +1. **Three contributed PRs are reviewed in the spec body.** They are the starting + material and the reason to take or reject each is inseparable from reading them. +2. **The spec argues against most of its own feature's justification.** That is the + finding, not a digression. diff --git a/specs/005-mcp-live-data/spec.md b/specs/005-mcp-live-data/spec.md new file mode 100644 index 0000000..95c2c79 --- /dev/null +++ b/specs/005-mcp-live-data/spec.md @@ -0,0 +1,203 @@ +# Feature Specification: Live Reactome Data and Analysis + +**Feature Branch**: `spec/mcp-live-data` + +**Created**: 2026-09-10 + +**Status**: Draft. Three contributed PRs; two decisions (D1, D2) for the team. + +**Input**: #127, #137 and #142 from @GovindhKishore, integrating the `reactome-mcp` +server so the chatbot can query live Reactome APIs and run enrichment analysis. + +## Two problems, wrongly bundled as one + +The PRs are motivated by a single sentence: MCP tools *"query Reactome APIs directly, +meaning they always return current data regardless of when embeddings were built."* +That is true, and it conflates two problems with very different costs. + +### Problem 1 — the answers are two releases out of date + +Measured today: + +| | | +|---|---| +| installed bundle | **Release95** | +| bundle built | 2026-09-02, eight days ago | +| `reactome.org/ContentService/data/database/version` | **97** | + +The bundle is two releases behind, and it was already two behind on the day it was +built. So this is not drift from age — nothing rebuilt it against a current release. + +**This does not need MCP.** It needs the bundle rebuilt, and rebuilt on a schedule. +That is `bin/embeddings_manager` and a cron entry, against a capability the +repository already has. + +### Problem 2 — the chatbot cannot analyse anything + +`reactome-mcp` exposes enrichment analysis, pathway traversal and entity lookup. +None of that is expressible as similarity search over stored text. No amount of +rebuilding fixes it, because it is not a retrieval problem: the user gives a gene +list and wants a computation. + +**This is the part only MCP can do**, and it is the honest reason to take the work. + +Separating them matters because Problem 1 is most of the stated benefit and the +cheapest fix, while Problem 2 is the smaller-sounding benefit that actually requires +the architecture. + +## What the three PRs do + +| PR | adds | lines | +|---|---|---| +| #127 | an MCP client speaking JSON-RPC over stdio to a spawned subprocess | +161 | +| #137 | five MCP tools wrapped as LangChain `StructuredTool`s, wired to React-to-Me | +317/−20 | +| #142 | an LLM router choosing between RAG, MCP search and MCP analysis | +450/−20 | + +They build on each other in order and are the work of one contributor. Taken +together they are a coherent design, and the sequencing is right: client, then +tools, then routing. + +## What taking them costs + +**A prototype dependency.** `reactome/reactome-mcp` is Reactome's own repository, +which is the good case — but its description reads *"This is just a prototype for +now"* and it was last pushed 2026-07-01, over two months ago. The chatbot would take +a runtime dependency on it. + +**A subprocess in the container.** #127 uses `asyncio.create_subprocess_exec` with +stdio pipes, so the image must carry the MCP server and its runtime, and each chat +process spawns and supervises a child. The container now runs as a non-root user +(#198), which that must work under. Nothing today spawns a process; this would be +the first. + +**A second router.** #142 adds `create_query_router` alongside the existing +`create_intent_classifier`, which already routes between `reactome` and `userguide`. +Two LLM classification calls on the same question, in a pipeline where removing +LLM calls was the point of spec 001 — retrieval went from 21 calls to one. The +existing classifier should gain the new destinations rather than acquire a sibling. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 — Answers reflect the current release (Priority: P1) + +A user asks about a pathway added in Release 96 or 97 and gets it. + +**Why this priority**: It is the largest part of the stated benefit and by far the +cheapest to deliver. It is P1 *and* it is not what the PRs build. + +**Independent Test**: Rebuild the bundle against Release97, ask about content added +since 95, compare against today. + +**Acceptance Scenarios**: + +1. **Given** a rebuilt bundle, **When** a user asks about recent content, **Then** + the answer includes it. +2. **Given** a release cadence, **When** a new release ships, **Then** rebuilding is + a scheduled operation rather than a thing someone remembers. + +--- + +### User Story 2 — The chatbot can run an enrichment analysis (Priority: P2) + +A user pastes a gene list and asks which pathways are over-represented. The chatbot +runs the analysis and explains the result. + +**Why this priority**: Genuinely new capability, and impossible without something +like MCP. P2 below staleness only because staleness affects every question and this +affects a class of question we do not serve at all today. + +**Acceptance Scenarios**: + +1. **Given** a gene list, **When** analysis is requested, **Then** a real Reactome + analysis runs and its result is explained. +2. **Given** the MCP server is unavailable, **When** analysis is requested, **Then** + the chatbot says so plainly rather than answering from the vector store as if it + had analysed anything. + +--- + +### User Story 3 — Routing costs one classification, not two (Priority: P2) + +A question is classified once, into one of the destinations available. + +**Acceptance Scenarios**: + +1. **Given** MCP is enabled, **When** a question arrives, **Then** exactly one + classification call is made. +2. **Given** MCP is disabled, **When** a question arrives, **Then** behaviour and + call count are exactly as today. + +### Edge Cases + +- **The MCP server dies mid-conversation.** A supervised subprocess needs a defined + answer: restart, degrade to RAG, or fail. Silently degrading to RAG is the worst + option, because an analysis question would get a retrieval answer. +- **The prototype changes its tool surface.** Five tools are wrapped by name; a + rename upstream breaks them at call time, not at start-up. +- **Live and stored data disagree.** The vector store says Release95, the API says + 97. An answer that mixes both without saying so is a new class of wrong. +- **Analysis latency.** Enrichment is not a sub-second call, and the chat surface + already runs at 22s per question. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The installed bundle MUST be rebuildable against the current release + without code changes. +- **FR-002**: Rebuilding MUST be schedulable, not a remembered manual step. +- **FR-003**: The deployed release version MUST be visible — an operator must be able + to see which release is being answered from. +- **FR-004**: If MCP is adopted, it MUST be optional: with it disabled, behaviour and + LLM call count are exactly as today. +- **FR-005**: If MCP is adopted, questions MUST be classified exactly once, by + extending the existing intent classifier rather than adding a second. +- **FR-006**: An unavailable MCP server MUST produce an explicit failure for + analysis questions, never a silent fall back to retrieval. +- **FR-007**: Where an answer draws on live API data rather than the bundle, that + MUST be distinguishable. + +## Success Criteria *(mandatory)* + +- **SC-001**: The gap between the deployed bundle's release and Reactome's current + release is at most one. +- **SC-002**: An operator can determine the answering release without reading code. +- **SC-003**: With MCP disabled, a question costs the same LLM calls as today. +- **SC-004**: A gene-list question produces a real analysis or an explicit refusal, + never a retrieval answer dressed as one. + +## Decisions for the team + +### D1 — Rebuild the bundle now, independently of MCP? + +Recommended: **yes, and first.** Two releases behind is the larger share of the +stated benefit, it needs no new dependency, no subprocess and no router, and it can +ship this week. It also makes the MCP decision honest by removing staleness from its +justification, leaving analysis — which is the real case. + +### D2 — Adopt MCP for analysis? + +| option | what it means | +|---|---| +| **A. Not yet** | Rebuild bundles, revisit when `reactome-mcp` is past prototype. Costs nothing; the capability gap remains. | +| **B. Adopt behind a flag** | Take #127 and #137, fold routing into the existing classifier rather than #142's second one, ship disabled by default. Real capability, contained blast radius, a prototype dependency that cannot affect anyone who has not enabled it. | +| **C. Adopt fully** | Everything the PRs propose, on by default. Fastest to the capability, and puts a self-described prototype and a supervised subprocess in the path of every user. | + +**Recommendation: B**, contingent on D1 being done first. It buys the irreplaceable +part while the dependency is still a prototype, and FR-004 means the cost of being +wrong is a flag nobody turned on. + +## Assumptions + +- `reactome-mcp` staying a Reactome project. If it were third-party the answer would + be different; a prototype from the same organisation is a shared risk, not an + external one. +- Rebuilding bundles is routine. If it is not, that is the finding, and it makes + D1 more urgent rather than less. + +## Out of Scope + +- Which MCP tools to wrap beyond the five #137 chose. +- Replacing retrieval with MCP. Retrieval over curated text is what the product is; + MCP adds computation beside it. +- Analysis result presentation in the UI. From eb31e6aaae847e82595f8b018b7d25365e069472 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 14 Sep 2026 13:33:17 +0000 Subject: [PATCH 2/2] Add a pre-flight for the Release 98 rebuild D1 is answered: Release 98 is nearly done and new embeddings go to production then. So MCP is to be judged on analysis alone, which was the point of separating the two problems. Checked the rebuild path against this machine rather than assuming it works, and three things are not ready. Each fails partway through a long job rather than at the start. Disk: the bundle is 3.4G and there is 5.2G free on a 95%-full volume, so a second one does not fit comfortably -- and the old one cannot be deleted first because the running chatbot is serving from it. S3: there is no ~/.aws, no AWS_* in the environment and nothing in .env, so ls-remote, pull and push all fail with AccessDenied. CI assumes an AWS role by OIDC but only for the ECR image push, not the embeddings bucket. push is how a bundle reaches production, so this blocks the move, not just the build. Neo4j: make reads the Reactome graph and defaults to localhost, and nothing documents it. The connection has to be passed explicitly. What is ready is the part most likely to have rotted: every data_generation module imports cleanly on LangChain 1.x. Co-Authored-By: Claude Opus 5 --- deploy/rebuilding-embeddings.md | 106 ++++++++++++++++++++++++++++++++ specs/005-mcp-live-data/spec.md | 16 ++++- 2 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 deploy/rebuilding-embeddings.md diff --git a/deploy/rebuilding-embeddings.md b/deploy/rebuilding-embeddings.md new file mode 100644 index 0000000..efe1764 --- /dev/null +++ b/deploy/rebuilding-embeddings.md @@ -0,0 +1,106 @@ +# Rebuilding the embeddings bundle + +For a new Reactome release. Written 2026-09-14, ahead of Release 98, after checking +each step against this machine — the gaps below are real, not hypothetical. + +## Where things stand + +| | | +|---|---| +| installed bundle | `openai/text-embedding-3-large/reactome/Release95` | +| built | 2026-09-02 | +| Reactome current | **97**, with 98 nearly done | + +The bundle was already two releases behind on the day it was built, so this is not +drift — nothing rebuilt it against a current release. That is the whole reason this +document exists. + +## Pre-flight: three things that are not ready + +Checked, not assumed. Each one fails partway through a long job rather than at the +start, which is the worst way to find out. + +### 1. Disk — the tightest constraint + +``` +existing bundle 3.4 G +free on / 5.2 G (95% used) +``` + +A second bundle needs roughly another 3.4 G, leaving under 2 G for the operating +system, Docker and the build's own scratch space. **This will not fit comfortably.** + +Before starting: free space, or build somewhere else. `deploy/beta/reclaim-docker-space.sh` +exists for the Docker side. Note the old bundle cannot simply be deleted first — +the running chatbot is serving from it. + +### 2. S3 — no credentials on this machine + +``` +$ ./bin/embeddings_manager ls-remote +botocore.errorfactory.AccessDenied: ... ListObjects ... Access Denied +``` + +There is no `~/.aws`, no `AWS_*` in the environment, and nothing in `.env`. So +`pull`, `push` and `ls-remote` all fail here. CI authenticates to AWS by OIDC role +assumption, but only for the ECR image push — not for the embeddings bucket. + +`push` is how a new bundle reaches production, so **this must be solved before the +rebuild, not after it**. + +### 3. Neo4j — needed, undocumented + +`embeddings_manager make` reads the Reactome graph database and defaults to +`bolt://localhost:7687`. Nothing in `.env` or `env_template` configures it, so the +connection must be passed explicitly: + +```bash +./bin/embeddings_manager make \ + openai/text-embedding-3-large/reactome/Release98 \ + --neo4j-uri bolt://:7687 \ + --neo4j-username \ + --neo4j-password +``` + +## What is ready + +The generation code itself. Every `data_generation` module imports cleanly on +LangChain 1.x after the 2026-09-09 upgrade — `reactome`, `neo4j_connector`, +`uniprot`, `alliance` and `userguide`. That was the part most likely to have rotted, +and it has not. + +## The sequence + +```bash +./bin/embeddings_manager make openai/text-embedding-3-large/reactome/Release98 \ + --neo4j-uri ... --neo4j-username ... --neo4j-password ... +./bin/embeddings_manager push openai/text-embedding-3-large/reactome/Release98 +./bin/embeddings_manager use openai/text-embedding-3-large/reactome/Release98 +``` + +Then restart the chatbot so `AgentGraph` picks up the new bundle, and confirm with +`./bin/embeddings_manager which`. + +## After rebuilding, check retrieval actually improved + +A new bundle is a change to what reaches the model, so constitution Article II +applies. `bin/retrieval_baseline` captures before and after against a fixed question +set: + +```bash +./bin/retrieval_baseline capture --out before-98.json # while 95 is active +# ... rebuild and switch ... +./bin/retrieval_baseline capture --out after-98.json +./bin/retrieval_baseline compare before-98.json after-98.json +``` + +Expect large differences — that is the point — but the comparison shows *what* +changed rather than only that something did. + +## A note on chromadb + +The bundle is written by `langchain_community.vectorstores.Chroma` in +`data_generation` and read by `langchain_chroma` in the retriever. Both sit on +chromadb, which is pinned below 1.0 (see `pyproject.toml`): chromadb 1.x migrates a +bundle's sqlite in place on first open, which needs write access the container does +not have. A bundle built now is readable by both. diff --git a/specs/005-mcp-live-data/spec.md b/specs/005-mcp-live-data/spec.md index 95c2c79..b2eefba 100644 --- a/specs/005-mcp-live-data/spec.md +++ b/specs/005-mcp-live-data/spec.md @@ -171,9 +171,19 @@ A question is classified once, into one of the destinations available. ### D1 — Rebuild the bundle now, independently of MCP? Recommended: **yes, and first.** Two releases behind is the larger share of the -stated benefit, it needs no new dependency, no subprocess and no router, and it can -ship this week. It also makes the MCP decision honest by removing staleness from its -justification, leaving analysis — which is the real case. +stated benefit, it needs no new dependency, no subprocess and no router. It also +makes the MCP decision honest by removing staleness from its justification, leaving +analysis — which is the real case. + +**Answered 2026-09-14: Release 98 is nearly done, and the plan is to generate new +embeddings and move them to production then.** So D1 is settled and MCP is to be +judged on analysis alone. + +A readiness check against this machine found three things that are not ready and +would each fail partway through a long job: disk (3.4 G bundle, 5.2 G free), S3 +(no credentials here, so `push` cannot reach production), and Neo4j (needed by +`make`, configured nowhere). Written up in +[`deploy/rebuilding-embeddings.md`](../../deploy/rebuilding-embeddings.md). ### D2 — Adopt MCP for analysis?