Repository navigation
Conversation
Add GET /v3/releases/changes and the MCP tool get_release_changes, which diff IDC release N against N-1 (series added/revised/removed, new/updated/ removed collections, analysis results) from index + prior_versions_index. Improve discoverability of version history: get_idc_version description, a whats_new MCP prompt, an INSTRUCTIONS line and idc://guide section, descriptions for prior_versions_index, and notable_columns in list_tables. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7c7c804 to
eb7b1ac
Compare
fedorov
left a comment
There was a problem hiding this comment.
Review: correct for the served release, wrong for past ones
For the served release (v24) the diff matches the issue's numbers. For earlier releases, two problems in the diff SQL cause wrong results. The tests only check the served release, which is the one case where neither problem can happen.
Fix before merge
- Bucket moves reported as revisions (
releases.py:38).prior_versions_indexsplits one content version into two rows when only the storage bucket changes. Of the 2,843 series reported as revised in v20, 2,253 have the samecrdc_series_uuidin v19: they were moved frompublic-datasets-idctoidc-open-data, not changed. - Series that move between collections are misclassified (
releases.py:32). At v5,apollogoes from 43 series to 0 andapollo_5_lsccfrom 0 to 43. The diff reportsapollo_5_lsccas updated (43 revised) and doesn't listapolloat all. - MCP
version: intrejects"v23"(server.py:264). That is the format the tool itself returns inprevious_version. The PR description says"v24"is accepted, but the core's handling of thevprefix can't be reached from either adapter. - The new
prior_versions_indexdocs describe one row per version (schema.py:155/172,_GUIDE, user guide). The data has 2,253 uuids with two rows each, so SQL written from these docs will repeat problem 1.
Also worth fixing: patients_affected isn't scoped to collection. The analysis_results section and the collections section can disagree. "Current version" is read from a different source than get_idc_version. The whats_new prompt normalizes the version in the adapter. isdigit() accepts characters like '²'. The query is recomputed on every call. There are also a few nits. Details are in the inline comments.
All numbers above were checked with DuckDB against idc-index-data 24.2.2.
🤖 Drafted with Claude Code (automated review, posted at my request).
| p.SeriesInstanceUID IS NOT NULL AS in_prev, | ||
| CASE WHEN p.SeriesInstanceUID IS NULL THEN 'added' | ||
| WHEN c.SeriesInstanceUID IS NULL THEN 'removed' | ||
| WHEN c.lo = ? THEN 'revised' END AS change, |
There was a problem hiding this comment.
Bucket moves are reported as content revisions. c.lo = N marks a series as revised whenever its current version row starts at N, but prior_versions_index also starts a new row when only the storage bucket changes. It contains 2,253 crdc_series_uuids with two contiguous rows each (e.g. 19–19 and 20–21).
At v20 this reports series_revised = 2,843. 2,253 of those have the same crdc_series_uuid in v19: they only moved from public-datasets-idc to idc-open-data (mostly cptac_ccrcc, cptac_ucec, cptac_pda and ccdi_mci). The real count is about 590, and those collections are wrongly shown as updated.
Fix: carry crdc_series_uuid in v and use WHEN c.crdc_series_uuid <> p.crdc_series_uuid THEN 'revised', or merge adjacent prior rows with the same uuid first.
| cur AS (SELECT * FROM v WHERE lo <= ? AND hi >= ?), | ||
| prev AS (SELECT * FROM v WHERE lo <= ? AND hi >= ?), | ||
| s AS ( | ||
| SELECT COALESCE(c.collection_id, p.collection_id) AS collection_id, |
There was a problem hiding this comment.
Series that move between collections get attributed to the new collection. COALESCE(c.collection_id, p.collection_id) assigns each joined row to its current collection, so the previous-release membership is credited to the new collection.
v5 example: apollo goes from 43 series to 0 and apollo_5_lscc from 0 to 43. This returns apollo_5_lscc as updated with 43 revised, leaves it out of new_collections, returns removed_collections=[], and has no row for apollo.
Fix: join on (SeriesInstanceUID, collection_id) (a move then counts as removed from one collection and added to the other), or compute collection presence for each release separately.
| COALESCE(sum(cur_mb) FILTER (WHERE change = 'added'), 0) AS added_mb, | ||
| COALESCE(sum(cur_mb) FILTER (WHERE change = 'revised'), 0) AS revised_mb, | ||
| COALESCE(sum(prev_mb) FILTER (WHERE change = 'removed'), 0) AS removed_mb, | ||
| count(DISTINCT PatientID) FILTER (WHERE change IS NOT NULL) AS patients_affected |
There was a problem hiding this comment.
count(DISTINCT PatientID) isn't scoped to collection. PatientIDs are only unique within a collection (80 PatientIDs in the current index appear in more than one), so the top-level total merges different patients and can be less than the sum of the per-collection counts. Suggest count(DISTINCT (collection_id, PatientID)).
| analysis = self.backend.query( | ||
| "SELECT analysis_result_id, " | ||
| "count(*) FILTER (WHERE series_init_idc_version = ?) AS series_added, " | ||
| "min(series_init_idc_version) = ? AS is_new " |
There was a problem hiding this comment.
This section uses series_init_idc_version on current index rows, while the collections section uses version rows. So:
is_new = min(init) = Nignores analysis-result series that were later removed, so a result can be reported as new in a release after the one where it actually first appeared.- An analysis result whose series were only revised in N shows up as revisions in
collectionsbut is missing fromanalysis_results.
Deriving both sections from the same v/cur/prev sets would keep them consistent.
|
|
||
| def release_changes(self, version: int | str | None = None) -> ReleaseChanges: | ||
| dates = self._release_dates() | ||
| current = max(dates) |
There was a problem hiding this comment.
"Current version" comes from max(version_metadata_index.idc_version) here, but DiscoveryService.version() (what get_idc_version returns) derives it from the idc-index-data version. If an idc-index-data release ever ships a version_metadata_index that is out of step with its own version, current_version here will differ from get_idc_version. It is also bound as hi for every index row, so the default call would describe the wrong release. Consider reusing the same helper.
|
|
||
| @mcp.tool() | ||
| @guard | ||
| def get_release_changes(version: int | None = None) -> dict: |
There was a problem hiding this comment.
version: int | None makes pydantic reject "v23", which is the format this tool returns in idc_version / previous_version and the format get_idc_version returns. An agent that passes previous_version back in gets ToolError: Input should be a valid integer, and REST ?version=v23 returns 422. The core's v prefix handling (and its test) can't be reached from either adapter, even though the PR description says "v24" is accepted. Suggest int | str | None here and on the REST query param, and let _parse_version validate.
| @mcp.prompt() | ||
| def whats_new(version: str = "") -> str: | ||
| """Summarize what is new in an IDC data release (the latest one by default).""" | ||
| n = version.strip().lower().removeprefix("v") |
There was a problem hiding this comment.
This repeats the version normalization in the adapter with no validation (against the "adapters are thin" rule in CLAUDE.md), and pastes the raw text into the generated call. whats_new(version="latest") produces get_release_changes(version=latest). Suggest reusing the core _parse_version (made public) and falling back to the no-argument form on invalid input.
| "crdc_series_uuid": "Identifier of this specific version of the series (changes on " | ||
| "every revision); never equal to a crdc_series_uuid in `index`.", | ||
| "min_idc_version": "First IDC release (integer) that served this version of the series.", | ||
| "max_idc_version": "Last IDC release (integer) that served this version of the series; " |
There was a problem hiding this comment.
The data contradicts these descriptions. prior_versions_index is not one row per version: 2,253 crdc_series_uuids have two contiguous rows that differ only by bucket. So "the next release revised or removed it" is wrong for those rows (see the comment on releases.py:38). The same claim is in the table description (line 155), _GUIDE, and docs/user-guide.md. An LLM that follows these docs in run_sql (e.g. max_idc_version = 19 AND SeriesInstanceUID IN index meaning "revised in v20") will count those 2,253 bucket moves as revisions.
Also consider adding crdc_series_uuid to NOTABLE_COLUMNS for this table, since it's the column that tells the two cases apart.
|
|
||
| class CollectionChange(BaseModel): | ||
| collection_id: str | ||
| status: str = Field( |
There was a problem hiding this comment.
Nit: status only ever holds 'new' | 'removed' | 'updated'. Literal["new", "removed", "updated"] would validate it and put the enum in /v3/openapi.json and the MCP output schema.
| ) | ||
|
|
||
|
|
||
| def test_latest_release_is_default_and_self_consistent(ctx): |
There was a problem hiding this comment.
These invariants only run against the served release, which is the one case where neither the split-row problem nor the collection-move problem can happen, so both pass the suite. Suggest adding past-release checks, e.g.:
- every series counted as
revisedin N has a differentcrdc_series_uuidin N-1 (catches the v20 bucket moves); release_changes(5)listsapollo_5_lsccinnew_collectionsandapolloinremoved_collections.
Closes #40.
Summary
The served release already carries the full history of earlier releases, but nothing on the tool surface pointed at it. This PR turns that into a first-class capability and fixes the discoverability gaps the issue diagnosed.
GET /v3/releases/changes?version=N/ MCPget_release_changes(version)— diff of release N vs N-1: series added / revised / removed (TB, patients affected), collections new / updated / removed, analysis results that gained series. Defaults to the served release; accepts24or"v24".index(valid fromseries_revised_idc_version) or a row inprior_versions_index(validmin_idc_version–max_idc_version). Comparing which versions span N vs N-1 gives an exact diff, including removed series, which the issue's query can't see.get_idc_versiondescription points at release history; one-lineINSTRUCTIONStouch + "Release history" section inidc://guide;whats_newMCP prompt;prior_versions_indextable/column descriptions (fill-ins only where upstream is empty);notable_columnsonlist_tablesentries.Validation
For v24 the service reproduces the issue's numbers exactly (39,872 added, 5.66 TB, 15 new collections, 7 revised in
acrin_nsclc_fdg_pet/anti_pd_1_lung) and additionally reports 1,034 series removed frombonemarrowwsi_pediatricleukemia. Query runs in ~0.08s.New
tests/test_releases.py: self-consistency invariants (totals = per-collection sums; added + revised = series withseries_revised_idc_version = current), v1 has no predecessor, bad versions are clean errors (400 on REST), core == REST == MCP parity, and discoverability checks (tool description vocabulary, prompt, schema docs, notable columns exist). Full suite: 109 passed.Known limitation
analysis_resultsfor past releases counts only series still present today (prior_versions_indexhas noanalysis_result_id); documented in the field description.Docs:
docs/user-guide.md(new Release history section, endpoint/tool tables),CHANGELOG.mdunder[Unreleased].🤖 Generated with Claude Code