diff --git a/CHANGELOG.md b/CHANGELOG.md index 75bef17..7351486 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,17 @@ Refactors, CI, and formatting land in the git history, not here. ## [Unreleased] +### Removed + +- **The local-download surface: `POST /v3/download` (REST) and the `download_cohort` MCP tool** + (beta contract change). Both only worked when the server ran on the caller's own machine and + errored everywhere else — on the hosted deployment (the common case) the tool's mere presence + misled agents into calling it. Downloading through the server is also never the right path: + every IDC bucket is public, so direct S3/GCS transfer is strictly more efficient. Retrieval is + now manifests/URLs only on every surface — use `get_cohort_urls` / `POST /v3/cohort/manifest.txt` + or the ready-to-run `idc` CLI commands in the `build_cohort` response. The + `IDC_API_ENABLE_LOCAL_DOWNLOAD` config variable is gone with it. + ### Fixed - `get_cohort_urls` / `POST /v3/cohort/manifest.txt` with `source=gcs` now return `s3://` URLs diff --git a/README.md b/README.md index 990d371..44d169f 100644 --- a/README.md +++ b/README.md @@ -79,8 +79,8 @@ needed. ```bash uv run idc-api # REST API → http://127.0.0.1:8000 (Swagger at /v3/docs) -uv run idc-mcp # MCP server, stdio (local) — can also download files -uv run idc-mcp --http --host 0.0.0.0 --port 8080 # MCP server, hosted/shared (manifests only) +uv run idc-mcp # MCP server, stdio (local) +uv run idc-mcp --http --host 0.0.0.0 --port 8080 # MCP server, hosted/shared ``` Quick taste — build a breast-MRI cohort over REST: diff --git a/dev/api_v3_plan.md b/dev/api_v3_plan.md index f2ea25b..679f94b 100644 --- a/dev/api_v3_plan.md +++ b/dev/api_v3_plan.md @@ -42,6 +42,10 @@ open; **no authentication** is required of callers. the MCP server **in local (stdio) mode** can additionally perform the real download via `idc-index`. Hosted/remote MCP behaves like REST (manifests only). No server-side staging/zipping (collections reach TBs). + *Superseded during beta:* the local-download surface (`POST /v3/download` / + `download_cohort`) was removed before 3.0.0 — a tool that exists but errors on the + dominant hosted deployment misled agents, and direct S3/GCS transfer (all buckets are + public) is strictly better. Retrieval is manifests/URLs only in every mode; see CHANGELOG. - **MVP scope:** **Core radiology + discovery** and **Viewer URLs + citations + licenses**. Specialized indices (ct/mr/pt, seg/ann/rtstruct, sm) and clinical data are later phases. @@ -67,7 +71,6 @@ src/idc_api/ viewer.py # OHIF/SLIM viewer URLs citations.py # DOI-based citations (APA/BibTeX/CSL/Turtle) licenses.py # license breakdown for a selection - download.py # LOCAL-ONLY: real file transfer via idc-index/s5cmd models/ # Pydantic request/response models = the shared contract schema/ # index column metadata, filterable-attribute + filter defs rest/ # FastAPI app: routers are ~10-line wrappers over services @@ -86,8 +89,7 @@ reuse, rather than rebuild: all ~16 indices). - `IDCClient` methods to wrap directly: `get_idc_version()`, `indices_overview` / `get_index_schema()` (schema discovery), `get_viewer_URL()`, `citations_from_selection()` - (+ `CITATION_FORMAT_*`), `download_from_selection()` / `download_dicom_series()` (local - download), `fetch_index()` (later phases). + (+ `CITATION_FORMAT_*`), `fetch_index()` (later phases). - **Do not** reuse `IDCClient`'s single shared DuckDB connection for serving — DuckDB connections aren't thread-safe. Instead `duckdb_backend.py` opens its own **read-only** connection over the same Parquet and hands out per-request cursors (see Safety). @@ -122,8 +124,8 @@ No forked code, no SQL string surgery, no webapp dependency, no shared-secret to **Retrieval** - `get_manifest` — s5cmd manifest + public https URLs (AWS+GCS) + `idc download` command. -- `download_cohort` — **MCP local mode only**; performs the transfer via `idc-index`. - Absent/disabled on the hosted REST + remote MCP surface. +- `download_cohort` — *planned as MCP-local-only, then removed during beta* (see the + superseded note under *Decisions locked with the user*). ## LLM-first details (apply throughout) - **Prescriptive tool descriptions** — every MCP tool says *when to call it*, not just what @@ -235,7 +237,7 @@ general MCP guidance to treat all tool inputs as untrusted and apply defense in guarded `sql_query` + schema discovery + viewer/citations/licenses + manifest/URLs. Both REST and MCP. Cloud Run deploy + local stdio MCP entrypoint. - **Phase 2:** specialized indices (ct/mr/pt/contrast/volume_geometry, seg/ann/rtstruct, - sm/sm_instance) as tables + targeted tools; local `download_cohort`; CDN caching + sm/sm_instance) as tables + targeted tools; CDN caching (see [caching_and_cdn.md](caching_and_cdn.md)); generated client SDK + examples. - **Phase 3:** `BigQueryBackend` behind the same interface for full DICOM metadata, per-segment anatomy, and SR radiomics/qualitative measurements; clinical index + tables. diff --git a/dev/architecture.md b/dev/architecture.md index 4f0633f..ee477af 100644 --- a/dev/architecture.md +++ b/dev/architecture.md @@ -10,7 +10,7 @@ for *how to work in it* see [`dev/developer_guide.md`](developer_guide.md). must be exposable as a well-described MCP tool *and* an HTTP endpoint with no duplicated logic. 2. **Lean on `idc-index`.** Reuse its bundled Parquet index + DuckDB engine; do not - reimplement queries, citations, viewer URLs, or downloads from scratch. This avoids any + reimplement queries, citations, viewer URLs, or manifests from scratch. This avoids any BigQuery/webapp coupling and SQL-string surgery. 3. **Swappable backend.** Storage sits behind a narrow `QueryBackend` interface so an alternative engine could be substituted without touching services or adapters — not a @@ -81,7 +81,6 @@ Pydantic models: | `ViewerService` | [viewer.py](../src/idc_api/core/services/viewer.py) | OHIF/SLIM viewer URLs | | `CitationsService` | [citations.py](../src/idc_api/core/services/citations.py) | DOI-based citations | | `LicenseService` | [licenses.py](../src/idc_api/core/services/licenses.py) | license breakdown | -| `DownloadService` | [download.py](../src/idc_api/core/services/download.py) | local-only file transfer via idc-index | ### `core/models.py` — the shared contract [models.py](../src/idc_api/core/models.py) holds the Pydantic request/response models returned @@ -179,13 +178,12 @@ Full threat model and primary-source references (OWASP, DuckDB Securing guide) a | | REST API | MCP — local (stdio) | MCP — hosted (http) | |---|---|---|---| | Runs | hosted (Cloud Run) | on the user's machine | hosted | -| Filesystem access to caller | no | **yes** | no | -| `download_cohort` / `POST /download` | 501 (disabled) | performs real transfer via idc-index | disabled | -| Data retrieval | manifest + URLs + `idc` command | real download **or** manifest | manifest + URLs | +| Data retrieval | manifest + URLs + `idc` command | manifest + URLs + `idc` command | manifest + URLs + `idc` command | -`main()` in [`mcp/server.py`](../src/idc_api/mcp/server.py) flips -`settings.enable_local_download` on for stdio. `DownloadService.available()` reads that flag at -call time. +Retrieval is deliberately **manifests/URLs only** in every mode: all IDC buckets are public, so +the caller transfers directly from S3/GCS (`idc` CLI / s5cmd) — strictly faster than proxying +bytes through the service, and identical behavior local or hosted. (A local-only download +endpoint/tool existed pre-3.0.0 and was removed in beta; see CHANGELOG.) The hosted HTTP transport is configured **stateless** (`stateless_http=True`, `json_response=True` on the `FastMCP(...)` constructor) so each request is self-contained and diff --git a/dev/developer_guide.md b/dev/developer_guide.md index 1a76004..20a70e6 100644 --- a/dev/developer_guide.md +++ b/dev/developer_guide.md @@ -198,7 +198,6 @@ Environment variables (prefix `IDC_API_`), defined in | `SQL_TIMEOUT_SECONDS` | 30 | Statement timeout | | `DEFAULT_PAGE_SIZE` / `MAX_PAGE_SIZE` | 100 / 5000 | Manifest paging | | `MANIFEST_HARD_CAP` | 100000 | Max series a single manifest enumerates | -| `ENABLE_LOCAL_DOWNLOAD` | false | Allow real downloads (stdio MCP sets this) | | `CORS_ALLOW_ORIGINS` / `HOST` / `PORT` | `["*"]` / 127.0.0.1 / 8000 | REST serving | | `SQL_LOG_MODE` / `SQL_LOG_CHARS` | snippet / 200 | How `run_sql`/`POST /v3/sql` renders in the audit log: `snippet` (first `SQL_LOG_CHARS` chars) or `hash` (a short digest, no query text) | | `BUILD` | (unset) | Deploy stamp appended to the MCP `serverInfo.version` | diff --git a/docs/user-guide.md b/docs/user-guide.md index ef7d044..b7be998 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -56,7 +56,7 @@ understanding, because picking the right one makes everything else easy: |---|---|---|---| | **Discovery** | "What exists? What can I filter on?" | `GET /v3/version`, `/v3/stats`, `/v3/collections`, `/v3/collections/{id}`, `/v3/analysis_results`, `/v3/attributes`, `/v3/attributes/{attr}/values` | `get_idc_version`, `get_stats`, `list_collections`, `get_collection`, `list_analysis_results`, `list_attributes`, `get_attribute_values` | | **Cohort** | "How big is *my* selection, and what's in it?" | `POST /v3/cohort/counts`, `POST /v3/cohort/manifest` | `build_cohort` | -| **Retrieval** | "Give me the download links / files" | `POST /v3/cohort/manifest.txt`, `POST /v3/download` | `get_cohort_urls`, `download_cohort` | +| **Retrieval** | "Give me the download links" | `POST /v3/cohort/manifest.txt` | `get_cohort_urls` | | **SQL** | "Run my custom query" + schema | `GET /v3/tables`, `/v3/tables/{table}`, `POST /v3/sql` | `list_tables`, `get_table_schema`, `run_sql` | | **Side tools** | View / cite / license-check a cohort | `GET /v3/viewer-url`, `POST /v3/citations`, `POST /v3/licenses` | `get_viewer_url`, `get_citations`, `get_licenses` | @@ -238,7 +238,6 @@ uv run idc-api # http://127.0.0.1:8000 — Swagger UI at /v3/docs | `GET /v3/viewer-url` | OHIF/SLIM viewer link for a study/series | | `POST /v3/citations` | Citations for a cohort | | `POST /v3/licenses` | License breakdown for a cohort | -| `POST /v3/download` | Local download convenience (returns 501 unless enabled) — prefer `/cohort/manifest.txt` | ### Worked examples @@ -325,22 +324,13 @@ curl -s localhost:8000/v3/citations \ curl -s 'localhost:8000/v3/viewer-url?study_instance_uid=1.3.6.1.4.1.14519.5.2.1.7009.9004.983700485806071099502442051273' ``` -**Local download** — only when the server runs in local mode (otherwise returns `501`); use -`dry_run` to preview the plan without transferring: - -```bash -curl -s localhost:8000/v3/download \ - -H 'content-type: application/json' \ - -d '{"download_dir": "/data/idc", "collection_id": ["nlst"], "dry_run": true}' -``` - --- ## 3. Using the MCP server (LLM agents) ```bash -uv run idc-mcp # stdio (local) — can also download files -uv run idc-mcp --http --host 0.0.0.0 --port 8080 # hosted/shared (manifests only) +uv run idc-mcp # stdio (local) +uv run idc-mcp --http --host 0.0.0.0 --port 8080 # hosted/shared ``` ### Tools, by surface @@ -350,7 +340,7 @@ uv run idc-mcp --http --host 0.0.0.0 --port 8080 # hosted/shared (manifests - **Schema (for SQL):** `list_tables`, `get_table_schema` - **Clinical data:** `list_clinical_tables`, `get_clinical_table_schema`, `get_clinical_table` - **Cohort / query:** `build_cohort`, `run_sql` -- **Retrieval & side tools:** `get_cohort_urls`, `download_cohort`, `get_viewer_url`, +- **Retrieval & side tools:** `get_cohort_urls`, `get_viewer_url`, `get_citations`, `get_licenses` - **Resources:** `idc://guide` (data model + recommended workflow), `idc://tables`, `idc://schema/{table}` @@ -411,28 +401,19 @@ configured stateless with plain-JSON responses** — each request is self-contai - **Session-bound MCP features are not available** (server→client sampling, elicitation, resource subscriptions, streamed progress) — this server exposes only client-initiated tools + static resources, so it doesn't use them. -- **`download_cohort` can't write to your machine** over HTTP — retrieval returns a manifest + - URLs instead (see *Local vs hosted* below). Use the stdio config above when you want real - downloads. Operator-side detail (deploy command, host-header / DNS-rebinding settings, the autoscaling rationale) is in [deployment.md](../dev/deployment.md). -### Local vs hosted (downloads) - -An MCP server can run **locally** (stdio, on your machine — it can write files, so -`download_cohort` actually fetches DICOM via `idc-index`/s5cmd) or **hosted** (HTTP, shared — -no filesystem access, so retrieval returns a manifest + public URLs + an `idc download` -command instead). Same tools, two behaviors. - --- ## 4. Getting the data -**Recommended: get a manifest, then pull the files directly from S3/GCS.** All series URLs -point at **public AWS S3 and GCS buckets — no credentials needed** — so the transfer never -goes through the API server, works the same against the hosted or a local instance, and scales -to whole collections. Two ways to do it: +**Get a manifest, then pull the files directly from S3/GCS.** All series URLs point at +**public AWS S3 and GCS buckets — no credentials needed** — so the transfer never goes through +the API server (the server never moves bytes; retrieval always means URLs/manifests), works +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`: @@ -452,14 +433,6 @@ to whole collections. Two ways to do it: Install the CLI with `pip install idc-index` (provides the `idc` command). -**Local download mode** — `POST /v3/download` (REST) or `download_cohort` (MCP) drives that -same `idc-index`/s5cmd transfer for you as a convenience, but it works **only when the server -itself runs on your machine** (hosted deployments return a clear error instead). It doesn't -move data any faster than options 1–2 above, so prefer those for bulk transfers or when -scripting against the hosted service; reach for this only when you're already running the -server locally and want the tool/endpoint to do the transfer for you. Start with `dry_run` to -report size, confirm, then run for real. - --- ## 5. Guarded SQL — why it's safe @@ -506,7 +479,6 @@ Environment variables (prefix `IDC_API_`): | `DEFAULT_PAGE_SIZE` | `100` | Default `cohort/manifest` page size | | `MAX_PAGE_SIZE` | `5000` | Upper bound on page size | | `MANIFEST_HARD_CAP` | `100000` | Max series enumerated into a manifest | -| `ENABLE_LOCAL_DOWNLOAD` | `false` | Allow `download` to write files locally | | `CORS_ALLOW_ORIGINS` | `["*"]` | Allowed CORS origins (REST). List value — set as JSON, e.g. `["https://app.example.com"]` | | `HSTS_MAX_AGE` | `31536000` | `Strict-Transport-Security` max-age (seconds) added to every REST and hosted-MCP response. Default is the production value (1 year); dev/test deploys use `3600` so a bad deploy can't lock browsers out for a year. `0` disables the header | | `HOST` / `PORT` | `127.0.0.1` / `8000` | REST bind address | diff --git a/src/idc_api/core/context.py b/src/idc_api/core/context.py index 58604f3..ee8fa54 100644 --- a/src/idc_api/core/context.py +++ b/src/idc_api/core/context.py @@ -10,7 +10,6 @@ ClinicalService, CohortService, DiscoveryService, - DownloadService, LicenseService, ManifestService, QueryService, @@ -30,7 +29,6 @@ def __init__(self, settings: Settings | None = None): self.viewer = ViewerService(self.backend) self.citations = CitationsService(self.backend) self.licenses = LicenseService(self.backend) - self.download = DownloadService(self.backend, self.settings) def close(self) -> None: self.backend.close() diff --git a/src/idc_api/core/errors.py b/src/idc_api/core/errors.py index 6c2b10b..48d91f4 100644 --- a/src/idc_api/core/errors.py +++ b/src/idc_api/core/errors.py @@ -44,11 +44,3 @@ class QueryTimeoutError(IDCAPIError): class ResultTooLargeError(IDCAPIError): code = "result_too_large" status = 400 - - -class UnsupportedOperationError(IDCAPIError): - """The capability exists but is disabled in this deployment mode (e.g. local download - on a hosted server).""" - - code = "unsupported_operation" - status = 501 diff --git a/src/idc_api/core/services/__init__.py b/src/idc_api/core/services/__init__.py index 41ed599..69643e1 100644 --- a/src/idc_api/core/services/__init__.py +++ b/src/idc_api/core/services/__init__.py @@ -8,7 +8,6 @@ from .clinical import ClinicalService from .cohort import CohortService from .discovery import DiscoveryService -from .download import DownloadService from .licenses import LicenseService from .manifest import ManifestService from .query import QueryService @@ -19,7 +18,6 @@ "ClinicalService", "CohortService", "DiscoveryService", - "DownloadService", "LicenseService", "ManifestService", "QueryService", diff --git a/src/idc_api/core/services/download.py b/src/idc_api/core/services/download.py deleted file mode 100644 index bee1518..0000000 --- a/src/idc_api/core/services/download.py +++ /dev/null @@ -1,71 +0,0 @@ -"""LOCAL-ONLY download via idc-index/s5cmd. - -Enabled only when ``settings.enable_local_download`` is True — i.e. the MCP server running -locally (stdio) on the user's machine. On the hosted REST API / remote MCP it raises -``UnsupportedOperationError`` (the server has no access to the caller's filesystem); callers -use the manifest/URLs instead. The heavy ``IDCClient`` is created lazily and guarded by a -lock (its DuckDB connection is not thread-safe).""" - -from __future__ import annotations - -import threading -from typing import Any - -from ..errors import InvalidQueryError, UnsupportedOperationError - - -class DownloadService: - def __init__(self, backend, settings): - self.backend = backend - self.settings = settings - self._client = None - self._lock = threading.Lock() - - def available(self) -> bool: - return bool(self.settings.enable_local_download) - - def _get_client(self): - if self._client is None: - from idc_index import IDCClient # heavy import; only when actually downloading - - self._client = IDCClient() - return self._client - - def download( - self, - download_dir: str, - collection_id: list[str] | str | None = None, - patientId: list[str] | str | None = None, - studyInstanceUID: list[str] | str | None = None, - seriesInstanceUID: list[str] | str | None = None, - dry_run: bool = False, - source_bucket_location: str = "aws", - ) -> dict[str, Any]: - if not self.available(): - raise UnsupportedOperationError( - "Local download is disabled in this deployment. Use the manifest/URLs and " - "download with the `idc` CLI or s5cmd. (Local download is available when the " - "IDC MCP server runs locally on your machine.)" - ) - if not any([collection_id, patientId, studyInstanceUID, seriesInstanceUID]): - raise InvalidQueryError( - "Provide at least one selection: collection_id, patientId, " - "studyInstanceUID, or seriesInstanceUID." - ) - - with self._lock: - client = self._get_client() - client.download_from_selection( - downloadDir=download_dir, - collection_id=collection_id, - patientId=patientId, - studyInstanceUID=studyInstanceUID, - seriesInstanceUID=seriesInstanceUID, - dry_run=dry_run, - source_bucket_location=source_bucket_location, - ) - return { - "status": "dry_run" if dry_run else "completed", - "download_dir": download_dir, - "source": source_bucket_location, - } diff --git a/src/idc_api/core/services/manifest.py b/src/idc_api/core/services/manifest.py index 55c3ae7..09a7da6 100644 --- a/src/idc_api/core/services/manifest.py +++ b/src/idc_api/core/services/manifest.py @@ -1,6 +1,6 @@ """Manifest generation: public series URLs (AWS + GCS), s5cmd-style manifest text, and -ready-to-run ``idc`` CLI commands for a cohort. The service never moves bytes — local -download lives in ``download.py`` (MCP local mode only).""" +ready-to-run ``idc`` CLI commands for a cohort. The service never moves bytes — callers +download directly from the public S3/GCS buckets (``idc`` CLI / s5cmd).""" from __future__ import annotations diff --git a/src/idc_api/mcp/server.py b/src/idc_api/mcp/server.py index c99a0f3..ac86740 100644 --- a/src/idc_api/mcp/server.py +++ b/src/idc_api/mcp/server.py @@ -7,8 +7,11 @@ columns/values instead of guessing. Transports: - * stdio (default) — runs locally on the user's machine; ``download_cohort`` can fetch files. - * streamable-http (``--http``) — hosted/shared; download is disabled (manifests only). + * stdio (default) — runs locally on the user's machine. + * streamable-http (``--http``) — hosted/shared. + +Retrieval is manifests/URLs only on both transports: data transfer happens directly from the +public S3/GCS buckets (``idc`` CLI / s5cmd), never through this server. """ from __future__ import annotations @@ -55,9 +58,9 @@ (non-imaging) attributes — staging, demographics, therapy — use list_clinical_tables, then get_clinical_table_schema / get_clinical_table to read the rows (or run_sql against `clinical.`). -3. IDC is large (100+ TB) — always report counts/size_TB and warn before any download. Prefer - get_cohort_urls / the returned `idc` commands — direct S3/GCS transfer, no server involved. - download_cohort only drives that same transfer, and only when the server runs locally. +3. IDC is large (100+ TB) — always report counts/size_TB and warn before any download. To get + data, use get_cohort_urls / the returned `idc` commands — direct S3/GCS transfer from public + buckets, no server involved. Cite with get_citations (per-dataset citations plus the IDC paper to acknowledge IDC itself); respect get_licenses (CC-BY vs CC-BY-NC). See `idc://guide` for the data model, the full tool list, and join examples.""" @@ -455,33 +458,6 @@ def get_licenses(terms: dict | None = None, ranges: dict | None = None) -> dict: return ctx.licenses.get_licenses(f).model_dump(mode="json") -@mcp.tool() -@guard -def download_cohort( - download_dir: str, - collection_id: list[str] | None = None, - patientId: list[str] | None = None, - studyInstanceUID: list[str] | None = None, - seriesInstanceUID: list[str] | None = None, - dry_run: bool = True, - source: str = "aws", -) -> dict: - """Download DICOM files for a selection to a local directory (via idc-index/s5cmd). Prefer - get_cohort_urls / the idc commands for direct S3/GCS transfer — this tool just drives that - same transfer for convenience, and only works when this MCP server runs locally on the - user's machine; otherwise it returns a clear error. Start with dry_run=True to report the - size, confirm with the user, then dry_run=False.""" - return ctx.download.download( - download_dir=download_dir, - collection_id=collection_id, - patientId=patientId, - studyInstanceUID=studyInstanceUID, - seriesInstanceUID=seriesInstanceUID, - dry_run=dry_run, - source_bucket_location=source, - ) - - # --- resources ---------------------------------------------------------------------------- _GUIDE = """\ @@ -500,7 +476,7 @@ def download_cohort( names + valid values) you filter on. - *Cohort* (`build_cohort`) — turn a chosen combination of that vocabulary into distinct counts + a sample of series + a download payload. -- *Retrieval* (`get_cohort_urls`, `download_cohort`) — the download half: public URLs / files. +- *Retrieval* (`get_cohort_urls`) — the download half: public URLs for direct S3/GCS transfer. - *SQL* (`list_tables`, `get_table_schema`, `run_sql`) — the escape hatch for anything `build_cohort` can't express (GROUP BY, joins, custom aggregations). - *Clinical* (`list_clinical_tables`, `get_clinical_table_schema`, `get_clinical_table`) — @@ -528,12 +504,11 @@ def download_cohort( `truncated=false` means the result is complete; `true` means raise the limit and re-check (or narrow/aggregate). `run_sql`'s `max_rows` is clamped to a server ceiling, so there is no 'unlimited' value — for bulk *series* use the cohort/manifest tools, not raw `run_sql` rows. -4. *Get the data:* prefer `get_cohort_urls` (public s3:// URLs; `source=gcs` reaches GCS via +4. *Get the data:* `get_cohort_urls` (public s3:// URLs; `source=gcs` reaches GCS via its S3-compatible endpoint, still s3://) or the ready-to-run `idc` CLI commands already in the `build_cohort` response — both transfer directly from S3/GCS with no server involved, and work the same whether this server is hosted or local. - `download_cohort` just drives that same transfer for convenience, and only works when the - server itself runs on the user's machine (hosted deployments reject it). + This server never moves files itself; hand the user the commands/URLs to run. 5. *Be a good citizen:* check `get_licenses` (CC BY vs CC BY-NC) and, when publishing, include `get_citations` output — both the per-dataset `citations` and `idc_acknowledgment` (the IDC paper, https://doi.org/10.1148/rg.230180) to acknowledge IDC itself. @@ -669,8 +644,6 @@ def main() -> None: log_level=mcp.settings.log_level.lower(), ) else: - # Local stdio mode: the server is on the user's machine, so enable real downloads. - ctx.settings.enable_local_download = True mcp.run(transport="stdio") diff --git a/src/idc_api/rest/app.py b/src/idc_api/rest/app.py index 0862304..c5fb12e 100644 --- a/src/idc_api/rest/app.py +++ b/src/idc_api/rest/app.py @@ -123,29 +123,6 @@ class CitationsRequest(BaseModel): citation_format: str = "apa" -class DownloadRequest(BaseModel): - model_config = ConfigDict( - json_schema_extra={ - "examples": [ - { - "download_dir": "/data/idc", - "collection_id": ["nlst"], - "dry_run": True, - "source_bucket_location": "aws", - } - ] - } - ) - - download_dir: str - collection_id: list[str] | None = None - patientId: list[str] | None = None - studyInstanceUID: list[str] | None = None - seriesInstanceUID: list[str] | None = None - dry_run: bool = False - source_bucket_location: str = "aws" - - @asynccontextmanager async def lifespan(app: FastAPI): # Build the DuckDB backend at startup so the first request is fast. @@ -588,24 +565,6 @@ def licenses(filters: CohortFilters): (CC BY-NC) before reuse.""" return C().licenses.get_licenses(filters) - # --- download (local mode only) --- - @app.post(f"{API_PREFIX}/download", tags=["tools"], summary="Download cohort (local only)") - def download(req: DownloadRequest): - """Download DICOM files for a selection to a local directory (via idc-index/s5cmd). - Prefer `/cohort/manifest.txt` or the idc CLI commands for direct S3/GCS transfer — this - endpoint just drives that same transfer for convenience, and works only when the API - runs locally on the caller's machine; a hosted deployment returns a clear error. Start - with `dry_run=true` to report the download size, then set `dry_run=false` to transfer.""" - return C().download.download( - download_dir=req.download_dir, - collection_id=req.collection_id, - patientId=req.patientId, - studyInstanceUID=req.studyInstanceUID, - seriesInstanceUID=req.seriesInstanceUID, - dry_run=req.dry_run, - source_bucket_location=req.source_bucket_location, - ) - return app diff --git a/src/idc_api/settings.py b/src/idc_api/settings.py index 41bac57..5c1ebfe 100644 --- a/src/idc_api/settings.py +++ b/src/idc_api/settings.py @@ -48,11 +48,6 @@ class Settings(BaseSettings): # every redeploy, making it possible to confirm which build a hosted instance is running. build: str | None = None - # --- Deployment mode --- - # True only when the MCP server runs locally (stdio) on the user's machine, where it may - # actually download files. Hosted REST / remote MCP keep this False (manifests only). - enable_local_download: bool = False - # --- MCP HTTP transport security --- # The MCP streamable-HTTP transport has DNS-rebinding protection that allow-lists the Host # header (localhost-only by default), which rejects a hosted domain (e.g. Cloud Run) with diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 64d1ac1..cc4b562 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -29,9 +29,10 @@ async def test_tools_registered(server): "get_viewer_url", "get_citations", "get_licenses", - "download_cohort", } assert expected <= names + # Removed in beta: retrieval is manifests/URLs only, so no local-download tool. + assert "download_cohort" not in names async def test_tool_descriptions_are_prescriptive(server): diff --git a/tests/test_rest.py b/tests/test_rest.py index 8f174a4..dfa540c 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -137,12 +137,11 @@ def test_sql_engine_error_is_a_clean_400(client): assert "no_such_column" in err["message"] -def test_download_disabled_returns_501(client): - r = client.post( - "/v3/download", json={"download_dir": "/tmp/x", "collection_id": ["rider_pilot"]} - ) - assert r.status_code == 501 - assert r.json()["error"]["code"] == "unsupported_operation" +def test_download_endpoint_removed(client): + # Retrieval is manifests/URLs only: data transfers directly from the public S3/GCS + # buckets, never through the API. The local-download endpoint was removed in beta. + r = client.post("/v3/download", json={"download_dir": "/tmp/x"}) + assert r.status_code == 404 def test_openapi_served(client):