From 1186eddc44f39f6c0ef9a08546e122ba69299cd7 Mon Sep 17 00:00:00 2001 From: Luke Sargent Date: Thu, 13 Aug 2026 13:42:53 -0700 Subject: [PATCH 1/5] retry mechanics in the CLI tool --- cli_tools/mcdi/mcdi/download/cli.py | 7 ++ cli_tools/mcdi/mcdi/download/sources/base.py | 34 +++++++ cli_tools/mcdi/mcdi/download/sources/gdc.py | 9 +- cli_tools/mcdi/mcdi/download/sources/idc.py | 42 ++++++++- cli_tools/mcdi/mcdi/download/sources/pdc.py | 18 +++- cli_tools/mcdi/tests/test_download_cli.py | 70 ++++++++++++++ cli_tools/mcdi/tests/test_download_idc.py | 91 +++++++++++++++++++ cli_tools/mcdi/tests/test_download_sources.py | 42 +++++++++ 8 files changed, 307 insertions(+), 6 deletions(-) diff --git a/cli_tools/mcdi/mcdi/download/cli.py b/cli_tools/mcdi/mcdi/download/cli.py index caed83c..31ed708 100644 --- a/cli_tools/mcdi/mcdi/download/cli.py +++ b/cli_tools/mcdi/mcdi/download/cli.py @@ -113,4 +113,11 @@ def run(args: argparse.Namespace) -> int: f"\nDone: {len(results)} total, {len(failed)} error(s), " f"{len(mismatches)} checksum mismatch(es), {len(extract_failed)} extraction error(s)" ) + + retry_ids = {r.entry.file_id for r in failed + mismatches} + if 0 < len(retry_ids) < len(results): + retry_manifest = args.manifest.with_name(args.manifest.stem + ".retry" + args.manifest.suffix) + source.write_subset_manifest(args.manifest, retry_ids, retry_manifest) + print(f"Wrote retry manifest for {len(retry_ids)} failed file(s) to {retry_manifest}") + return 1 if failed or mismatches or extract_failed else 0 diff --git a/cli_tools/mcdi/mcdi/download/sources/base.py b/cli_tools/mcdi/mcdi/download/sources/base.py index 92bb39f..66a0787 100644 --- a/cli_tools/mcdi/mcdi/download/sources/base.py +++ b/cli_tools/mcdi/mcdi/download/sources/base.py @@ -28,6 +28,30 @@ def read_lines(path: Path, limit: int = CONTENT_SNIFF_LINES) -> list[str]: return [line for line, _ in zip(f, range(limit))] +def write_csv_subset( + original_path: Path, + output_path: Path, + delimiter: str, + key_column: str, + keep_keys: set[str], +) -> None: + """Re-parse ``original_path`` and write the rows whose ``key_column`` value is in + ``keep_keys`` to ``output_path``, preserving the original header/columns and delimiter. + + Used to carve a re-parseable "retry manifest" out of a manifest a service already + produced, rather than reconstructing rows from the fields ``FileEntry`` retains + (which would risk dropping columns a service's format requires but mcdi doesn't use). + """ + with open(original_path, newline="") as fin: + reader = csv.DictReader(fin, delimiter=delimiter) + fieldnames = reader.fieldnames or [] + rows = [row for row in reader if (row.get(key_column) or "").strip() in keep_keys] + with open(output_path, "w", newline="") as fout: + writer = csv.DictWriter(fout, fieldnames=fieldnames, delimiter=delimiter) + writer.writeheader() + writer.writerows(rows) + + @dataclass class FileEntry: """A single file to download, normalized across manifest formats.""" @@ -63,6 +87,16 @@ def parse_manifest(self, path: Path) -> list[FileEntry]: def request_kwargs(self, entry: FileEntry) -> dict: """Extra kwargs (e.g. headers) to pass to requests.get() for this entry.""" + @abstractmethod + def write_subset_manifest( + self, original_manifest: Path, failed_file_ids: set[str], output_path: Path + ) -> None: + """Write a manifest at ``output_path``, in this source's own format, containing + only the rows for ``failed_file_ids`` (``FileEntry.file_id`` values) from + ``original_manifest`` — so a partial download failure can be retried by feeding + ``output_path`` straight back into ``mcdi download``. + """ + def rate_limit(self) -> Optional["RateLimit"]: """Optional pacing/rate-limit policy applied to downloads from this source.""" return None diff --git a/cli_tools/mcdi/mcdi/download/sources/gdc.py b/cli_tools/mcdi/mcdi/download/sources/gdc.py index af90850..682c40a 100644 --- a/cli_tools/mcdi/mcdi/download/sources/gdc.py +++ b/cli_tools/mcdi/mcdi/download/sources/gdc.py @@ -7,7 +7,7 @@ import requests -from .base import FileEntry, Source +from .base import FileEntry, Source, write_csv_subset log = logging.getLogger("mcdi.download.gdc") @@ -53,6 +53,13 @@ def request_kwargs(self, entry: FileEntry) -> dict: headers["X-Auth-Token"] = self.token return {"headers": headers} + def write_subset_manifest( + self, original_manifest: Path, failed_file_ids: set[str], output_path: Path + ) -> None: + write_csv_subset( + original_manifest, output_path, delimiter="\t", key_column="id", keep_keys=failed_file_ids + ) + def known_open(self, entries: list[FileEntry], session: requests.Session) -> set[str]: """Bulk-query GDC's ``access`` field for every id (no token needed).""" ids = [e.file_id for e in entries] diff --git a/cli_tools/mcdi/mcdi/download/sources/idc.py b/cli_tools/mcdi/mcdi/download/sources/idc.py index 324ebfd..0ce9bb1 100644 --- a/cli_tools/mcdi/mcdi/download/sources/idc.py +++ b/cli_tools/mcdi/mcdi/download/sources/idc.py @@ -13,7 +13,7 @@ from ...errors import ApiError, InputError from ...net import build_session -from .base import CANDIDATE_DELIMITERS, FileEntry, Source, read_header +from .base import CANDIDATE_DELIMITERS, FileEntry, Source, read_header, write_csv_subset # Columns in an IDC cohort manifest (CSV/TSV): series-level (one row per # series) or BigQuery-export (one row per SOPInstanceUID, same columns). @@ -166,6 +166,9 @@ class IDCSource(Source): def __init__(self, lookup: Optional[IdcIndexLookup] = None): self._lookup = lookup or IdcIndexLookup() + # file_id (S3 key) -> (series_uid, crdc_series_uuid), populated by parse_manifest; + # lets write_subset_manifest map a failed download back to its manifest row. + self._series_by_file_id: dict[str, tuple[str, str]] = {} @staticmethod def sniff(header_fields: list[str]) -> bool: @@ -176,6 +179,7 @@ def sniff_lines(lines: list[str]) -> bool: return any(_S5CMD_CP_RE.match(line.strip()) for line in lines) def parse_manifest(self, path: Path) -> list[FileEntry]: + self._series_by_file_id = {} column, ids = _extract_series_refs(path) if not ids: raise InputError(f"IDC manifest {path} contained no resolvable series references.") @@ -205,7 +209,7 @@ def _entries_for_series(self, session: requests.Session, info: SeriesInfo) -> li Path("idc") / info.collection_id / info.patient_id / info.study_uid / f"{info.modality}_{info.series_uid}" ) - return [ + entries = [ FileEntry( file_id=key, filename=key.rsplit("/", 1)[-1], @@ -216,6 +220,9 @@ def _entries_for_series(self, session: requests.Session, info: SeriesInfo) -> li ) for key, size in _list_series_objects(session, info.aws_bucket, info.crdc_series_uuid) ] + for entry in entries: + self._series_by_file_id[entry.file_id] = (info.series_uid, info.crdc_series_uuid) + return entries def request_kwargs(self, entry: FileEntry) -> dict: return {} @@ -224,3 +231,34 @@ def known_open(self, entries: list[FileEntry], session: requests.Session) -> set """Already confirmed to exist via the listing in ``parse_manifest`` - skip the redundant per-file probe in ``engine.check_access``.""" return {e.file_id for e in entries} + + def write_subset_manifest( + self, original_manifest: Path, failed_file_ids: set[str], output_path: Path + ) -> None: + """Map each failed file back to the series (manifest row) it came from - one + manifest row can expand into many files, so a failure pulls in the whole series. + Re-running the result is safe: files already on disk are skipped (see + ``engine._already_present``).""" + delimiter = _csv_delimiter(original_manifest) + if delimiter is not None: + series_uids = { + self._series_by_file_id[fid][0] + for fid in failed_file_ids + if fid in self._series_by_file_id + } + write_csv_subset( + original_manifest, output_path, delimiter=delimiter, key_column="SeriesInstanceUID", + keep_keys=series_uids, + ) + return + + crdc_uuids = { + self._series_by_file_id[fid][1] + for fid in failed_file_ids + if fid in self._series_by_file_id + } + with open(original_manifest) as fin, open(output_path, "w") as fout: + for raw in fin: + m = _S5CMD_CP_RE.match(raw.strip()) + if m and m.group(1) in crdc_uuids: + fout.write(raw) diff --git a/cli_tools/mcdi/mcdi/download/sources/pdc.py b/cli_tools/mcdi/mcdi/download/sources/pdc.py index 86e4e21..f4b13ba 100644 --- a/cli_tools/mcdi/mcdi/download/sources/pdc.py +++ b/cli_tools/mcdi/mcdi/download/sources/pdc.py @@ -13,7 +13,7 @@ from ...errors import ApiError, InputError from ...net import build_session -from .base import CANDIDATE_DELIMITERS, FileEntry, RateLimit, Source, read_header +from .base import CANDIDATE_DELIMITERS, FileEntry, RateLimit, Source, read_header, write_csv_subset REQUIRED_HEADERS = {"PDC Study ID", "Data Category", "File Type", "File Download Link"} @@ -104,11 +104,14 @@ def __init__(self, refresher: Optional[PdcUrlRefresher] = None): def sniff(header_fields: list[str]) -> bool: return REQUIRED_HEADERS.issubset(set(header_fields)) - def parse_manifest(self, path: Path) -> list[FileEntry]: - delimiter = next( + def _detect_delimiter(self, path: Path) -> str: + return next( (d for d in CANDIDATE_DELIMITERS if self.sniff(read_header(path, d))), CANDIDATE_DELIMITERS[-1], ) + + def parse_manifest(self, path: Path) -> list[FileEntry]: + delimiter = self._detect_delimiter(path) rows = [] with open(path, newline="") as f: reader = csv.DictReader(f, delimiter=delimiter) @@ -179,6 +182,15 @@ def _refresh_stale(self, rows: list[dict]) -> None: def request_kwargs(self, entry: FileEntry) -> dict: return {} + def write_subset_manifest( + self, original_manifest: Path, failed_file_ids: set[str], output_path: Path + ) -> None: + delimiter = self._detect_delimiter(original_manifest) + write_csv_subset( + original_manifest, output_path, delimiter=delimiter, key_column="File Name", + keep_keys=failed_file_ids, + ) + def rate_limit(self) -> RateLimit: # Avoids tripping PDC's 24h per-IP restriction on repeated file downloads. return RateLimit(max_per_window=10, window_seconds=600, per_file_sleep_seconds=2) diff --git a/cli_tools/mcdi/tests/test_download_cli.py b/cli_tools/mcdi/tests/test_download_cli.py index 6e19428..3a7a6c5 100644 --- a/cli_tools/mcdi/tests/test_download_cli.py +++ b/cli_tools/mcdi/tests/test_download_cli.py @@ -212,6 +212,76 @@ def test_extract_flag_unpacks_archive(tmp_path, requests_mock): assert requests_mock.call_count == 3 # 1 known_open + 1 pre-flight probe + 1 download, first run only +def test_partial_failure_writes_retry_manifest(tmp_path, requests_mock): + """A file that passes the pre-flight probe but fails the real download (e.g. it + went unreachable in between) should land in a retry manifest alongside the ones + that succeeded - not silently lost.""" + _mock_known_open_empty(requests_mock) + manifest = tmp_path / "gdc_manifest.txt" + _write_gdc_manifest(manifest, [ + ("uuid1", "a.txt", _md5(FILE_A), len(FILE_A)), + ("uuid2", "b.txt", _md5(FILE_B), len(FILE_B)), + ]) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", content=FILE_A) + # uuid2: pre-flight ranged probe (sends "Range") succeeds; the real download + # (no "Range" header) fails - simulating the file going unreachable mid-run. + requests_mock.get("https://api.gdc.cancer.gov/data/uuid2", status_code=404) + requests_mock.get( + "https://api.gdc.cancer.gov/data/uuid2", + request_headers={"Range": "bytes=0-0"}, + status_code=206, content=b"", + ) + + output_dir = tmp_path / "out" + rc = main([ + "download", + "--manifest", str(manifest), + "--output-dir", str(output_dir), + "--retries", "0", + ]) + assert rc == 1 + assert (output_dir / "gdc" / "uuid1" / "a.txt").read_bytes() == FILE_A + + retry_manifest = tmp_path / "gdc_manifest.retry.txt" + assert retry_manifest.exists() + lines = retry_manifest.read_text().splitlines() + assert len(lines) == 2 # header + the one failed row + assert lines[1].startswith("uuid2\t") + + # the retry manifest can itself be fed straight back into `mcdi download` + requests_mock.get("https://api.gdc.cancer.gov/data/uuid2", content=FILE_B) + rc_retry = main([ + "download", + "--manifest", str(retry_manifest), + "--output-dir", str(output_dir), + ]) + assert rc_retry == 0 + assert (output_dir / "gdc" / "uuid2" / "b.txt").read_bytes() == FILE_B + + +def test_full_failure_does_not_write_retry_manifest(tmp_path, requests_mock): + """If every file fails, the original manifest already *is* the retry manifest - + no need to write a redundant copy.""" + _mock_known_open_empty(requests_mock) + manifest = tmp_path / "gdc_manifest.txt" + _write_gdc_manifest(manifest, [("uuid1", "a.txt", _md5(FILE_A), len(FILE_A))]) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", status_code=404) + requests_mock.get( + "https://api.gdc.cancer.gov/data/uuid1", + request_headers={"Range": "bytes=0-0"}, + status_code=206, content=b"", + ) + + rc = main([ + "download", + "--manifest", str(manifest), + "--output-dir", str(tmp_path / "out"), + "--retries", "0", + ]) + assert rc == 1 + assert not (tmp_path / "gdc_manifest.retry.txt").exists() + + def test_extract_failure_leaves_archive_in_place(tmp_path, requests_mock): _mock_known_open_empty(requests_mock) # Named like a tar.gz but not actually one - extraction must fail cleanly. diff --git a/cli_tools/mcdi/tests/test_download_idc.py b/cli_tools/mcdi/tests/test_download_idc.py index dc67589..5aebdac 100644 --- a/cli_tools/mcdi/tests/test_download_idc.py +++ b/cli_tools/mcdi/tests/test_download_idc.py @@ -252,3 +252,94 @@ def test_known_open_vouches_for_every_entry(tmp_path, requests_mock): def test_request_kwargs_is_empty(): assert IDCSource().request_kwargs(entry=None) == {} + + +# --- write_subset_manifest -------------------------------------------------------------- + + +def test_write_subset_manifest_csv_form_keeps_only_failed_series(tmp_path, requests_mock): + manifest = tmp_path / "cohort.csv" + manifest.write_text( + CSV_HEADER + + "pat1,coll,1.2.study.A,1.2.series.A,,,,gs://x\n" + + "pat2,coll,1.2.study.B,1.2.series.B,,,,gs://y\n" + ) + _mock_listing( + requests_mock, + "idc-open-data", + { + SERIES_A.crdc_series_uuid: [([(f"{SERIES_A.crdc_series_uuid}/a.dcm", 10)], False)], + SERIES_B.crdc_series_uuid: [([(f"{SERIES_B.crdc_series_uuid}/b.dcm", 20)], False)], + }, + ) + lookup = FakeLookup({SERIES_A.series_uid: SERIES_A, SERIES_B.series_uid: SERIES_B}) + source = IDCSource(lookup=lookup) + entries = source.parse_manifest(manifest) + failed_entry = next(e for e in entries if e.filename == "a.dcm") + + output = tmp_path / "cohort.retry.csv" + source.write_subset_manifest(manifest, {failed_entry.file_id}, output) + + text = output.read_text() + assert "1.2.series.A" in text + assert "1.2.series.B" not in text + assert text.splitlines()[0] == CSV_HEADER.strip() + + # the subset manifest must itself be re-parseable by the same source + retried = source.parse_manifest(output) + assert {e.filename for e in retried} == {"a.dcm"} + + +def test_write_subset_manifest_s5cmd_form_keeps_only_failed_series(tmp_path, requests_mock): + manifest = tmp_path / "m.s5cmd" + manifest.write_text( + f"cp s3://idc-open-data/{SERIES_A.crdc_series_uuid}/* .\n" + f"cp s3://idc-open-data/{SERIES_B.crdc_series_uuid}/* .\n" + ) + _mock_listing( + requests_mock, + "idc-open-data", + { + SERIES_A.crdc_series_uuid: [([(f"{SERIES_A.crdc_series_uuid}/a.dcm", 10)], False)], + SERIES_B.crdc_series_uuid: [([(f"{SERIES_B.crdc_series_uuid}/b.dcm", 20)], False)], + }, + ) + lookup = FakeLookup({SERIES_A.crdc_series_uuid: SERIES_A, SERIES_B.crdc_series_uuid: SERIES_B}) + source = IDCSource(lookup=lookup) + entries = source.parse_manifest(manifest) + failed_entry = next(e for e in entries if e.filename == "b.dcm") + + output = tmp_path / "m.retry.s5cmd" + source.write_subset_manifest(manifest, {failed_entry.file_id}, output) + + lines = output.read_text().splitlines() + assert lines == [f"cp s3://idc-open-data/{SERIES_B.crdc_series_uuid}/* ."] + + retried = source.parse_manifest(output) + assert {e.filename for e in retried} == {"b.dcm"} + + +def test_write_subset_manifest_keeps_whole_series_when_one_file_of_many_fails(tmp_path, requests_mock): + manifest = tmp_path / "m.s5cmd" + manifest.write_text(f"cp s3://idc-open-data/{SERIES_A.crdc_series_uuid}/* .\n") + _mock_listing( + requests_mock, + "idc-open-data", + { + SERIES_A.crdc_series_uuid: [ + ([(f"{SERIES_A.crdc_series_uuid}/a.dcm", 10), (f"{SERIES_A.crdc_series_uuid}/b.dcm", 20)], False) + ], + }, + ) + lookup = FakeLookup({SERIES_A.crdc_series_uuid: SERIES_A}) + source = IDCSource(lookup=lookup) + entries = source.parse_manifest(manifest) + failed_entry = next(e for e in entries if e.filename == "a.dcm") + + output = tmp_path / "m.retry.s5cmd" + source.write_subset_manifest(manifest, {failed_entry.file_id}, output) + + # Manifest granularity is per-series, not per-file: the whole series comes + # back even though only one of its two files failed. + lines = output.read_text().splitlines() + assert lines == [f"cp s3://idc-open-data/{SERIES_A.crdc_series_uuid}/* ."] diff --git a/cli_tools/mcdi/tests/test_download_sources.py b/cli_tools/mcdi/tests/test_download_sources.py index ade3393..8fc2f04 100644 --- a/cli_tools/mcdi/tests/test_download_sources.py +++ b/cli_tools/mcdi/tests/test_download_sources.py @@ -115,6 +115,48 @@ def test_detect_source_matches_headerless_manifest_via_sniff_lines(tmp_path): assert detect_source(manifest) == "idc" +def test_gdc_write_subset_manifest_keeps_only_failed_rows(tmp_path): + manifest = tmp_path / "gdc_manifest.txt" + manifest.write_text( + "id\tfilename\tmd5\tsize\tstate\n" + "uuid1\tA.svs\tmd5a\t100\treleased\n" + "uuid2\tB.svs\tmd5b\t200\treleased\n" + ) + output = tmp_path / "gdc_manifest.retry.txt" + GDCSource().write_subset_manifest(manifest, {"uuid2"}, output) + + lines = output.read_text().splitlines() + assert lines[0] == "id\tfilename\tmd5\tsize\tstate" + assert lines[1] == "uuid2\tB.svs\tmd5b\t200\treleased" + assert len(lines) == 2 + + # the subset manifest must itself be re-parseable + retried = GDCSource().parse_manifest(output) + assert [e.file_id for e in retried] == ["uuid2"] + + +def test_pdc_write_subset_manifest_keeps_only_failed_rows(tmp_path): + url_a = _fresh_signed_url("https://example.cloudfront.net/a.raw") + url_b = _fresh_signed_url("https://example.cloudfront.net/b.raw") + manifest = tmp_path / "pdc_manifest.csv" + manifest.write_text( + "PDC Study ID,PDC Study Version,Data Category,File Type,File Name," + "File MD5sum,File Download Link\n" + f"PDC000109,1,Raw Mass Spectra,raw,a.raw,md5a,{url_a}\n" + f"PDC000109,1,Raw Mass Spectra,raw,b.raw,md5b,{url_b}\n" + ) + output = tmp_path / "pdc_manifest.retry.csv" + PDCSource(refresher=_UnusedRefresher()).write_subset_manifest(manifest, {"b.raw"}, output) + + lines = output.read_text().splitlines() + assert len(lines) == 2 + assert "b.raw" in lines[1] + assert "a.raw" not in lines[1] + + retried = PDCSource(refresher=_UnusedRefresher()).parse_manifest(output) + assert [e.filename for e in retried] == ["b.raw"] + + def test_pdc_parse_manifest_ignores_filename_extension(tmp_path): manifest = tmp_path / "dataset_3.dat" manifest.write_text( From 9f6d6e7f76188b0ee0c69b1fe435185ac418816d Mon Sep 17 00:00:00 2001 From: Luke Sargent Date: Thu, 13 Aug 2026 14:59:00 -0700 Subject: [PATCH 2/5] adding partial download logic to galaxy tool --- cli_tools/mcdi/mcdi/__init__.py | 2 +- cli_tools/mcdi/mcdi/download/cli.py | 21 ++++++++++-- cli_tools/mcdi/tests/test_download_cli.py | 6 ++-- tools/manifest_downloader/macros.xml | 2 +- .../manifest_downloader.xml | 33 +++++++++++++++++-- .../test-data/gdc_manifest_bad_id.txt | 2 ++ tools/manifest_gdc/macros.xml | 2 +- 7 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 tools/manifest_downloader/test-data/gdc_manifest_bad_id.txt diff --git a/cli_tools/mcdi/mcdi/__init__.py b/cli_tools/mcdi/mcdi/__init__.py index 3aaafce..b0afb73 100644 --- a/cli_tools/mcdi/mcdi/__init__.py +++ b/cli_tools/mcdi/mcdi/__init__.py @@ -6,7 +6,7 @@ import os -__version__ = "0.6.0" +__version__ = "0.7.0" # Container image build id (e.g. git SHA); empty for local/editable installs. BUILD = os.environ.get("MCDI_BUILD", "").strip() diff --git a/cli_tools/mcdi/mcdi/download/cli.py b/cli_tools/mcdi/mcdi/download/cli.py index 31ed708..99d7017 100644 --- a/cli_tools/mcdi/mcdi/download/cli.py +++ b/cli_tools/mcdi/mcdi/download/cli.py @@ -56,6 +56,13 @@ def add_arguments(subparsers: argparse._SubParsersAction) -> argparse.ArgumentPa "--retry-backoff", type=float, default=5.0, help="Seconds to wait before each retry pass, multiplied by the attempt number (default: 5.0)", ) + parser.add_argument( + "--retry-manifest-out", type=Path, + help="Where to write the retry manifest on a partial failure (default: " + "'.retry', next to --manifest). Only written if some but not all files " + "failed; use this to redirect it somewhere writable when --manifest's own directory " + "isn't (e.g. a Galaxy job's staged input).", + ) parser.add_argument("--verbose", action="store_true") parser.set_defaults(func=run) return parser @@ -115,9 +122,17 @@ def run(args: argparse.Namespace) -> int: ) retry_ids = {r.entry.file_id for r in failed + mismatches} - if 0 < len(retry_ids) < len(results): - retry_manifest = args.manifest.with_name(args.manifest.stem + ".retry" + args.manifest.suffix) + partial = 0 < len(retry_ids) < len(results) + if partial: + retry_manifest = args.retry_manifest_out or args.manifest.with_name( + args.manifest.stem + ".retry" + args.manifest.suffix + ) source.write_subset_manifest(args.manifest, retry_ids, retry_manifest) print(f"Wrote retry manifest for {len(retry_ids)} failed file(s) to {retry_manifest}") - return 1 if failed or mismatches or extract_failed else 0 + if not (failed or mismatches or extract_failed): + return 0 + # Some but not all files ended up missing/wrong (or all downloaded but some failed to + # extract): outputs exist but are incomplete. Distinct from 1 (nothing usable at all) so + # callers - e.g. the Galaxy wrapper's exit-code mapping - can tell the two apart. + return 3 if len(retry_ids) < len(results) else 1 diff --git a/cli_tools/mcdi/tests/test_download_cli.py b/cli_tools/mcdi/tests/test_download_cli.py index 3a7a6c5..23ec699 100644 --- a/cli_tools/mcdi/tests/test_download_cli.py +++ b/cli_tools/mcdi/tests/test_download_cli.py @@ -239,7 +239,7 @@ def test_partial_failure_writes_retry_manifest(tmp_path, requests_mock): "--output-dir", str(output_dir), "--retries", "0", ]) - assert rc == 1 + assert rc == 3 # partial success: some files missing, but not all assert (output_dir / "gdc" / "uuid1" / "a.txt").read_bytes() == FILE_A retry_manifest = tmp_path / "gdc_manifest.retry.txt" @@ -297,7 +297,9 @@ def test_extract_failure_leaves_archive_in_place(tmp_path, requests_mock): "--output-dir", str(output_dir), "--extract", ]) - assert rc == 1 # extraction errors are reported as a run failure + # extraction errors are reported as a partial failure - the download itself + # succeeded and the archive is still present, just not unpacked + assert rc == 3 dest_dir = output_dir / "gdc" / "uuid1" # the archive was NOT moved aside, since extraction never succeeded diff --git a/tools/manifest_downloader/macros.xml b/tools/manifest_downloader/macros.xml index a262654..9344866 100644 --- a/tools/manifest_downloader/macros.xml +++ b/tools/manifest_downloader/macros.xml @@ -1,5 +1,5 @@ - 0.6.0 + 0.7.0 0 25.1 diff --git a/tools/manifest_downloader/manifest_downloader.xml b/tools/manifest_downloader/manifest_downloader.xml index cc27ede..828497c 100644 --- a/tools/manifest_downloader/manifest_downloader.xml +++ b/tools/manifest_downloader/manifest_downloader.xml @@ -11,8 +11,14 @@ macros.xml + + + + + + + + + @@ -111,6 +120,11 @@ + + + + diff --git a/tools/manifest_downloader/test-data/gdc_manifest_bad_id.txt b/tools/manifest_downloader/test-data/gdc_manifest_bad_id.txt new file mode 100644 index 0000000..1fd68db --- /dev/null +++ b/tools/manifest_downloader/test-data/gdc_manifest_bad_id.txt @@ -0,0 +1,2 @@ +id filename md5 size state +00000000-0000-0000-0000-000000000000 nonexistent.txt deadbeefdeadbeefdeadbeefdeadbeef 1 released diff --git a/tools/manifest_gdc/macros.xml b/tools/manifest_gdc/macros.xml index 01e9c2a..5cca180 100644 --- a/tools/manifest_gdc/macros.xml +++ b/tools/manifest_gdc/macros.xml @@ -1,5 +1,5 @@ - 0.6.0 + 0.7.0 0 22.05 From cf229311833199bc072f3148ce23d3984dfca623 Mon Sep 17 00:00:00 2001 From: Luke Sargent Date: Thu, 13 Aug 2026 15:52:03 -0700 Subject: [PATCH 3/5] partial success manifest dataset -> list of 0 or 1 - list of 0 does not appear in history --- .../manifest_downloader.xml | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/tools/manifest_downloader/manifest_downloader.xml b/tools/manifest_downloader/manifest_downloader.xml index 828497c..2fd4a56 100644 --- a/tools/manifest_downloader/manifest_downloader.xml +++ b/tools/manifest_downloader/manifest_downloader.xml @@ -44,9 +44,9 @@ - - - + + + @@ -197,19 +197,21 @@ start, but a file can still fail *after* downloading begins (e.g. a transient network error or the source going down mid-run), even after this tool's automatic retries. When that happens with at least one other file still succeeding, the job completes (green, with a warning noted in its job -info) rather than failing outright, and a second output dataset — **files to -retry**, a manifest in the same format as the input — appears alongside the -(partial) collection, containing just the files that didn't make it. Run -this tool again using that dataset as the manifest input to retry only -those; already-downloaded files are left alone either way. This second -output only appears when a retry is actually needed. +info) rather than failing outright, and a second output collection — **files +to retry**, holding a single manifest dataset in the same format as the +input — appears alongside the (partial) downloads, listing just the files +that didn't make it. Run this tool again using that dataset as the manifest +input to retry only those; already-downloaded files are left alone either +way. This second output is empty (no dataset inside it) unless a retry is +actually needed. **Output** A collection containing the files listed in the manifest, as downloaded from the corresponding data commons — or, for any file that was extracted, its extracted contents in place of the packed archive. On a partial failure, a -second **files to retry** dataset also appears (see above). +second **files to retry** collection also appears, holding one manifest +dataset (see above). ]]> From c8417a07c7e1cc4c0b86db2b9f699ea0edd8348c Mon Sep 17 00:00:00 2001 From: Luke Sargent Date: Tue, 1 Sep 2026 11:56:51 -0700 Subject: [PATCH 4/5] retry output file fix --- .../manifest_downloader.xml | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/tools/manifest_downloader/manifest_downloader.xml b/tools/manifest_downloader/manifest_downloader.xml index 2fd4a56..11aea14 100644 --- a/tools/manifest_downloader/manifest_downloader.xml +++ b/tools/manifest_downloader/manifest_downloader.xml @@ -18,10 +18,15 @@ galaxy.json; fi; + exit \$MCDI_EXIT; ]]> - - - @@ -197,21 +199,20 @@ start, but a file can still fail *after* downloading begins (e.g. a transient network error or the source going down mid-run), even after this tool's automatic retries. When that happens with at least one other file still succeeding, the job completes (green, with a warning noted in its job -info) rather than failing outright, and a second output collection — **files -to retry**, holding a single manifest dataset in the same format as the -input — appears alongside the (partial) downloads, listing just the files -that didn't make it. Run this tool again using that dataset as the manifest -input to retry only those; already-downloaded files are left alone either -way. This second output is empty (no dataset inside it) unless a retry is -actually needed. +info) rather than failing outright, and a second output dataset — **files to +retry**, a manifest in the same format as the input — appears alongside the +(partial) downloads, listing just the files that didn't make it. Run this +tool again using that dataset as the manifest input to retry only those; +already-downloaded files are left alone either way. This second output only +appears in your history when a retry is actually needed — it's absent +entirely on a fully successful run. **Output** A collection containing the files listed in the manifest, as downloaded from the corresponding data commons — or, for any file that was extracted, its extracted contents in place of the packed archive. On a partial failure, a -second **files to retry** collection also appears, holding one manifest -dataset (see above). +second **files to retry** dataset also appears (see above). ]]> From 6c5fc26503798960cc6f2ba436353ba163a67a09 Mon Sep 17 00:00:00 2001 From: Luke Sargent Date: Tue, 1 Sep 2026 13:36:51 -0700 Subject: [PATCH 5/5] update readme --- cli_tools/mcdi/README.md | 116 ++++++++++++++++++++++----------------- 1 file changed, 65 insertions(+), 51 deletions(-) diff --git a/cli_tools/mcdi/README.md b/cli_tools/mcdi/README.md index c0c9f57..3cdaf6e 100644 --- a/cli_tools/mcdi/README.md +++ b/cli_tools/mcdi/README.md @@ -1,17 +1,41 @@ -# GaCDI — Galaxy Cancer Data Importers +# mcdi — Multi-Commons Data Importer -Galaxy Cancer Data Importers (GaCDI) provides Galaxy tools for importing cancer -datasets from major public and controlled-access cancer data repositories into -Galaxy histories. This package provides one command, `mcdi` (Multi-Commons Data -Importer), with two subcommands: `mcdi manifest` builds a manifest from -filters, and `mcdi download` downloads the files a GDC, PDC, or IDC manifest -lists (whether built here or exported from a portal). +`mcdi` is a command-line tool for importing cancer datasets from major public +and controlled-access cancer data repositories: the NCI Genomic Data Commons +([GDC](https://portal.gdc.cancer.gov)), Proteomic Data Commons +([PDC](https://pdc.cancer.gov)), and Imaging Data Commons +([IDC](https://portal.imaging.datacommons.cancer.gov)). It has two +subcommands: `mcdi manifest` builds a manifest from filters, and `mcdi +download` downloads the files a GDC, PDC, or IDC manifest lists. + +Manifests are usually exported directly from one of those three portals; +`mcdi download` accepts any of their native export formats as-is. `mcdi +manifest gdc` additionally builds a GDC manifest — plus enriched metadata — +straight from filters, as an alternative to the portal UI. + +`mcdi` also powers the GaCDI (Galaxy Cancer Data Importers) Galaxy tools +published in this repo (`manifest_gdc`, `manifest_downloader`) — see their +own help text for Galaxy-specific usage. + +## Installation + +```bash +pip install cli_tools/mcdi +``` + +Or run the published container directly, without installing anything locally +(the same image also backs the repo's Galaxy tools): + +```bash +docker run --rm quay.io/goeckslab/mcdi: mcdi manifest gdc --help +docker run --rm quay.io/goeckslab/mcdi: mcdi download --help +``` ## Manifest Builder -`gacdi_manifest_gdc` generates the **manifests** that drive the importers. Instead -of downloading a whole dataset, the user filters the NCI -[Genomic Data Commons](https://gdc.cancer.gov/) and gets exactly the files they +`mcdi manifest gdc` builds the **manifests** that drive the downloader. +Instead of downloading a whole dataset, you filter the NCI +[Genomic Data Commons](https://gdc.cancer.gov/) and get exactly the files you want, described in two complementary outputs: - **GDC manifest** — strict `id / filename / md5 / size / state`, consumable @@ -60,26 +84,24 @@ a raw GDC filters JSON (`--raw-filters`). The manifest is emitted in a determini ### Feeding the manifest into `mcdi download` -A single Galaxy workflow goes *filter → manifest → download → analysis*: build -the manifest here, run **GaCDI Manifest Downloader** (`mcdi download`, below) -to bring the files into the history, then join `metadata.tsv` to those history -datasets to attach clinical labels, the `galaxy_ext` datatype hint, and subtype -annotations to each sample. +The usual flow is *filter → manifest → download → analysis*: build the +manifest here, run `mcdi download` (below) to fetch the files, then join +`metadata.tsv` to the downloaded files to attach clinical labels, the +`galaxy_ext` datatype hint, and subtype annotations to each sample. (This +hand-off is also what powers the paired GaCDI Galaxy tools.) -This works because of a contract between the two tools (locked by -`tests/test_importer_contract.py`): +This works because of a contract between the two tools: 1. **Manifest → downloader.** `gdc_manifest.txt` is a TSV whose header (`id, filename, md5, size, state`) is a superset of what `mcdi download`'s - GDC parsing requires (`id/filename/md5/size`); its datatype (`txt`) is - accepted by the downloader's manifest input (`tabular,txt`). Rows with no - `id` are dropped so the manifest and metadata stay row-aligned. The same - file also works with `gdc-client download -m gdc_manifest.txt`. -2. **Metadata ↔ history.** `metadata.tsv` leads with `file_id` and `filename` — - the same keys the downloaded collection's datasets are named/identified by - — so after download you join the metadata to the collection on `file_id` - (stable UUID) or `filename` to attach clinical labels, the `galaxy_ext` - datatype hint, and subtype annotations to each sample in the history. + GDC parsing requires (`id/filename/md5/size`). Rows with no `id` are + dropped so the manifest and metadata stay row-aligned. The same file also + works with `gdc-client download -m gdc_manifest.txt`. +2. **Metadata ↔ downloaded files.** `metadata.tsv` leads with `file_id` and + `filename` — the same identifiers `mcdi download` organizes its output by + (see "Output layout" below) — so after download you join the metadata on + `file_id` (stable UUID) or `filename` to attach clinical labels, the + `galaxy_ext` datatype hint, and subtype annotations to each sample. ## Downloading files from a manifest @@ -124,6 +146,7 @@ The data commons is auto-detected from the manifest's content; pass | `--token-file PATH` | File containing a GDC auth token, for controlled-access files | | `--retries N` | Extra attempts for files that fail transiently within this run (default: 2) | | `--retry-backoff SECONDS` | Wait before each retry pass, multiplied by the attempt number (default: 5.0) | +| `--retry-manifest-out PATH` | Where to write the retry manifest on a partial failure (default: `.retry`, next to `--manifest`) | Some commons files are themselves archives (e.g. a `.tar.gz` bundle of slides). `--extract` unpacks any recognized archive into the same directory it was @@ -131,9 +154,9 @@ downloaded into, right after downloading it. On success, the archive itself then moves to a sibling `.mcdi-archives/` directory (mirroring `--output-dir`'s layout) — it isn't deleted, just relocated out of the way, so `--output-dir` ends up holding only the extracted contents, not a -redundant copy of the packed archive next to them. A tool that recursively -collects everything under `--output-dir` (e.g. Galaxy's `discover_datasets`) -then only ever sees the actual extracted files. If extraction fails, the +redundant copy of the packed archive next to them. Anything that +recursively scans `--output-dir` afterward only ever sees the actual +extracted files. If extraction fails, the archive is left where it was downloaded instead, so there's still something to show for it. The archive's presence in `.mcdi-archives/` also doubles as the idempotency marker, so reruns skip both re-downloading and @@ -152,10 +175,16 @@ file. This always runs; there's no flag to skip it. request, a failed file (connection errors, `429`/`5xx`, or a checksum mismatch — not permanent-looking failures like `401`/`403`/`404`) gets `--retries` more whole-batch attempts, waiting `--retry-backoff × attempt` -seconds between passes. This matters most where nothing will manually rerun -the command for you on a failure — e.g. a Galaxy job, where a retried job -gets a fresh working directory, not the partial output of the failed attempt, -so anything not resolved within the one invocation is lost. +seconds between passes. This matters most in automated contexts where +nothing else will rerun the command for you on a failure. + +**Partial success.** If some files succeed and others still fail after +retries, the run doesn't just fail outright — it also writes a manifest of +just the failed files (`.retry`, or `--retry-manifest-out`'s +path if given) so you can rerun `mcdi download` against only those, instead +of the whole original manifest. The exit code tells you which case you're +in: `0` clean success, `3` partial success (some files missing, but at least +one succeeded), `1` total failure (nothing usable was produced). Controlled-access GDC files need an auth token, obtained by logging into the GDC portal and downloading your token. Provide it either via the `GDC_TOKEN` @@ -181,29 +210,14 @@ downloaded (verifying checksums too, if `--verify-checksum` is set), so interrupted runs can simply be re-run — the pre-flight check skips them too, so a rerun over mostly-complete output is cheap, not another full pass. -## Runtime environment - -Both Galaxy tools (`manifest_gdc`, `manifest_downloader`) reference the same -pinned container, `quay.io/goeckslab/mcdi:` — there's no Conda -package, so the container is the sole runtime. It's built from -`cli_tools/mcdi/Dockerfile`, tagged from `mcdi/__init__.py`'s `__version__`, -and built/pushed automatically by `.github/workflows/containers.yml` on -merges to `main` that touch `cli_tools/mcdi/**`. +## License -```bash -docker build -t mcdi:dev cli_tools/mcdi -docker run --rm mcdi:dev mcdi manifest gdc --help -docker run --rm mcdi:dev mcdi download --help -``` +See [LICENSE](LICENSE). -## Development +## Contributing ```bash python -m pip install -e '.[dev]' pytest -q -m "not network" # mocked; add `-m network` for live-API tests planemo lint tools/manifest_gdc tools/manifest_downloader ``` - -## License - -See [LICENSE](LICENSE).