diff --git a/CHANGELOG.md b/CHANGELOG.md index 7beb6c3..8fa8994 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,16 @@ 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)). +- 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. ### Changed 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/docs/user-guide.md b/docs/user-guide.md index 77eccc3..3cc4b5c 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 @@ -466,10 +487,18 @@ 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: + ```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: 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..0eec5e8 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 @@ -36,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 @@ -74,18 +83,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 - ) - if only_collection: - for cid in terms["collection_id"]: - commands.append(f"idc download {cid} --download-dir ./idc-data") + # 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 {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/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..b983c57 --- /dev/null +++ b/tests/test_analysis_results_filter.py @@ -0,0 +1,143 @@ +"""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") + + +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"} + 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"]) 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 + )