From 53830ae3ce3630183c6c07bbb4046315d452db09 Mon Sep 17 00:00:00 2001 From: Andrey Fedorov Date: Mon, 19 Oct 2026 16:56:55 -0400 Subject: [PATCH 1/3] feat(api): cohort filters can exclude or isolate analysis results Analysis result series carry the collection_id of the images they analyze, so a collection_id filter returns both, and no term value selects original data: its analysis_result_id is NULL. Through the cohort surfaces there was no way to drop the derived series (two thirds of NLST) short of rewriting the query in SQL and losing the manifest. CohortFilters gains analysis_results ('include' / 'exclude' / 'only'), named and valued as in idc-index's download_from_selection so the two tools read the same. It compiles to analysis_result_id IS [NOT] NULL on top of terms and ranges. It is not a predicate on its own for require_filter, since either side is about half of IDC, and 'exclude' with an analysis_result_id term is refused instead of returning a zero that reads as "no such data". For a collection-only filter the download payload switches from `idc download `, which would fetch the analysis results too, to `idc download-from-selection --analysis-results`. The issue preferred a source_type term. A mode field matches idc-index, needs no synthetic column, and keeps the choice visible in filters_applied. get_attribute_values also returns null_count. Previously it listed the analysis_result_id values with truncated=false and nothing to say that over half of index had no value at all. The analysis_result_id attribute note points from the term filter to analysis_results. Closes #43 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 ++ docs/user-guide.md | 26 ++++++ src/idc_api/core/filters.py | 22 ++++- src/idc_api/core/models.py | 24 ++++- src/idc_api/core/schema.py | 11 ++- src/idc_api/core/services/discovery.py | 6 ++ src/idc_api/core/services/manifest.py | 10 +- src/idc_api/mcp/server.py | 59 ++++++++---- src/idc_api/rest/app.py | 22 +++-- tests/test_analysis_results_filter.py | 121 +++++++++++++++++++++++++ tests/test_filter_shape.py | 2 +- 11 files changed, 281 insertions(+), 30 deletions(-) create mode 100644 tests/test_analysis_results_filter.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 7beb6c3..c929aac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,14 @@ Refactors, CI, and formatting land in the git history, not here. statement through DuckDB's prepared-statement mechanism. A caller no longer has to quote or escape a value it did not author — an identifier taken from a URL, a name a user typed — and such a value can no longer change the shape of the query. +- Cohort filters take `analysis_results`: `include` (default), `exclude` (original data only) + or `only` (analysis results only), as in idc-index's `download_from_selection`. It applies to + `cohort/counts`, `cohort/manifest`, `cohort/manifest.txt`, `citations`, `licenses` and the + matching MCP tools, and is echoed in `filters_applied`. A collection filter returns that + collection's analysis results too, and until now there was no way to drop them short of SQL + ([#43](https://github.com/ImagingDataCommons/IDC-REST-MCP/issues/43)). +- `get_attribute_values` / `GET /v3/attributes/{attribute}/values` return `null_count`, the + number of series with no value, which `values` does not list. ### Changed diff --git a/docs/user-guide.md b/docs/user-guide.md index 77eccc3..a0a686a 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -35,6 +35,12 @@ of an analysis result's series (and vice versa). Filter on whichever axis you ac (`analysis_results_index` lists each result's source collections in its plural `collections` field.) +An analysis result's series carry the `collection_id` of the images they analyze, so a +`collection_id` filter returns the collection's original images *and* any analysis results on +them — in NLST, about two thirds of the series are analysis results (IDC v25). Original data has no +`analysis_result_id` (it is NULL), so set the cohort filter's `analysis_results` to `exclude` for +original data only, or `only` for analysis results only. + The main queryable table is **`index`** — one row per **series**, the unit you filter, count, and download. IDC is large (~100+ TB total), so always check counts/size before downloading. @@ -298,6 +304,9 @@ request/response example for each. curl -s 'localhost:8000/v3/attributes/Modality/values?limit=10' ``` +Series with no value for the attribute are not listed; `null_count` says how many there are. +For `analysis_result_id` that is all of the original data. + **Other read-only lookups (GET)** — no body, just the URL: ```bash @@ -337,6 +346,18 @@ must be numbers; bounds on `StudyDate` / `SeriesDate` must be dates as `"YYYY-MM `"YYYYMMDD"` is accepted and normalized). Any other bound is rejected with a 400 rather than silently matching nothing. +`analysis_results` keeps or drops the series that belong to analysis results: `include` +(default), `exclude` (original data only) or `only` (analysis results only) — the same option +`idc download-from-selection` takes. It applies on top of `terms` and `ranges` but is not a +predicate on its own, so a manifest still needs at least one of those; combining `exclude` with an +`analysis_result_id` term is a 400. NLST's original images only: + +```bash +curl -s localhost:8000/v3/cohort/counts \ + -H 'content-type: application/json' \ + -d '{"filters": {"terms": {"collection_id": ["nlst"]}, "analysis_results": "exclude"}}' +``` + **Get the full manifest as plain text** (for `idc download-from-manifest` / `s5cmd`): ```bash @@ -471,6 +492,11 @@ do it: ```bash idc download nlst --download-dir ./idc-data ``` + That fetches the collection's analysis results too. With `analysis_results` set to + `exclude` or `only`, the payload emits the split form instead: + ```bash + idc download-from-selection --collection-id nlst --analysis-results exclude --download-dir ./idc-data + ``` 2. **A manifest** — save the `manifest.txt` output (or any SQL result's `series_aws_url` column) and feed it to the CLI: ```bash diff --git a/src/idc_api/core/filters.py b/src/idc_api/core/filters.py index 424bbd5..e85760e 100644 --- a/src/idc_api/core/filters.py +++ b/src/idc_api/core/filters.py @@ -99,13 +99,26 @@ def compile_filters(filters: CohortFilters) -> CompiledFilters: params.append(rng.lte) applied_ranges[attr] = rng + mode = filters.analysis_results + if mode == "exclude" and "analysis_result_id" in applied_terms: + # Always empty. Refused rather than answered with a zero that reads as "no such data". + raise InvalidQueryError( + "analysis_results='exclude' keeps original data only, so it cannot be combined with " + "an analysis_result_id term, which selects analysis results. Drop one of the two." + ) + # Analysis result series are the ones with an analysis_result_id; original data has NULL. + if mode == "exclude": + clauses.append('"analysis_result_id" IS NULL') + elif mode == "only": + clauses.append('"analysis_result_id" IS NOT NULL') + if not clauses: warnings.append(UNFILTERED_WARNING) return CompiledFilters( where=" AND ".join(clauses) if clauses else "TRUE", params=params, - applied=CohortFilters(terms=applied_terms, ranges=applied_ranges), + applied=CohortFilters(terms=applied_terms, ranges=applied_ranges, analysis_results=mode), warnings=warnings, ) @@ -164,10 +177,17 @@ def require_filter(compiled: CompiledFilters, action: str) -> None: not: an unfiltered manifest is a download payload for the whole archive, which no caller means to ask for. Those raise instead, with the dropped-predicate warnings attached so the caller can see *why* their filter came out empty. + + ``analysis_results`` alone does not count: either side of that split is roughly half the + archive (about half a million series). """ if compiled.applied.terms or compiled.applied.ranges: return dropped = [w for w in compiled.warnings if w != UNFILTERED_WARNING] + if compiled.applied.analysis_results != "include": + dropped.append( + "analysis_results on its own is not a predicate: either side of it is about half of IDC." + ) raise InvalidQueryError( f"At least one filter predicate is required to {action}: unfiltered, that is every series " "in IDC (100+ TB). Use the stats surface for archive-wide totals, or cohort counts to " diff --git a/src/idc_api/core/models.py b/src/idc_api/core/models.py index 0400239..d475b26 100644 --- a/src/idc_api/core/models.py +++ b/src/idc_api/core/models.py @@ -2,10 +2,13 @@ from __future__ import annotations -from typing import Any +from typing import Any, Literal from pydantic import BaseModel, ConfigDict, Field +# Same name and values as idc-index's download_from_selection(analysis_results=...). +AnalysisResultsMode = Literal["include", "exclude", "only"] + # --- discovery ---------------------------------------------------------------------------- @@ -95,6 +98,11 @@ class AttributeValues(BaseModel): attribute: str values: list[AttributeValue] truncated: bool = False + null_count: int = Field( + 0, + description="Series with no value (NULL) for this attribute. They are not listed in " + "`values`, and a term filter cannot select them.", + ) note: str = Field("", description="Semantic caveat about this attribute, when one applies.") @@ -158,8 +166,9 @@ class NumericRange(BaseModel): class CohortFilters(BaseModel): """Structured filters over the main `index` table. ``terms`` does equality/IN (OR within - an attribute, AND across attributes); ``ranges`` does gte/lte. Discover valid attribute - names with ``list_attributes`` and valid values with ``get_attribute_values``.""" + an attribute, AND across attributes); ``ranges`` does gte/lte; ``analysis_results`` keeps + or drops the series that belong to analysis results. Discover valid attribute names with + ``list_attributes`` and valid values with ``get_attribute_values``.""" model_config = ConfigDict( # An unrecognized key is a hard error, never silently ignored. Ignoring one meant a @@ -182,6 +191,15 @@ class CohortFilters(BaseModel): examples=[{"collection_id": ["nlst"], "Modality": ["CT"]}], ) ranges: dict[str, NumericRange] = Field(default_factory=dict) + analysis_results: AnalysisResultsMode = Field( + "include", + description="Whether series that belong to analysis results (derived data: " + "segmentations, annotations, radiomics) are in the cohort: 'include' keeps them " + "(default), 'exclude' keeps original data only, 'only' keeps analysis results only. " + "Analysis result series carry the collection_id of the images they analyze, so a " + "collection_id filter alone returns both. Applied on top of terms and ranges; not a " + "predicate on its own, so it does not satisfy the at-least-one-predicate rule.", + ) # Shared by every response built from a filter, so the same two field names mean the same thing diff --git a/src/idc_api/core/schema.py b/src/idc_api/core/schema.py index 293e88a..ffab3f2 100644 --- a/src/idc_api/core/schema.py +++ b/src/idc_api/core/schema.py @@ -241,7 +241,16 @@ def table_schema(table: str) -> dict: FILTERABLE_ATTRIBUTES: list[dict] = [ {"name": "collection_id", "kind": "term", "categorical": True}, - {"name": "analysis_result_id", "kind": "term", "categorical": True}, + { + "name": "analysis_result_id", + "kind": "term", + "categorical": True, + "note": ( + "A term filter can only select analysis results by id. To keep original data only " + "(analysis_result_id NULL), or analysis results only, set the cohort filter's " + "analysis_results to 'exclude' or 'only'." + ), + }, {"name": "PatientID", "kind": "term", "categorical": False}, {"name": "StudyInstanceUID", "kind": "term", "categorical": False}, {"name": "SeriesInstanceUID", "kind": "term", "categorical": False}, diff --git a/src/idc_api/core/services/discovery.py b/src/idc_api/core/services/discovery.py index 9d961f1..283e958 100644 --- a/src/idc_api/core/services/discovery.py +++ b/src/idc_api/core/services/discovery.py @@ -154,9 +154,15 @@ def get_attribute_values(self, attribute: str, limit: int = 100) -> AttributeVal ) truncated = len(res.rows) > limit rows = res.rows[:limit] + # The values above leave out NULLs; count them so that a mostly-NULL column (e.g. + # analysis_result_id, NULL for all original data) doesn't read as fully enumerated. + null_count = self.backend.query( + f'SELECT count(*) AS n FROM index WHERE "{attribute}" IS NULL' # nosec B608 + ).rows[0]["n"] return AttributeValues( attribute=attribute, values=[AttributeValue(value=r["value"], count=r["count"]) for r in rows], truncated=truncated, + null_count=null_count, note=schema.attribute_note(attribute), ) diff --git a/src/idc_api/core/services/manifest.py b/src/idc_api/core/services/manifest.py index afaa96e..5e75b37 100644 --- a/src/idc_api/core/services/manifest.py +++ b/src/idc_api/core/services/manifest.py @@ -81,9 +81,17 @@ def download_info( and not (filters.ranges or {}) and len(terms["collection_id"]) >= 1 ) + mode = filters.analysis_results if only_collection: for cid in terms["collection_id"]: - commands.append(f"idc download {cid} --download-dir ./idc-data") + if mode == "include": + commands.append(f"idc download {cid} --download-dir ./idc-data") + else: + # `idc download` takes everything in the collection; this one can split it. + commands.append( + f"idc download-from-selection --collection-id {cid} " + f"--analysis-results {mode} --download-dir ./idc-data" + ) commands.append( "# Or save the full manifest (see the manifest endpoint/tool), then:\n" "idc download-from-manifest idc_manifest.txt --download-dir ./idc-data" diff --git a/src/idc_api/mcp/server.py b/src/idc_api/mcp/server.py index 4f4c263..f163d4f 100644 --- a/src/idc_api/mcp/server.py +++ b/src/idc_api/mcp/server.py @@ -36,7 +36,7 @@ from ..core import version as core_version from ..core.context import AppContext from ..core.errors import IDCAPIError, InvalidQueryError -from ..core.models import CohortFilters, NumericRange +from ..core.models import AnalysisResultsMode, CohortFilters, NumericRange from ..http_headers import HSTSMiddleware from ..settings import get_settings @@ -206,7 +206,9 @@ def wrapper(*args, **kwargs): return wrapper -def _filters(terms: dict | None, ranges: dict | None) -> CohortFilters: +def _filters( + terms: dict | None, ranges: dict | None, analysis_results: str = "include" +) -> CohortFilters: """Build the core filter object from loose tool arguments. The models reject unknown keys (a silently-dropped predicate would return all of IDC as if @@ -218,11 +220,13 @@ def _filters(terms: dict | None, ranges: dict | None) -> CohortFilters: return CohortFilters( terms=terms or {}, ranges={k: NumericRange(**v) for k, v in (ranges or {}).items()}, + analysis_results=analysis_results, ) except ValidationError as exc: raise InvalidQueryError( "Malformed filter arguments: terms is {attribute: [values]} (e.g. " - '{"Modality": ["MR"]}) and ranges is {attribute: {"gte": x, "lte": y}}. ' + '{"Modality": ["MR"]}), ranges is {attribute: {"gte": x, "lte": y}}, and ' + "analysis_results is 'include', 'exclude' or 'only'. " f"Details: {exc.error_count()} validation error(s): " + "; ".join(f"{'.'.join(str(p) for p in e['loc'])}: {e['msg']}" for e in exc.errors()) ) from None @@ -298,7 +302,8 @@ def get_attribute_values(attribute: str, limit: int = 50) -> dict: Keep `limit` small while exploring; the result carries a `truncated` flag — if true you did NOT get every distinct value, so raise `limit` and re-check. `limit` is capped server-side (so a large value returns all values for any realistic categorical attribute); a - `truncated=false` response means you have the complete list.""" + `truncated=false` response means you have the complete list. Series with no value are not + listed; `null_count` says how many there are.""" return ctx.discovery.get_attribute_values(attribute, limit=limit).model_dump(mode="json") @@ -370,6 +375,7 @@ def get_clinical_table(table: str, max_rows: int = 100) -> dict: def build_cohort( terms: dict | None = None, ranges: dict | None = None, + analysis_results: AnalysisResultsMode = "include", page: int = 0, page_size: int = 5, ) -> dict: @@ -383,10 +389,16 @@ def build_cohort( SeriesDate. Discover valid attributes with list_attributes and valid values with get_attribute_values. For anything these structured filters can't express, use run_sql. + `analysis_results` decides whether series from analysis results (derived segmentations, + annotations, radiomics) are in the cohort: 'include' (default), 'exclude' (original data + only) or 'only'. Derived series carry the collection_id of the images they analyze, so a + collection_id filter alone returns both — set 'exclude' when the user wants a collection's + original images. + At least one filter predicate is required — an unfiltered cohort is the whole 100+ TB archive; use get_stats for archive-wide totals. The result echoes `counts.filters_applied` and `counts.warnings`: check them rather than assuming your filter landed.""" - f = _filters(terms, ranges) + f = _filters(terms, ranges, analysis_results) return ctx.cohort.build_manifest(f, page=page, page_size=page_size).model_dump(mode="json") @@ -428,6 +440,7 @@ def run_sql( def get_cohort_urls( terms: dict | None = None, ranges: dict | None = None, + analysis_results: AnalysisResultsMode = "include", source: str = "aws", limit: int = 100, ) -> dict: @@ -437,9 +450,10 @@ def get_cohort_urls( ever expects s3:// lines). Returns up to `limit` URLs (increase for full manifests). These are anonymous public URLs — easiest is the `idc` CLI (handles either cloud); driving it yourself, `s5cmd --no-sign-request` works directly for source=aws, and for source=gcs add - `--endpoint-url https://storage.googleapis.com`. At least one filter predicate is required: - unfiltered, this would enumerate every series in IDC.""" - f = _filters(terms, ranges) + `--endpoint-url https://storage.googleapis.com`. `terms`, `ranges` and `analysis_results` + filter as in build_cohort. At least one filter predicate is required: unfiltered, this would + enumerate every series in IDC.""" + f = _filters(terms, ranges, analysis_results) urls, truncated = ctx.manifest.manifest_lines(f, source=source, limit=limit) return { "source": source, @@ -472,24 +486,30 @@ def get_viewer_url( def get_citations( terms: dict | None = None, ranges: dict | None = None, + analysis_results: AnalysisResultsMode = "include", citation_format: str = "apa", ) -> dict: """Get the publications to cite for a cohort: per-dataset citations (from the cohort's source DOIs) in `citations`, plus the IDC paper in `idc_acknowledgment`. `citation_format` is one of: apa, bibtex, csl-json, turtle. When the user publishes results using IDC data, always include the per-dataset citations AND acknowledge IDC itself via `idc_acknowledgment` - (see the `recommendation` field).""" - f = _filters(terms, ranges) + (see the `recommendation` field). `terms`, `ranges` and `analysis_results` filter as in + build_cohort.""" + f = _filters(terms, ranges, analysis_results) return ctx.citations.get_citations(f, citation_format=citation_format).model_dump(mode="json") @mcp.tool() @guard -def get_licenses(terms: dict | None = None, ranges: dict | None = None) -> dict: +def get_licenses( + terms: dict | None = None, + ranges: dict | None = None, + analysis_results: AnalysisResultsMode = "include", +) -> dict: """Get the license breakdown (series count + size per license) for a cohort. Use to check whether data is commercial-friendly (CC BY) or non-commercial only (CC BY-NC) before the - user reuses it.""" - f = _filters(terms, ranges) + user reuses it. `terms`, `ranges` and `analysis_results` filter as in build_cohort.""" + f = _filters(terms, ranges, analysis_results) return ctx.licenses.get_licenses(f).model_dump(mode="json") @@ -502,8 +522,10 @@ def get_licenses(terms: dict | None = None, ranges: dict | None = None) -> dict: Patient → Study → Series, with two *independent* grouping labels: `collection_id` (the source dataset, e.g. `nlst`, `tcga_luad`; a patient is in exactly one) and `analysis_result_id` (a derived dataset — segmentations/annotations — that can span *multiple* collections, so it is -not nested under one). The main table is `index` (one row per *series*). IDC is large -(~100+ TB) — always check size before suggesting a download. +not nested under one). An analysis result's series carry the `collection_id` of the images they +analyze, so a `collection_id` filter returns original data *and* analysis results; original +data has a NULL `analysis_result_id`. The main table is `index` (one row per *series*). IDC is +large (~100+ TB) — always check size before suggesting a download. **The tools form a few families that build on each other:** - *Discovery* (`get_stats`, `list_collections`, `get_collection`, `list_analysis_results`, @@ -527,10 +549,15 @@ def get_licenses(terms: dict | None = None, ranges: dict | None = None) -> dict: 1. *Find data:* `list_collections` / `get_collection` (imaging datasets), `list_analysis_results` (derived annotations & segmentations). 2. *Ground filters (do this first to avoid wrong values):* `list_attributes` → valid attributes; - `get_attribute_values(attribute=...)` → valid values + counts (correct casing!). If the + `get_attribute_values(attribute=...)` → valid values + counts (correct casing!), plus + `null_count` for series with no value, which a term filter cannot select. If the property you need is not there (e.g. what anatomy a segmentation contains), it likely lives in a specialized index — see *Tables for run_sql* below. 3. *Build:* `build_cohort(terms={...}, ranges={...})` → counts, sample series, download payload. + `analysis_results='exclude'` keeps original data only (e.g. a collection's own images without + the segmentations layered on them), `'only'` keeps analysis results only; the same argument + works on `get_cohort_urls`, `get_citations` and `get_licenses`. It is not a predicate on its + own, and `'exclude'` with an `analysis_result_id` term is refused. *How to tell your filter was applied:* every filtered result echoes `filters_applied` (the predicates actually used) plus a `warnings` list. Empty `filters_applied` means nothing was filtered and the counts describe the ENTIRE archive — never report that as a cohort. A diff --git a/src/idc_api/rest/app.py b/src/idc_api/rest/app.py index c9495e5..fd8f329 100644 --- a/src/idc_api/rest/app.py +++ b/src/idc_api/rest/app.py @@ -80,7 +80,7 @@ class _FilterRequest(BaseModel): @classmethod def _reject_bare_filter(cls, data: Any) -> Any: if isinstance(data, dict): - stray = [k for k in ("terms", "ranges") if k in data] + stray = [k for k in ("terms", "ranges", "analysis_results") if k in data] if stray: raise ValueError( "the filter object goes under `filters`, e.g. " @@ -176,7 +176,10 @@ def _filter_errors(*, requires_filter: bool = False) -> dict: class CountsRequest(_FilterRequest): model_config = ConfigDict( json_schema_extra={ - "examples": [{"filters": {"terms": {"collection_id": ["nlst"], "Modality": ["CT"]}}}] + "examples": [ + {"filters": {"terms": {"collection_id": ["nlst"], "Modality": ["CT"]}}}, + {"filters": {"terms": {"collection_id": ["nlst"]}, "analysis_results": "exclude"}}, + ] } ) @@ -500,7 +503,8 @@ def attribute_values( table, e.g. `Modality` or `BodyPartExamined`. Query this before filtering by an attribute so you use real values with the correct casing rather than guessing. The response carries a `truncated` flag: when `false` the list is complete; when `true`, raise `limit` (capped - server-side) and re-check.""" + server-side) and re-check. Series with no value are not listed; `null_count` says how many + there are.""" return C().discovery.get_attribute_values(attribute, limit=limit) # --- schema discovery --- @@ -590,7 +594,10 @@ def cohort_counts(req: CountsRequest): and total `size_TB` — without the sample rows or download payload. Use it as a fast size check before building a full manifest or downloading. `filters.terms` is `{attribute: [values]}` for equality/IN; `filters.ranges` is - `{attribute: {"gte": x, "lte": y}}` for numeric or date ranges. + `{attribute: {"gte": x, "lte": y}}` for numeric or date ranges; `filters.analysis_results` + is `include` (default), `exclude` (original data only) or `only` (analysis results only). + Analysis result series carry the `collection_id` of the images they analyze, so a + `collection_id` filter alone returns both. The response echoes `filters_applied` (the predicates actually used) and `warnings`. **Check them rather than assuming your filter landed**: an empty `filters_applied` means @@ -610,9 +617,10 @@ def cohort_manifest(req: ManifestRequest): studies, series, instances, size_TB), a page of matching series, and a download payload (idc commands plus a manifest preview). `filters.terms` is `{attribute: [values]}` for equality/IN (e.g. `{"Modality": ["MR"]}`); `filters.ranges` is - `{attribute: {"gte": x, "lte": y}}` for numeric or date ranges. Discover valid attributes - via `/attributes` and valid values via `/attributes/{attribute}/values`. For anything - these structured filters can't express, use `/sql`. + `{attribute: {"gte": x, "lte": y}}` for numeric or date ranges; `filters.analysis_results` + set to `exclude` keeps original data only, `only` keeps analysis results only. Discover + valid attributes via `/attributes` and valid values via `/attributes/{attribute}/values`. + For anything these structured filters can't express, use `/sql`. **At least one filter predicate is required** — unfiltered, this is a download payload for the entire archive; use `/stats` for archive-wide totals. `counts.filters_applied` echoes diff --git a/tests/test_analysis_results_filter.py b/tests/test_analysis_results_filter.py new file mode 100644 index 0000000..a8f96c4 --- /dev/null +++ b/tests/test_analysis_results_filter.py @@ -0,0 +1,121 @@ +"""Cohort filters can keep or drop analysis results (#43). + +Analysis result series carry the collection_id of the images they analyze, so a collection_id +filter alone returns both, and no term value means "no analysis result": original data has a NULL +analysis_result_id. ``analysis_results`` ('include' / 'exclude' / 'only', as in idc-index's +download_from_selection) is the cohort-level switch, and ``get_attribute_values`` reports the +NULLs it cannot list as values. +""" + +from __future__ import annotations + +import pytest +from mcp.server.fastmcp.exceptions import ToolError + +from idc_api.core.models import CohortFilters +from idc_api.mcp.server import mcp + +# rider_pilot has original series and a qiba_volct_1b analysis result, and is small. +_TERMS = {"collection_id": ["rider_pilot"]} + + +def _series(ctx, mode: str) -> int: + return ctx.cohort.counts(CohortFilters(terms=_TERMS, analysis_results=mode)).series + + +def test_exclude_and_only_partition_the_cohort(ctx): + include, exclude, only = (_series(ctx, m) for m in ("include", "exclude", "only")) + assert exclude > 0 and only > 0 + assert include == exclude + only + + original = ctx.query.run_sql( + "SELECT count(*) AS n FROM index WHERE collection_id = ? AND analysis_result_id IS NULL", + ["rider_pilot"], + ).rows[0]["n"] + assert exclude == original + + +async def test_rest_and_mcp_apply_and_echo_it(ctx, client, parse_mcp): + expected = _series(ctx, "exclude") + + rest = client.post( + "/v3/cohort/counts", json={"filters": {"terms": _TERMS, "analysis_results": "exclude"}} + ).json() + mcp_out = parse_mcp( + await mcp.call_tool("build_cohort", {"terms": _TERMS, "analysis_results": "exclude"}) + ) + assert rest["series"] == mcp_out["total_series"] == expected + assert rest["filters_applied"]["analysis_results"] == "exclude" + assert mcp_out["counts"]["filters_applied"]["analysis_results"] == "exclude" + + urls = client.post( + "/v3/cohort/manifest.txt", + json={"filters": {"terms": _TERMS, "analysis_results": "exclude"}, "limit": 10_000}, + ).text.split() + assert len(urls) == expected + + +def test_download_command_carries_the_split(client): + def commands(mode): + body = {"filters": {"terms": _TERMS, "analysis_results": mode}, "include_rows": False} + return client.post("/v3/cohort/manifest", json=body).json()["download"]["idc_commands"] + + assert "idc download rider_pilot --download-dir ./idc-data" in commands("include") + # `idc download ` would fetch the analysis results too. + assert ( + "idc download-from-selection --collection-id rider_pilot --analysis-results exclude " + "--download-dir ./idc-data" + ) in commands("exclude") + + +async def test_exclude_with_an_analysis_result_id_is_refused(client): + """Always empty; a zero would read as "no such data".""" + filters = {"terms": {"analysis_result_id": ["qiba_volct_1b"]}, "analysis_results": "exclude"} + r = client.post("/v3/cohort/counts", json={"filters": filters}) + assert r.status_code == 400 + assert "cannot be combined" in r.json()["error"]["message"] + with pytest.raises(ToolError, match="cannot be combined"): + await mcp.call_tool("build_cohort", filters) + + +async def test_it_is_not_a_predicate_on_its_own(client): + """Either side of the split is about half of IDC, so it does not unlock enumeration.""" + r = client.post("/v3/cohort/manifest", json={"filters": {"analysis_results": "only"}}) + assert r.status_code == 400 + assert "not a predicate" in r.json()["error"]["message"] + with pytest.raises(ToolError, match="not a predicate"): + await mcp.call_tool("get_cohort_urls", {"analysis_results": "exclude"}) + + +async def test_unknown_mode_is_rejected(client): + r = client.post( + "/v3/cohort/counts", json={"filters": {"terms": _TERMS, "analysis_results": "none"}} + ) + assert r.status_code == 422 + with pytest.raises(ToolError): + await mcp.call_tool("build_cohort", {"terms": _TERMS, "analysis_results": "none"}) + + +def test_top_level_analysis_results_names_the_fix(client): + r = client.post("/v3/cohort/counts", json={"analysis_results": "exclude"}) + assert r.status_code == 422 + assert "goes under `filters`" in r.text + + +async def test_attribute_values_report_the_nulls(ctx, client, parse_mcp): + """Before, the original data (over half of `index`) was simply absent from the response.""" + total = ctx.discovery.stats().series + full = ctx.discovery.get_attribute_values("analysis_result_id", limit=10_000) + assert not full.truncated + assert full.null_count > 0 + assert full.null_count + sum(v.count for v in full.values) == total + assert "analysis_results" in full.note + + rest = client.get("/v3/attributes/analysis_result_id/values?limit=3").json() + mcp_out = parse_mcp( + await mcp.call_tool("get_attribute_values", {"attribute": "analysis_result_id", "limit": 3}) + ) + assert rest["null_count"] == mcp_out["null_count"] == full.null_count + + # A column with no NULLs says so. + assert ctx.discovery.get_attribute_values("collection_id", limit=1).null_count == 0 diff --git a/tests/test_filter_shape.py b/tests/test_filter_shape.py index 07e744c..2f6f890 100644 --- a/tests/test_filter_shape.py +++ b/tests/test_filter_shape.py @@ -128,7 +128,7 @@ def test_empty_filter_says_it_covers_everything(client): no longer be mistaken for a cohort.""" r = client.post("/v3/cohort/counts", json={"filters": {}}).json() assert r["series"] > _ALL_OF_IDC - assert r["filters_applied"] == {"terms": {}, "ranges": {}} + assert r["filters_applied"] == {"terms": {}, "ranges": {}, "analysis_results": "include"} assert any("ENTIRE IDC archive" in w for w in r["warnings"]) From e1b4457b26159bf3aee0b4dcccfdb0d175e1cc08 Mon Sep 17 00:00:00 2001 From: Andrey Fedorov Date: Mon, 19 Oct 2026 17:01:50 -0400 Subject: [PATCH 2/3] feat(api): one-line idc command for a whole analysis result The download payload offered `idc download ` for a filter of only collection_id values. A filter of only analysis_result_id values fell through to the manifest route alone, which also opened with "Or". `idc download ` resolves analysis_result_id too (idc-index 0.13.2+), so emit it for that case as well, and word the manifest line "Save" when it is the only option. The shape is now read from the applied filter rather than the request, so a predicate the compiler dropped can neither hide nor fake a whole-collection selection. Values are shlex-quoted: they come from the caller and land in a shell command the user may paste. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 ++ docs/user-guide.md | 7 ++++-- src/idc_api/core/services/manifest.py | 33 +++++++++++++++------------ tests/test_analysis_results_filter.py | 22 ++++++++++++++++++ 4 files changed, 47 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c929aac..8fa8994 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ Refactors, CI, and formatting land in the git history, not here. matching MCP tools, and is echoed in `filters_applied`. A collection filter returns that collection's analysis results too, and until now there was no way to drop them short of SQL ([#43](https://github.com/ImagingDataCommons/IDC-REST-MCP/issues/43)). +- The cohort download payload includes `idc download ` when the filter is only + `analysis_result_id` values, as it already did for `collection_id`. - `get_attribute_values` / `GET /v3/attributes/{attribute}/values` return `null_count`, the number of series with no value, which `values` does not list. diff --git a/docs/user-guide.md b/docs/user-guide.md index a0a686a..3cc4b5c 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -487,10 +487,13 @@ the API server (the server never moves bytes; retrieval always means URLs/manife the same against the hosted or a local instance, and scales to whole collections. Two ways to do it: -1. **Whole collection** — the simplest path; `cohort/manifest`'s download payload emits it for - you when your filter is a single `collection_id`: +1. **Whole collection or analysis result** — the simplest path; `cohort/manifest`'s download + payload emits it for you when your filter is only `collection_id` values, or only + `analysis_result_id` values (an analysis result is downloaded across all the collections it + spans): ```bash idc download nlst --download-dir ./idc-data + idc download nlstseg --download-dir ./idc-data ``` That fetches the collection's analysis results too. With `analysis_results` set to `exclude` or `only`, the payload emits the split form instead: diff --git a/src/idc_api/core/services/manifest.py b/src/idc_api/core/services/manifest.py index 5e75b37..7de89d8 100644 --- a/src/idc_api/core/services/manifest.py +++ b/src/idc_api/core/services/manifest.py @@ -4,6 +4,8 @@ from __future__ import annotations +import shlex + from ..backend.base import QueryBackend from ..filters import compile_filters, require_filter from ..models import CohortFilters, DownloadInfo @@ -74,26 +76,27 @@ def download_info( truncated = total_series > self.settings.manifest_hard_cap commands: list[str] = [] - # Whole-collection selection -> the simplest idc command. - terms = filters.terms or {} - only_collection = ( - list(terms.keys()) == ["collection_id"] - and not (filters.ranges or {}) - and len(terms["collection_id"]) >= 1 - ) - mode = filters.analysis_results - if only_collection: - for cid in terms["collection_id"]: - if mode == "include": - commands.append(f"idc download {cid} --download-dir ./idc-data") - else: + # A selection that is whole collections, or whole analysis results, has a one-line idc + # command (`idc download ` resolves either kind of id). Read from what was + # applied, so a predicate the compiler dropped doesn't hide or fake that shape. + terms = f.applied.terms + mode = f.applied.analysis_results + whole = next(iter(terms)) if len(terms) == 1 and not f.applied.ranges else None + if whole in ("collection_id", "analysis_result_id"): + for value in terms[whole]: + # Values come from the caller and land in a shell command; quote them. + value = shlex.quote(value) + if whole == "collection_id" and mode != "include": # `idc download` takes everything in the collection; this one can split it. commands.append( - f"idc download-from-selection --collection-id {cid} " + f"idc download-from-selection --collection-id {value} " f"--analysis-results {mode} --download-dir ./idc-data" ) + else: + commands.append(f"idc download {value} --download-dir ./idc-data") commands.append( - "# Or save the full manifest (see the manifest endpoint/tool), then:\n" + ("# Or save" if commands else "# Save") + + " the full manifest (see the manifest endpoint/tool), then:\n" "idc download-from-manifest idc_manifest.txt --download-dir ./idc-data" ) diff --git a/tests/test_analysis_results_filter.py b/tests/test_analysis_results_filter.py index a8f96c4..b983c57 100644 --- a/tests/test_analysis_results_filter.py +++ b/tests/test_analysis_results_filter.py @@ -68,6 +68,28 @@ def commands(mode): ) in commands("exclude") +def test_whole_analysis_result_has_a_one_line_command(client): + """`idc download ` resolves analysis_result_id as well as collection_id.""" + + def commands(terms): + body = {"filters": {"terms": terms}, "include_rows": False} + return client.post("/v3/cohort/manifest", json=body).json()["download"]["idc_commands"] + + cmds = commands({"analysis_result_id": ["qiba_volct_1b"]}) + assert cmds[0] == "idc download qiba_volct_1b --download-dir ./idc-data" + assert cmds[-1].startswith("# Or save the full manifest") + + # Narrowed to one collection it is no longer the whole analysis result: manifest only. + cmds = commands({"analysis_result_id": ["qiba_volct_1b"], "collection_id": ["rider_pilot"]}) + assert len(cmds) == 1 and cmds[0].startswith("# Save the full manifest") + + +def test_values_in_download_commands_are_shell_quoted(client): + body = {"filters": {"terms": {"collection_id": ["x; rm -rf ~"]}}, "include_rows": False} + cmds = client.post("/v3/cohort/manifest", json=body).json()["download"]["idc_commands"] + assert cmds[0] == "idc download 'x; rm -rf ~' --download-dir ./idc-data" + + async def test_exclude_with_an_analysis_result_id_is_refused(client): """Always empty; a zero would read as "no such data".""" filters = {"terms": {"analysis_result_id": ["qiba_volct_1b"]}, "analysis_results": "exclude"} From d55687a5b1f19d8b29e345be9ede984c2731d1d9 Mon Sep 17 00:00:00 2001 From: Andrey Fedorov Date: Mon, 19 Oct 2026 17:27:16 -0400 Subject: [PATCH 3/3] test(manifest): pin the text manifest as an idc-index input format idc download and idc download-from-manifest read each manifest line with pd.read_csv(header=None) as a single column, so one added field crashes them with a pandas "Length mismatch" error. Nothing recorded that: the only test checked that the first line starts with s3://, and #64 proposed widening the (JSON) manifest rows without anyone noting where the CLI's constraint actually lies. Make it architecture invariant #8 in CLAUDE.md and the developer guide, note it where the URLs are produced, and add tests that every line of manifest.txt and get_cohort_urls, for source=aws and gcs, is one bare s3:////* URL, and that IDCClient's download_from_manifest (the path both CLI commands take, with only _s5cmd_run stubbed) resolves every line to an IDC series and to the right endpoint. Both tests fail when the lines gain a field. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 6 ++ dev/developer_guide.md | 7 ++ src/idc_api/core/services/manifest.py | 9 ++- tests/test_manifest_contract.py | 93 +++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 tests/test_manifest_contract.py diff --git a/CLAUDE.md b/CLAUDE.md index bcde1a6..56894f3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,6 +40,12 @@ These are the rules the design depends on. Full detail and walkthroughs are in 7. **Errors:** raise an `IDCAPIError` subclass from services. REST maps it to `{status, code, message}`; the MCP `guard` decorator converts it to a clean `ToolError`. Never leak tracebacks. +8. **The text manifest is an idc-index input format.** `POST /v3/cohort/manifest.txt` and + `get_cohort_urls` return exactly one `s3:////*` URL per line and + nothing else, because `idc download` / `idc download-from-manifest` parse each line as a + single CSV column (an added field crashes them). Extra per-series fields belong in the JSON + `cohort/manifest` rows or in SQL, never on these lines. `tests/test_manifest_contract.py` + enforces it. Adding a capability touches five places (model → service → REST route → MCP tool → parity test). See the walkthrough in [dev/developer_guide.md](dev/developer_guide.md). diff --git a/dev/developer_guide.md b/dev/developer_guide.md index 712c9b9..4ce3b24 100644 --- a/dev/developer_guide.md +++ b/dev/developer_guide.md @@ -118,6 +118,13 @@ tests/ # pytest suite 7. **Errors:** raise an [`IDCAPIError`](../src/idc_api/core/errors.py) subclass from services. REST maps it to `{status, code, message}`; the MCP `guard` decorator converts it to a clean `ToolError`. Never leak tracebacks. +8. **The text manifest is an idc-index input format.** `POST /v3/cohort/manifest.txt` and + `get_cohort_urls` return exactly one `s3:////*` URL per line and + nothing else. `idc download` and `idc download-from-manifest` read each line as a single CSV + column, so an added field (a series number, a licence) makes them fail with a pandas + `Length mismatch` error. Extra per-series fields belong in the JSON `cohort/manifest` rows or + in SQL. [test_manifest_contract.py](../tests/test_manifest_contract.py) checks the line format + and feeds the output through idc-index's own manifest parsing. ## Walkthrough: add a new capability diff --git a/src/idc_api/core/services/manifest.py b/src/idc_api/core/services/manifest.py index 7de89d8..0eec5e8 100644 --- a/src/idc_api/core/services/manifest.py +++ b/src/idc_api/core/services/manifest.py @@ -38,7 +38,14 @@ def __init__(self, backend: QueryBackend, settings): self.settings = settings def _series_urls(self, where: str, params: list, limit: int) -> tuple[list[str], bool]: - """Return up to ``limit`` ``s3://`` series URLs plus a truncation flag.""" + """Return up to ``limit`` ``s3://`` series URLs plus a truncation flag. + + These URLs are the lines of the text manifest (``manifest_lines`` / ``manifest_text``), + which ``idc download`` / ``idc download-from-manifest`` read as one CSV column. Keep each + line a bare ``s3:////*``: any added field crashes the CLI. + Extra per-series fields go in the JSON manifest rows (``SeriesManifestRow``). Enforced by + tests/test_manifest_contract.py. + """ # `where` is compile_filters output: allow-listed columns, values bound below. rows = self.backend.query( f"SELECT series_aws_url FROM index WHERE {where} " # nosec B608 diff --git a/tests/test_manifest_contract.py b/tests/test_manifest_contract.py new file mode 100644 index 0000000..fd4d561 --- /dev/null +++ b/tests/test_manifest_contract.py @@ -0,0 +1,93 @@ +"""The text manifest is an idc-index input format (architecture invariant #8). + +`POST /v3/cohort/manifest.txt` and `get_cohort_urls` feed `idc download` / +`idc download-from-manifest`, which read each line with `pd.read_csv(header=None)` as a single +column. A line carrying one more field (a series number, a licence) makes them fail with a +pandas "Length mismatch" error. The JSON `cohort/manifest` rows are where extra per-series fields +go. These tests pin the line format, and feed our output through idc-index's own manifest +handling. +""" + +from __future__ import annotations + +import re + +import pytest + +from idc_api.mcp.server import mcp + +# One bare series URL: bucket, crdc_series_uuid, and the s5cmd series wildcard. Nothing else. +_LINE = re.compile( + r"s3://[a-z0-9][a-z0-9.-]*/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/\*" +) + + +@pytest.fixture(scope="module") +def series(ctx): + """The two smallest series in each AWS bucket. That covers the buckets that source=gcs remaps + and the one it doesn't, and keeps the size well below any disk-space check.""" + rows = ctx.query.run_sql( + "SELECT SeriesInstanceUID, series_size_MB FROM index " + "QUALIFY row_number() OVER (PARTITION BY aws_bucket ORDER BY series_size_MB) <= 2", + None, + max_rows=100, + ).rows + assert len(rows) >= 6, "expected at least three buckets" + return rows + + +def _manifest(client, series, source: str) -> str: + uids = [r["SeriesInstanceUID"] for r in series] + body = {"filters": {"terms": {"SeriesInstanceUID": uids}}, "source": source} + r = client.post("/v3/cohort/manifest.txt", json=body) + assert r.status_code == 200, r.text[:200] + return r.text + + +@pytest.mark.parametrize("source", ["aws", "gcs"]) +async def test_every_line_is_one_bare_series_url(client, series, source, parse_mcp): + text = _manifest(client, series, source) + lines = text.splitlines() + assert len(lines) == len(series) + for line in lines: + assert _LINE.fullmatch(line), f"not a bare series URL: {line!r}" + assert text.endswith("\n") + + # get_cohort_urls hands out the same lines. + uids = [r["SeriesInstanceUID"] for r in series] + urls = parse_mcp( + await mcp.call_tool( + "get_cohort_urls", {"terms": {"SeriesInstanceUID": uids}, "source": source} + ) + )["urls"] + assert urls == lines + + +@pytest.mark.parametrize( + ("source", "endpoint"), + [("aws", "https://s3.amazonaws.com"), ("gcs", "https://storage.googleapis.com")], +) +def test_idc_index_reads_our_manifest(client, series, source, endpoint, tmp_path, monkeypatch): + """Run the manifest through IDCClient.download_from_manifest, the path both CLI commands + take, with only the transfer stubbed out (as idc-index's own tests do). Every line must + resolve to an IDC series and the right endpoint.""" + IDCClient = pytest.importorskip("idc_index").IDCClient + client_idc = IDCClient() + captured = {} + monkeypatch.setattr(client_idc, "_s5cmd_run", lambda **kw: captured.update(kw)) + + manifest = tmp_path / "idc_manifest.txt" + manifest.write_text(_manifest(client, series, source)) + # validate_manifest=True would `s5cmd ls` a GCS URL over the network; parsing and series + # resolution run either way. + client_idc.download_from_manifest( + str(manifest), downloadDir=str(tmp_path / "out"), validate_manifest=(source == "aws") + ) + + resolved = captured["s5cmd_sync_helper_df"] + assert len(resolved) == len(series) + assert resolved["index_crdc_series_uuid"].notna().all() + assert captured["endpoint_to_use"] == endpoint + assert captured["total_size"] == pytest.approx( + sum(r["series_size_MB"] for r in series), abs=0.01 + )