From 4be2b1df4484dc55978c7ee4c702670bc3b08f5a Mon Sep 17 00:00:00 2001 From: Kilian Paul <81327374+Darlokt@users.noreply.github.com> Date: Fri, 15 May 2026 19:20:05 +0200 Subject: [PATCH 1/7] Test infrastructure cleanup and unification of optional datasets handling This commit refactors the test infrastructure to unify the handling of optional datasets across CI and local development. Changes: - Split existing tests into unit and integration tests, with clear guidelines in CONTRIBUTING.md, for better organization and clarity. - Existing tests were split into unit and integration tests based on their scope. - Split tests were moved to their new locations under `tests/unit/` and `tests/integration/`. - Unified missing locally missing datasets handling - Tests that require external datasets now check for their presence and skip with a clear message if unavailable. - Previously only some test suites did so (macsima, visium hd, etc.) but in different ways, with a non unified behaviour. - Added a new script `scripts/download_test_data.py` to download all optional datasets used by CI, with a clear CLI and documentation in CONTRIBUTING.md. - This script centralizes the logic for downloading test datasets, making it easier for developers to set up their local environment with the same data used in CI. - Added a new script `scripts/download_test_data_datasets.py` that defines the dataset keys and their metadata, which is used by both the downloader and the tests, to avoid duplication and ensure consistency. - This script serves as a single source of truth for available test datasets, their keys, and metadata, improving maintainability. - Allows for easier addition of new datasets in the future, as they only need to be added in one place. - Allows for easier developer experience, as they can easily download the datasets using the same script used in CI, without needing to manually find and download them from their sources. - Updated CI workflows to use the new downloader script, ensuring that the same datasets are used in both CI and local development. - This change ensures consistency between CI and local testing environments, reducing the likelihood of discrepancies due to missing or different datasets. - Bumped CI actions versions to their latest major versions - Updated `actions/checkout` to v6 and `actions/setup-python` to v6 in all workflows. - Updated `actions/cache` to v5 in the test workflow. --- .github/workflows/prepare_test_data.yaml | 74 +---- .github/workflows/release.yaml | 4 +- .github/workflows/test.yaml | 6 +- docs/contributing.md | 26 ++ pyproject.toml | 8 + scripts/download_test_data.py | 187 +++++++++++ scripts/download_test_data_datasets.py | 148 +++++++++ tests/_utils.py | 30 -- tests/conftest.py | 69 ++++ .../{ => integration/readers}/test_generic.py | 0 tests/integration/readers/test_macsima.py | 295 ++++++++++++++++++ .../{ => integration/readers}/test_seqfish.py | 25 +- .../readers}/test_visium_hd.py | 64 +--- .../{ => integration/readers}/test_xenium.py | 178 +++-------- tests/{ => unit/cli}/test_cli_alignment.py | 0 tests/{ => unit}/converters/__init__.py | 0 .../converters/test_legacy_anndata.py | 0 tests/{ => unit/readers}/test_macsima.py | 282 +---------------- tests/{ => unit}/readers/test_utils_image.py | 0 tests/unit/readers/test_visium_hd.py | 32 ++ tests/unit/readers/test_xenium.py | 105 +++++++ tests/unit/test_download_test_data.py | 203 ++++++++++++ tests/{ => unit}/test_init.py | 0 23 files changed, 1158 insertions(+), 578 deletions(-) create mode 100644 scripts/download_test_data.py create mode 100644 scripts/download_test_data_datasets.py delete mode 100644 tests/_utils.py rename tests/{ => integration/readers}/test_generic.py (100%) create mode 100644 tests/integration/readers/test_macsima.py rename tests/{ => integration/readers}/test_seqfish.py (70%) rename tests/{ => integration/readers}/test_visium_hd.py (72%) rename tests/{ => integration/readers}/test_xenium.py (58%) rename tests/{ => unit/cli}/test_cli_alignment.py (100%) rename tests/{ => unit}/converters/__init__.py (100%) rename tests/{ => unit}/converters/test_legacy_anndata.py (100%) rename tests/{ => unit/readers}/test_macsima.py (66%) rename tests/{ => unit}/readers/test_utils_image.py (100%) create mode 100644 tests/unit/readers/test_visium_hd.py create mode 100644 tests/unit/readers/test_xenium.py create mode 100644 tests/unit/test_download_test_data.py rename tests/{ => unit}/test_init.py (100%) diff --git a/.github/workflows/prepare_test_data.yaml b/.github/workflows/prepare_test_data.yaml index 34d15cca..ba9e5a21 100644 --- a/.github/workflows/prepare_test_data.yaml +++ b/.github/workflows/prepare_test_data.yaml @@ -6,78 +6,30 @@ on: workflow_dispatch: # Uncomment and adjust the branch name if you need to add new datasets to the artifact. # It needs to be a branch in the spatialdata-io origin repository, not from a fork. -# push: -# branches: -# - main + # push: + # branches: + # - main jobs: prepare-data: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - name: Checkout repository + uses: actions/checkout@v6 - - name: Download test datasets - run: | - mkdir -p ./data - cd ./data - - # ------- - # the Xenium datasets are licensed as CC BY 4.0, as shown here - # https://www.10xgenomics.com/support/software/xenium-onboard-analysis/latest/resources/xenium-example-data - - # 10x Genomics Xenium 2.0.0 - curl -O https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Breast_2fov/Xenium_V1_human_Breast_2fov_outs.zip - curl -O https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Lung_2fov/Xenium_V1_human_Lung_2fov_outs.zip - - # 10x Genomics Xenium 3.0.0 (5K) Mouse ileum, multimodal cell segmentation - # this file seems to be corrupted; skipping it for now - # curl -O https://cf.10xgenomics.com/samples/xenium/3.0.0/Xenium_Prime_MultiCellSeg_Mouse_Ileum_tiny/Xenium_Prime_MultiCellSeg_Mouse_Ileum_tiny.zip - - # 10x Genomics Xenium 3.0.0 (5K) Mouse ileum, nuclear expansion - curl -O https://cf.10xgenomics.com/samples/xenium/3.0.0/Xenium_Prime_Mouse_Ileum_tiny/Xenium_Prime_Mouse_Ileum_tiny_outs.zip - - # 10x Genomics Xenium 4.0.0 (v1) Human ovary, nuclear expansion - curl -O https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Human_Ovary_tiny/Xenium_V1_Human_Ovary_tiny_outs.zip - - # 10x Genomics Xenium 4.0.0 (v1) Human ovary, multimodal cell segmentation - curl -O https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_MultiCellSeg_Human_Ovary_tiny/Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip - - # 10x Genomics Xenium 4.0.0 (v1+Protein) Human kidney, multimodal cell segmentation - curl -O https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Protein_Human_Kidney_tiny/Xenium_V1_Protein_Human_Kidney_tiny_outs.zip - - # ------- - # the Visium HD dataset is licensed as CC BY 4.0, as shown here - # https://www.10xgenomics.com/support/software/space-ranger/latest/resources/visium-hd-example-data - - # 10x Genomics Visium HD 4.0.1 3' Mouse Brain Chunk - curl -O https://cf.10xgenomics.com/samples/spatial-exp/4.0.1/Visium_HD_Tiny_3prime_Dataset/Visium_HD_Tiny_3prime_Dataset_outs.zip - - # ------- - # we received written permission to make the following dataset public and integrate it in the CI system of spatialdata-io - # Spatial Genomics seqFISH v2 - curl -O https://s3.embl.de/spatialdata/raw_data/seqfish-2-test-dataset.zip - - # ------- - # MACSima OMAP datasets are licensed as CC BY 4.0 - # OMAP23 for format v1.x.x - curl -o OMAP23_small.zip "https://zenodo.org/api/records/18196452/files-archive" - - # OMAP10 for format v0.x.x - curl -o OMAP10_small.zip "https://zenodo.org/api/records/18196366/files-archive" + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: "3.13" - - name: Unzip files + - name: Download test datasets run: | - cd ./data - for file in *.zip; do - dir="${file%.zip}" - mkdir -p "$dir" - unzip "$file" -d "$dir" - rm "$file" - done + python scripts/download_test_data.py --output ./data - name: Upload artifacts - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7 with: name: data path: ./data + if-no-files-found: error diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 17cea589..2c17c872 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -9,9 +9,9 @@ jobs: runs-on: ubuntu-latest if: startsWith(github.ref, 'refs/tags/v') steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - name: Set up Python 3.12 - uses: actions/setup-python@v5 + uses: actions/setup-python@v6 with: python-version: "3.13" cache: pip diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 4ee1a57d..f128a314 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -26,9 +26,9 @@ jobs: PYTHON: ${{ matrix.python }} steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - name: Set up Python ${{ matrix.python }} - uses: actions/setup-python@v5 + uses: actions/setup-python@v6 with: python-version: ${{ matrix.python }} @@ -37,7 +37,7 @@ jobs: run: | echo "::set-output name=dir::$(pip cache dir)" - name: Restore pip cache - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: ${{ steps.pip-cache-dir.outputs.dir }} key: pip-${{ runner.os }}-${{ env.pythonLocation }}-${{ hashFiles('**/pyproject.toml') }} diff --git a/docs/contributing.md b/docs/contributing.md index ebfda601..b18b0ded 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -123,6 +123,32 @@ If the `download.py` and `to_zarr.py` scripts require Python imports for package We encourage testing the reader function and any helper function. +Tests are split by scope: + +- Unit tests live in `tests/unit/` and should not require downloaded test data. +- Integration tests live in `tests/integration/` and cover reader workflows, CLI commands, file I/O, and zarr roundtrips. +- Integration tests that require external datasets use dataset keys from `scripts/download_test_data_datasets.py`. + They resolve data under `SPATIALDATA_IO_TEST_DATA_DIR` when set, otherwise `data/` in the repository root. If the required dataset is unavailable, the test should skip with a clear message. + +Useful local commands: + +```bash +pytest tests/unit +pytest tests/integration +pytest -m "integration and data" +python scripts/download_test_data.py --group xenium +SPATIALDATA_IO_TEST_DATA_DIR=/path/to/data pytest -m data +``` + +To download the same optional datasets used by CI, run: + +```bash +python scripts/download_test_data.py +``` + +By default, the downloader skips datasets that already exist. Use `--force` to redownload selected datasets, `--dataset` for a single dataset key, and `--list` to show the available keys. +The dataset registry lives in `scripts/download_test_data_datasets.py`; append new entries there when adding or updating test datasets. + ### Testing multiple versions When multiple versions of the raw data format are present, we encourage testing the reader on all of them to ensure backward compatibility. This task is greatly simplified if small test datasets are used for the CI tests. If this is not available, we suggest running the tests locally on multiple versions of the data before the PR is ready for review. diff --git a/pyproject.toml b/pyproject.toml index 05c5baa2..9f7a131e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -82,6 +82,14 @@ testpaths = ["tests"] xfail_strict = true addopts = [ "--import-mode=importlib", # allow using test files with same name + "--strict-markers", +] +markers = [ + "unit: fast isolated tests that do not require external datasets", + "integration: multi-component tests, file I/O tests, or reader workflow tests", + "data: tests that require optional downloaded test datasets", + "slow: tests with comparatively high runtime", + "cli: command-line interface tests", ] [tool.ruff] diff --git a/scripts/download_test_data.py b/scripts/download_test_data.py new file mode 100644 index 00000000..d493fed5 --- /dev/null +++ b/scripts/download_test_data.py @@ -0,0 +1,187 @@ +from __future__ import annotations + +import argparse +import importlib.util +import shutil +import sys +import tempfile +import time +import urllib.error +import urllib.request +import zipfile +from pathlib import Path +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from types import ModuleType + + from download_test_data_datasets import TestDataset as TestDatasetType + + +# This script uses a custom user agent, because some providers block requests with the default Python user agent. +REQUEST_HEADERS = { + "User-Agent": "curl/8.0.0", + "Accept": "*/*", +} +DOWNLOAD_TIMEOUT_SEC = 60 +MAX_DOWNLOAD_ATTEMPTS = 3 +RETRY_DELAY_SEC = 2.0 +TRANSIENT_HTTP_STATUS_CODES = {408, 429} + + +def _load_dataset_manifest() -> ModuleType: + manifest_path = Path(__file__).with_name("download_test_data_datasets.py") + spec = importlib.util.spec_from_file_location("download_test_data_datasets", manifest_path) + if spec is None or spec.loader is None: + raise RuntimeError(f"Could not import {manifest_path}") + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +_DATASET_MANIFEST = _load_dataset_manifest() +TestDataset = _DATASET_MANIFEST.TestDataset +DATASETS: tuple[TestDatasetType, ...] = _DATASET_MANIFEST.DATASETS +get_dataset = _DATASET_MANIFEST.get_dataset +datasets_by_group = _DATASET_MANIFEST.datasets_by_group + + +class DatasetDownloadError(RuntimeError): + """Error raised when a test dataset cannot be downloaded or extracted.""" + + def __init__(self, dataset: TestDatasetType, action: str, reason: str, suggestion: str | None = None) -> None: + self.dataset = dataset + self.action = action + self.reason = reason + self.suggestion = suggestion + message = f"{dataset.key}: failed to {action} {dataset.url}: {reason}" + if suggestion is not None: + message = f"{message}. {suggestion}" + super().__init__(message) + + +def _parse_args() -> argparse.Namespace: + dataset_keys = sorted(dataset.key for dataset in DATASETS) + groups = sorted({dataset.group for dataset in DATASETS}) + parser = argparse.ArgumentParser(description="Download optional test datasets used by spatialdata-io CI.") + parser.add_argument("--output", type=Path, default=Path("data"), help="Directory where datasets are extracted.") + parser.add_argument( + "--dataset", + action="append", + choices=dataset_keys, + help="Dataset key to download. May be passed multiple times. Defaults to all datasets.", + ) + parser.add_argument( + "--group", + action="append", + choices=groups, + help="Dataset group to download. May be passed multiple times.", + ) + parser.add_argument("--force", action="store_true", help="Redownload and replace existing selected datasets.") + parser.add_argument("--list", action="store_true", help="List available datasets and exit.") + return parser.parse_args() + + +def _selected_datasets(dataset_keys: list[str] | None, groups: list[str] | None) -> list[TestDatasetType]: + selected_keys = set(dataset_keys or ()) + selected_groups = set(groups or ()) + if not selected_keys and not selected_groups: + return list(DATASETS) + return [dataset for dataset in DATASETS if dataset.key in selected_keys or dataset.group in selected_groups] + + +def _is_transient_error(error: BaseException) -> bool: + if isinstance(error, urllib.error.HTTPError): + return error.code in TRANSIENT_HTTP_STATUS_CODES or 500 <= error.code < 600 + return isinstance(error, urllib.error.URLError | TimeoutError) + + +def _download_error(dataset: TestDatasetType, error: BaseException) -> DatasetDownloadError: + if isinstance(error, urllib.error.HTTPError): + reason = f"HTTP {error.code} {error.reason}" + suggestion = None + if error.code == 403: + suggestion = ( + "The server rejected the request. The downloader sends a curl-like User-Agent; " + "if this persists, verify the URL in a browser or with curl and check whether the provider changed access rules" + ) + return DatasetDownloadError(dataset, "download", reason, suggestion) + if isinstance(error, urllib.error.URLError): + return DatasetDownloadError(dataset, "download", f"URL error: {error.reason}") + if isinstance(error, TimeoutError): + return DatasetDownloadError(dataset, "download", f"timed out after {DOWNLOAD_TIMEOUT_SEC} seconds") + return DatasetDownloadError(dataset, "download", str(error)) + + +def _download(dataset: TestDatasetType, archive_path: Path) -> None: + print(f"Downloading {dataset.key} from {dataset.url}") + for attempt in range(1, MAX_DOWNLOAD_ATTEMPTS + 1): + request = urllib.request.Request(dataset.url, headers=REQUEST_HEADERS) + try: + with urllib.request.urlopen(request, timeout=DOWNLOAD_TIMEOUT_SEC) as response: + with archive_path.open("wb") as handle: + shutil.copyfileobj(response, handle) + return + except (urllib.error.HTTPError, urllib.error.URLError, TimeoutError) as exc: + if attempt < MAX_DOWNLOAD_ATTEMPTS and _is_transient_error(exc): + print(f"Retrying {dataset.key} after download failure ({attempt}/{MAX_DOWNLOAD_ATTEMPTS}): {exc}") + time.sleep(RETRY_DELAY_SEC) + continue + raise _download_error(dataset, exc) from exc + + +def _extract_zip(dataset: TestDatasetType, archive_path: Path, destination: Path) -> None: + try: + with zipfile.ZipFile(archive_path) as archive: + archive.extractall(destination) + except zipfile.BadZipFile as exc: + raise DatasetDownloadError(dataset, "extract", "downloaded file is not a valid zip archive") from exc + + +def download_dataset(dataset: TestDatasetType, output: Path, force: bool) -> None: + target = output / dataset.extracted_dir + if target.exists() and not force: + print(f"Skipping {dataset.key}: {target} already exists") + return + + output.mkdir(parents=True, exist_ok=True) + with tempfile.TemporaryDirectory(prefix=f"{dataset.key}-", dir=output) as tmpdir: + tmp_path = Path(tmpdir) + archive_path = tmp_path / dataset.archive_name + extracted_path = tmp_path / dataset.extracted_dir + extracted_path.mkdir() + + _download(dataset, archive_path) + _extract_zip(dataset, archive_path, extracted_path) + + if target.exists(): + shutil.rmtree(target) + shutil.move(str(extracted_path), target) + print(f"Downloaded {dataset.key} to {target}") + + +def main() -> None: + args = _parse_args() + if args.list: + for dataset in DATASETS: + print(f"{dataset.key}\t{dataset.group}\t{dataset.extracted_dir}\t{dataset.source}") + return + + failures: list[DatasetDownloadError] = [] + for dataset in _selected_datasets(args.dataset, args.group): + try: + download_dataset(dataset, args.output, args.force) + except DatasetDownloadError as exc: + failures.append(exc) + print(f"ERROR: {exc}") + + if failures: + print(f"Failed to download {len(failures)} dataset(s):") + for failure in failures: + print(f"- {failure}") + raise SystemExit(1) + + +if __name__ == "__main__": + main() diff --git a/scripts/download_test_data_datasets.py b/scripts/download_test_data_datasets.py new file mode 100644 index 00000000..e6e42c2a --- /dev/null +++ b/scripts/download_test_data_datasets.py @@ -0,0 +1,148 @@ +from __future__ import annotations + +from dataclasses import dataclass +from functools import cache + + +@dataclass(frozen=True) +class TestDataset: + """Test dataset to be downloaded. + + Parameters + ---------- + key : str + Unique identifier for the dataset, used for referencing in tests and + the CLI. + group : str + Logical grouping of the dataset, for example ``"xenium"``, + ``"visium_hd"``, ``"seqfish"``, or ``"macsima"``. + url : str + Direct URL to the dataset archive, for example a ZIP file. + archive_name : str + Expected filename of the downloaded archive. + extracted_dir : str + Expected name of the directory created when the archive is extracted. + source : str + Human-readable description of the dataset source and license. + test_path : str + Optional path inside the extracted directory that should be passed to + integration tests. + """ + + key: str + group: str + url: str + archive_name: str + extracted_dir: str + source: str + test_path: str = "" + + +DATASETS = ( + TestDataset( + key="xenium_breast", + group="xenium", + url="https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Breast_2fov/Xenium_V1_human_Breast_2fov_outs.zip", + archive_name="Xenium_V1_human_Breast_2fov_outs.zip", + extracted_dir="Xenium_V1_human_Breast_2fov_outs", + source="10x Genomics Xenium 2.0.0, CC BY 4.0", + ), + TestDataset( + key="xenium_lung", + group="xenium", + url="https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Lung_2fov/Xenium_V1_human_Lung_2fov_outs.zip", + archive_name="Xenium_V1_human_Lung_2fov_outs.zip", + extracted_dir="Xenium_V1_human_Lung_2fov_outs", + source="10x Genomics Xenium 2.0.0, CC BY 4.0", + ), + TestDataset( + key="xenium_prime_mouse_ileum", + group="xenium", + url="https://cf.10xgenomics.com/samples/xenium/3.0.0/Xenium_Prime_Mouse_Ileum_tiny/Xenium_Prime_Mouse_Ileum_tiny_outs.zip", + archive_name="Xenium_Prime_Mouse_Ileum_tiny_outs.zip", + extracted_dir="Xenium_Prime_Mouse_Ileum_tiny_outs", + source="10x Genomics Xenium 3.0.0, CC BY 4.0", + ), + TestDataset( + key="xenium_ovary", + group="xenium", + url="https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Human_Ovary_tiny/Xenium_V1_Human_Ovary_tiny_outs.zip", + archive_name="Xenium_V1_Human_Ovary_tiny_outs.zip", + extracted_dir="Xenium_V1_Human_Ovary_tiny_outs", + source="10x Genomics Xenium 4.0.0, CC BY 4.0", + ), + TestDataset( + key="xenium_multicell_ovary", + group="xenium", + url="https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_MultiCellSeg_Human_Ovary_tiny/Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip", + archive_name="Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip", + extracted_dir="Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs", + source="10x Genomics Xenium 4.0.0, CC BY 4.0", + ), + TestDataset( + key="xenium_protein_kidney", + group="xenium", + url="https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Protein_Human_Kidney_tiny/Xenium_V1_Protein_Human_Kidney_tiny_outs.zip", + archive_name="Xenium_V1_Protein_Human_Kidney_tiny_outs.zip", + extracted_dir="Xenium_V1_Protein_Human_Kidney_tiny_outs", + source="10x Genomics Xenium 4.0.0, CC BY 4.0", + ), + TestDataset( + key="visium_hd_tiny", + group="visium_hd", + url="https://cf.10xgenomics.com/samples/spatial-exp/4.0.1/Visium_HD_Tiny_3prime_Dataset/Visium_HD_Tiny_3prime_Dataset_outs.zip", + archive_name="Visium_HD_Tiny_3prime_Dataset_outs.zip", + extracted_dir="Visium_HD_Tiny_3prime_Dataset_outs", + source="10x Genomics Visium HD 4.0.1, CC BY 4.0", + ), + TestDataset( + key="seqfish", + group="seqfish", + url="https://s3.embl.de/spatialdata/raw_data/seqfish-2-test-dataset.zip", + archive_name="seqfish-2-test-dataset.zip", + extracted_dir="seqfish-2-test-dataset", + source="Spatial Genomics seqFISH v2, public test data", + test_path="instrument 2 official", + ), + TestDataset( + key="macsima_omap23", + group="macsima", + url="https://zenodo.org/api/records/18196452/files-archive", + archive_name="OMAP23_small.zip", + extracted_dir="OMAP23_small", + source="MACSima OMAP23, CC BY 4.0", + ), + TestDataset( + key="macsima_omap10", + group="macsima", + url="https://zenodo.org/api/records/18196366/files-archive", + archive_name="OMAP10_small.zip", + extracted_dir="OMAP10_small", + source="MACSima OMAP10, CC BY 4.0", + ), +) + + +@cache +def _datasets_by_key() -> dict[str, TestDataset]: + return {dataset.key: dataset for dataset in DATASETS} + + +def get_dataset(key: str) -> TestDataset: + """Return the dataset registered for ``key``. + + Raises + ------ + KeyError + If ``key`` is not a registered dataset key. + """ + try: + return _datasets_by_key()[key] + except KeyError as exc: + available = ", ".join(sorted(_datasets_by_key())) + raise KeyError(f"Unknown test dataset key {key!r}. Available keys: {available}") from exc + + +def datasets_by_group(group: str) -> tuple[TestDataset, ...]: + """Return datasets registered for ``group``.""" + return tuple(dataset for dataset in DATASETS if dataset.group == group) diff --git a/tests/_utils.py b/tests/_utils.py deleted file mode 100644 index 5087b900..00000000 --- a/tests/_utils.py +++ /dev/null @@ -1,30 +0,0 @@ -import sys - -import pytest - - -def skip_if_below_python_version() -> pytest.mark.skipif: - """Decorator to skip tests if the Python version is below a specified version. - - This decorator prevents running tests on unsupported Python versions. Update the `MIN_VERSION` - constant to change the minimum Python version required for the tests. - - Returns - ------- - pytest.mark.skipif - A pytest marker that skips the test if the current Python version is below the specified `MIN_VERSION`. - - Notes - ----- - The current minimum version is set to Python 3.10. Adjust the `MIN_VERSION` constant as needed - to accommodate newer Python versions. - - Examples - -------- - >>> @skip_if_below_python_version() - >>> def test_some_feature(): - >>> assert True - """ - MIN_VERSION = (3, 13) - reason = f"Test requires Python {'.'.join(map(str, MIN_VERSION))} or higher" - return pytest.mark.skipif(sys.version_info < MIN_VERSION, reason=reason) diff --git a/tests/conftest.py b/tests/conftest.py index 5e4c26ad..54fe4686 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,76 @@ +from __future__ import annotations + +import importlib.util +import os +import sys +from pathlib import Path +from typing import TYPE_CHECKING, cast + import pytest from click.testing import CliRunner +if TYPE_CHECKING: + from collections.abc import Callable + from types import ModuleType + + from download_test_data_datasets import TestDataset as TestDatasetType + + +def _load_dataset_manifest() -> ModuleType: + manifest_path = Path(__file__).parents[1] / "scripts" / "download_test_data_datasets.py" + spec = importlib.util.spec_from_file_location("download_test_data_datasets", manifest_path) + if spec is None or spec.loader is None: + raise RuntimeError(f"Could not import {manifest_path}") + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +_DATASET_MANIFEST = _load_dataset_manifest() + @pytest.fixture def runner() -> CliRunner: return CliRunner() + + +@pytest.fixture(scope="session") +def test_data_dir() -> Path: + """Return the directory containing optional test datasets.""" + return Path(os.environ.get("SPATIALDATA_IO_TEST_DATA_DIR", "data")) + + +@pytest.fixture +def require_test_dataset(test_data_dir: Path) -> Callable[[str], Path]: + """Return a dataset path or skip the test if the dataset is unavailable.""" + + def _require_test_dataset(dataset_key: str) -> Path: + dataset = cast("TestDatasetType", _DATASET_MANIFEST.get_dataset(dataset_key)) + path = test_data_dir / dataset.extracted_dir + if dataset.test_path: + path /= dataset.test_path + if not path.is_dir(): + pytest.skip( + f"Test data for {dataset_key!r} not found at {path!s}. " + f"Download it with `uv run python scripts/download_test_data.py --dataset {dataset_key}` or set " + "SPATIALDATA_IO_TEST_DATA_DIR." + ) + return path + + return _require_test_dataset + + +def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: + """Apply tier markers from the test path.""" + for item in items: + path = Path(str(item.fspath)) + parts = path.parts + if "unit" in parts: + item.add_marker(pytest.mark.unit) + if "integration" in parts: + item.add_marker(pytest.mark.integration) + if "cli" in parts or "cli" in item.name: + item.add_marker(pytest.mark.cli) + if "require_test_dataset" in getattr(item, "fixturenames", ()): + item.add_marker(pytest.mark.data) diff --git a/tests/test_generic.py b/tests/integration/readers/test_generic.py similarity index 100% rename from tests/test_generic.py rename to tests/integration/readers/test_generic.py diff --git a/tests/integration/readers/test_macsima.py b/tests/integration/readers/test_macsima.py new file mode 100644 index 00000000..9c1c598c --- /dev/null +++ b/tests/integration/readers/test_macsima.py @@ -0,0 +1,295 @@ +import math +import os +import shutil +from collections.abc import Callable +from pathlib import Path +from tempfile import TemporaryDirectory +from typing import Any + +import numpy as np +import pandas as pd +import pytest +from click.testing import CliRunner +from spatialdata import read_zarr +from spatialdata.models import get_channel_names +from tifffile import imwrite + +from spatialdata_io.__main__ import macsima_wrapper +from spatialdata_io.readers.macsima import macsima + + +def test_images_with_invalid_ome_metadata_are_excluded( + tmp_path: Path, require_test_dataset: Callable[[str], Path] +) -> None: + # Write a tiff file without metadata + # Use same dimensions as OMAP10_small, which we will use as a positive example + height = 77 + width = 94 + arr = np.zeros((height, width, 1), dtype=np.uint16) + path_no_metadata = Path(tmp_path) / "tiff_no_metadata.tiff" + imwrite(path_no_metadata, arr, metadata=None, description=None, software=None, datetime=None) + + # Copy 1 image from OMAP10 small + omap_10_image_path = ( + require_test_dataset("macsima_omap10") / "C-001_S-000_S_APC_R-01_W-C-1_ROI-01_A-CD15_C-VIMC6.tif" + ) + shutil.copy(omap_10_image_path, Path(tmp_path)) + + sdata = macsima(tmp_path) + el = sdata[list(sdata.images.keys())[0]] + channels = get_channel_names(el) + assert channels == ["CD15"] + + +def test_multiple_subfolder_parsing_skips_emtpy_folders( + tmp_path: Path, require_test_dataset: Callable[[str], Path] +) -> None: + parent_folder = tmp_path / "test_folder" + shutil.copytree(require_test_dataset("macsima_omap23"), parent_folder / "OMAP23_small") + os.makedirs(parent_folder / "empty_folder") + + with pytest.warns(UserWarning, match="No tif files found in .* skipping it"): + sdata = macsima(parent_folder, parsing_style="processed_multiple_folders") + assert len(sdata.images.keys()) == 1 + + +@pytest.mark.parametrize( + "dataset,expected", + [ + pytest.param("macsima_omap10", {"y": (0, 77), "x": (0, 94)}, id="macsima_omap10"), + pytest.param("macsima_omap23", {"y": (0, 77), "x": (0, 93)}, id="macsima_omap23"), + ], +) +def test_image_size(dataset: str, expected: dict[str, Any], require_test_dataset: Callable[[str], Path]) -> None: + from spatialdata import get_extent + + f = require_test_dataset(dataset) + sdata = macsima(f, transformations=False) # Do not transform to make it easier to compare against pixel dimensions + el = sdata[list(sdata.images.keys())[0]] + cs = sdata.coordinate_systems[0] + + extent: dict[str, tuple[float, float]] = get_extent(el, coordinate_system=cs) + extent = {ax: (math.floor(extent[ax][0]), math.ceil(extent[ax][1])) for ax in extent} + assert extent == expected + + +@pytest.mark.parametrize( + "dataset,expected", + [ + pytest.param("macsima_omap10", 4, id="macsima_omap10"), + pytest.param("macsima_omap23", 5, id="macsima_omap23"), + ], +) +def test_total_channels(dataset: str, expected: int, require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) + sdata = macsima(f) + el = sdata[list(sdata.images.keys())[0]] + + # get the number of channels + channels: int = len(get_channel_names(el)) + assert channels == expected + + +@pytest.mark.parametrize( + "dataset,expected", + [ + pytest.param("macsima_omap10", ["R1 CD15", "R1 DAPI", "R2 Bcl 2", "R2 CD1c"], id="macsima_omap10"), + pytest.param( + "macsima_omap23", + ["R1 CD3", "R1 DAPI", "R2 CD279", "R4 CD66b", "R15 DAPI_background"], + id="macsima_omap23", + ), + ], +) +def test_channel_names_with_cycle_in_name( + dataset: str, expected: list[str], require_test_dataset: Callable[[str], Path] +) -> None: + f = require_test_dataset(dataset) + sdata = macsima(f, include_cycle_in_channel_name=True) + el = sdata[list(sdata.images.keys())[0]] + + # get the channel names + channels = get_channel_names(el) + assert list(channels) == expected + + +@pytest.mark.parametrize( + "dataset,expected", + [ + pytest.param("macsima_omap10", 2, id="macsima_omap10"), + pytest.param("macsima_omap23", 15, id="macsima_omap23"), + ], +) +def test_total_rounds(dataset: str, expected: list[int], require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) + sdata = macsima(f) + table = sdata[list(sdata.tables)[0]] + max_cycle = table.var["cycle"].max() + assert max_cycle == expected + + +@pytest.mark.parametrize( + "dataset,skip_rounds,expected", + [ + pytest.param("macsima_omap10", list(range(2, 4)), ["CD15", "DAPI"], id="macsima_omap10"), + pytest.param( + "macsima_omap23", + list(range(2, 16)), + ["CD3", "DAPI"], + id="macsima_omap23", + ), + ], +) +def test_skip_rounds( + dataset: str, skip_rounds: list[int], expected: list[str], require_test_dataset: Callable[[str], Path] +) -> None: + f = require_test_dataset(dataset) + sdata = macsima(f, skip_rounds=skip_rounds) + el = sdata[list(sdata.images.keys())[0]] + + # get the channel names + channels = get_channel_names(el) + assert list(channels) == expected, f"Expected {expected}, got {list(channels)}" + + +def test_processed_single_folder_parsing_returns_a_single_image_stack( + tmp_path: Path, require_test_dataset: Callable[[str], Path] +) -> None: + omap10_path = require_test_dataset("macsima_omap10") + shutil.copytree(omap10_path, tmp_path / "OMAP10_small_1") + shutil.copytree(omap10_path, tmp_path / "OMAP10_small_2") + + sdata = macsima(tmp_path, parsing_style="processed_single_folder") + + assert len(sdata.images) == 1 + # omap10_small has 4 channels, so we expect 8 here + el = sdata[list(sdata.images.keys())[0]] + assert len(get_channel_names(el)) == 8 + assert len(sdata.tables) == 1 + + +def test_processed_single_folder_parsing_warns_when_specifying_filtered_folders( + tmp_path: Path, require_test_dataset: Callable[[str], Path] +) -> None: + omap10_path = require_test_dataset("macsima_omap10") + shutil.copytree(omap10_path, tmp_path / "OMAP10_small_1") + shutil.copytree(omap10_path, tmp_path / "OMAP10_small_2") + with pytest.warns(UserWarning, match="filtering only happens for processed_multi_folders"): + macsima(tmp_path, parsing_style="processed_single_folder", filter_folder_names=["OMAP10_small_2"]) + + +def test_processed_multiple_folders_returns_an_image_stack_per_subfolder( + tmp_path: Path, require_test_dataset: Callable[[str], Path] +) -> None: + omap10_path = require_test_dataset("macsima_omap10") + shutil.copytree(omap10_path, tmp_path / "OMAP10_small_1") + shutil.copytree(omap10_path, tmp_path / "OMAP10_small_2") + + sdata = macsima(tmp_path, parsing_style="processed_multiple_folders") + + assert len(sdata.images) == 2 + for el in sdata.images.keys(): + assert len(get_channel_names(sdata[el])) == 4 + assert len(sdata.tables) == 2 + + +def test_processed_multiple_folders_skips_filtered_folder_names( + tmp_path: Path, require_test_dataset: Callable[[str], Path] +) -> None: + shutil.copytree(require_test_dataset("macsima_omap10"), tmp_path / "OMAP10_small") + shutil.copytree(require_test_dataset("macsima_omap23"), tmp_path / "OMAP23_small") + + sdata = macsima(tmp_path, parsing_style="processed_multiple_folders", filter_folder_names=["OMAP10_small"]) + assert len(sdata.images) == 1 + assert list(sdata.images.keys()) == ["OMAP23_small_image"] + assert len(sdata.tables) == 1 + assert list(sdata.tables.keys()) == ["OMAP23_small_table"] + + +METADATA_COLUMN_ORDER = [ + "cycle", + "imagetype", + "well", + "ROI", + "fluorophore", + "clone", + "exposure", +] + +EXPECTED_METADATA_OMAP10 = pd.DataFrame( + { + "name": ["CD15", "DAPI", "Bcl 2", "CD1c"], + "cycle": [1, 1, 2, 2], + "imagetype": ["stain", "stain", "stain", "stain"], + "well": ["C-1", "C-1", "C-1", "C-1"], + "ROI": [1, 1, 1, 1], + "fluorophore": ["APC", "DAPI", "FITC", "PE"], + "clone": ["VIMC6", pd.NA, "REA872", "REA694"], + "exposure": [2304.0, 40.0, 96.0, 144.0], + }, + index=["CD15", "DAPI", "Bcl 2", "CD1c"], + columns=METADATA_COLUMN_ORDER, +) + +EXPECTED_METADATA_OMAP23 = pd.DataFrame( + { + "name": ["CD3", "DAPI", "CD279", "CD66b", "DAPI_background"], + "cycle": [1, 1, 2, 4, 15], + "imagetype": ["stain", "stain", "stain", "stain", "bleach"], + "well": ["D01", "D01", "D01", "D01", "D01"], + "ROI": [1, 1, 1, 1, 1], + "fluorophore": ["APC", "DAPI", "PE", "FITC", "DAPI"], + "clone": ["REA1151", pd.NA, "REA1165", "REA306", pd.NA], + "exposure": [1212.52, 51.0, 322.12, 856.68, 51.0], + }, + index=["CD3", "DAPI", "CD279", "CD66b", "DAPI_background"], + columns=METADATA_COLUMN_ORDER, +) + + +@pytest.mark.parametrize( + "dataset,expected_df", + [ + pytest.param("macsima_omap10", EXPECTED_METADATA_OMAP10, id="macsima_omap10"), + pytest.param("macsima_omap23", EXPECTED_METADATA_OMAP23, id="macsima_omap23"), + ], +) +def test_metadata_table(dataset: str, expected_df: pd.DataFrame, require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) + sdata = macsima(f) + table = sdata[list(sdata.tables.keys())[0]] + + # Convert table.var to a DataFrame and align to expected columns + actual = table.var[METADATA_COLUMN_ORDER] + + pd.testing.assert_frame_equal(actual, expected_df) + + +@pytest.mark.parametrize( + "dataset", + [ + pytest.param("macsima_omap10", id="macsima_omap10"), + pytest.param("macsima_omap23", id="macsima_omap23"), + ], +) +def test_cli_macsima(runner: CliRunner, dataset: str, require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) + with TemporaryDirectory() as tmpdir: + output_zarr = Path(tmpdir) / "data.zarr" + result = runner.invoke( + macsima_wrapper, + [ + "--input", + f, + "--output", + output_zarr, + "--subset", + 500, + "--c-subset", + 1, + "--multiscale", + False, + ], + ) + assert result.exit_code == 0, result.output + _ = read_zarr(output_zarr) diff --git a/tests/test_seqfish.py b/tests/integration/readers/test_seqfish.py similarity index 70% rename from tests/test_seqfish.py rename to tests/integration/readers/test_seqfish.py index b71bfa6a..7b51c6d8 100644 --- a/tests/test_seqfish.py +++ b/tests/integration/readers/test_seqfish.py @@ -1,4 +1,5 @@ import math +from collections.abc import Callable from pathlib import Path from tempfile import TemporaryDirectory @@ -8,20 +9,24 @@ from spatialdata_io.__main__ import seqfish_wrapper from spatialdata_io.readers.seqfish import seqfish -from tests._utils import skip_if_below_python_version # See https://github.com/scverse/spatialdata-io/blob/main/.github/workflows/prepare_test_data.yaml for instructions on # how to download and place the data on disk -@skip_if_below_python_version() @pytest.mark.parametrize( - "dataset,expected", [("seqfish-2-test-dataset/instrument 2 official", "{'y': (0, 108), 'x': (0, 108)}")] + "dataset,expected", + [pytest.param("seqfish", "{'y': (0, 108), 'x': (0, 108)}", id="seqfish")], ) @pytest.mark.parametrize("rois", [["Roi1"], None]) @pytest.mark.parametrize("cells_as_circles", [False, True]) -def test_example_data(dataset: str, expected: str, rois: list[int] | None, cells_as_circles: bool) -> None: - f = Path("./data") / dataset - assert f.is_dir() +def test_example_data( + dataset: str, + expected: str, + rois: list[int] | None, + cells_as_circles: bool, + require_test_dataset: Callable[[str], Path], +) -> None: + f = require_test_dataset(dataset) sdata = seqfish(f, cells_as_circles=cells_as_circles, rois=rois) from spatialdata import get_extent @@ -34,11 +39,9 @@ def test_example_data(dataset: str, expected: str, rois: list[int] | None, cells assert str(extent) == expected -@skip_if_below_python_version() -@pytest.mark.parametrize("dataset", ["seqfish-2-test-dataset/instrument 2 official"]) -def test_cli_seqfish(runner: CliRunner, dataset: str) -> None: - f = Path("./data") / dataset - assert f.is_dir() +@pytest.mark.parametrize("dataset", [pytest.param("seqfish", id="seqfish")]) +def test_cli_seqfish(runner: CliRunner, dataset: str, require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) with TemporaryDirectory() as tmpdir: output_zarr = Path(tmpdir) / "data.zarr" result = runner.invoke( diff --git a/tests/test_visium_hd.py b/tests/integration/readers/test_visium_hd.py similarity index 72% rename from tests/test_visium_hd.py rename to tests/integration/readers/test_visium_hd.py index 41dfd890..5104f48e 100644 --- a/tests/test_visium_hd.py +++ b/tests/integration/readers/test_visium_hd.py @@ -1,8 +1,8 @@ import math +from collections.abc import Callable from pathlib import Path from tempfile import TemporaryDirectory -import numpy as np import pytest from click.testing import CliRunner from spatialdata import get_extent, read_zarr @@ -10,54 +10,19 @@ from spatialdata_io.__main__ import visium_hd_wrapper from spatialdata_io._constants._constants import VisiumHDKeys -from spatialdata_io.readers.visium_hd import ( - _decompose_projective_matrix, - _projective_matrix_is_affine, - visium_hd, -) -from tests._utils import skip_if_below_python_version - -# --- UNIT TESTS FOR HELPER FUNCTIONS --- - - -def test_projective_matrix_is_affine() -> None: - """Test the affine matrix check function.""" - # An affine matrix should have [0, 0, 1] as its last row - affine_matrix = np.array([[2, 0.5, 10], [0.5, 2, 20], [0, 0, 1]]) - assert _projective_matrix_is_affine(affine_matrix) - - # A projective matrix is not affine if the last row is different - projective_matrix = np.array([[2, 0.5, 10], [0.5, 2, 20], [0.01, 0.02, 1]]) - assert not _projective_matrix_is_affine(projective_matrix) - - -def test_decompose_projective_matrix() -> None: - """Test the decomposition of a projective matrix into affine and shift components.""" - projective_matrix = np.array([[1, 2, 3], [4, 5, 6], [0.1, 0.2, 1]]) - affine, shift = _decompose_projective_matrix(projective_matrix) - - expected_affine = np.array([[1, 2, 3], [4, 5, 6], [0, 0, 1]]) - - # The affine component should be correctly extracted - assert np.allclose(affine, expected_affine) - # Recomposing the affine and shift matrices should yield the original projective matrix - assert np.allclose(affine @ shift, projective_matrix) - +from spatialdata_io.readers.visium_hd import visium_hd # --- END-TO-END TESTS ON EXAMPLE DATA --- -# This dataset name is used to locate the test data in the './data/' directory. +# This dataset key is used to locate the test data in the dataset manifest. # See https://github.com/scverse/spatialdata-io/blob/main/.github/workflows/prepare_test_data.yaml # for instructions on how to download and place the data on disk. -DATASET_FOLDER = "Visium_HD_Tiny_3prime_Dataset_outs" +DATASET_KEY = "visium_hd_tiny" DATASET_ID = "visium_hd_tiny" -@skip_if_below_python_version() -def test_visium_hd_data_extent() -> None: +def test_visium_hd_data_extent(require_test_dataset: Callable[[str], Path]) -> None: """Check the spatial extent of the loaded Visium HD data.""" - f = Path("./data") / DATASET_FOLDER - if not f.is_dir(): - pytest.skip(f"Test data not found at '{f}'. Skipping extent test.") + f = require_test_dataset(DATASET_KEY) sdata = visium_hd(f, dataset_id=DATASET_ID) extent = get_extent(sdata, exact=False, coordinate_system="visium_hd_tiny_downscaled_lowres") @@ -68,7 +33,6 @@ def test_visium_hd_data_extent() -> None: assert str(extent) == expected_extent -@skip_if_below_python_version() @pytest.mark.parametrize( "params", [ @@ -122,11 +86,9 @@ def test_visium_hd_data_extent() -> None: }, ], ) -def test_visium_hd_data_integrity(params: dict[str, bool]) -> None: +def test_visium_hd_data_integrity(params: dict[str, bool], require_test_dataset: Callable[[str], Path]) -> None: """Check the integrity of various components of the loaded SpatialData object.""" - f = Path("./data") / DATASET_FOLDER - if not f.is_dir(): - pytest.skip(f"Test data not found at '{f}'. Skipping integrity test.") + f = require_test_dataset(DATASET_KEY) sdata = visium_hd(f, dataset_id=DATASET_ID, **params) @@ -182,17 +144,13 @@ def test_visium_hd_data_integrity(params: dict[str, bool]) -> None: # --- CLI WRAPPER TEST --- -@skip_if_below_python_version() @pytest.mark.parametrize( "dataset", - ["Visium_HD_Tiny_3prime_Dataset_outs"], + [pytest.param("visium_hd_tiny", id="visium_hd_tiny")], ) -def test_cli_visium_hd(runner: CliRunner, dataset: str) -> None: +def test_cli_visium_hd(runner: CliRunner, dataset: str, require_test_dataset: Callable[[str], Path]) -> None: """Test the command-line interface for the Visium HD reader.""" - f = Path("./data") / dataset[0] - - if not f.is_dir(): - pytest.skip(f"Test data not found at '{f}'. Skipping CLI test.") + f = require_test_dataset(dataset) with TemporaryDirectory() as tmpdir: output_zarr = Path(tmpdir) / "data.zarr" diff --git a/tests/test_xenium.py b/tests/integration/readers/test_xenium.py similarity index 58% rename from tests/test_xenium.py rename to tests/integration/readers/test_xenium.py index 17903888..a5ae278a 100644 --- a/tests/test_xenium.py +++ b/tests/integration/readers/test_xenium.py @@ -1,89 +1,41 @@ import math +from collections.abc import Callable from pathlib import Path from tempfile import TemporaryDirectory import numpy as np import pytest from click.testing import CliRunner -from pytest_mock import MockerFixture from spatialdata import match_table_to_element, read_zarr -from spatialdata.models import TableModel, get_table_keys +from spatialdata.models import get_table_keys from spatialdata_io.__main__ import xenium_wrapper -from spatialdata_io.readers.xenium import ( - _cell_id_str_from_prefix_suffix_uint32_reference, - cell_id_str_from_prefix_suffix_uint32, - prefix_suffix_uint32_from_cell_id_str, - xenium, -) -from tests._utils import skip_if_below_python_version - - -def test_cell_id_str_from_prefix_suffix_uint32() -> None: - cell_id_prefix = np.array([1, 1437536272, 1437536273], dtype=np.uint32) - dataset_suffix = np.array([1, 1, 2]) - expected = np.array(["aaaaaaab-1", "ffkpbaba-1", "ffkpbabb-2"]) - - result = cell_id_str_from_prefix_suffix_uint32(cell_id_prefix, dataset_suffix) - reference = _cell_id_str_from_prefix_suffix_uint32_reference(cell_id_prefix, dataset_suffix) - assert np.array_equal(result, expected) - assert np.array_equal(reference, expected) - - -def test_cell_id_str_optimized_matches_reference() -> None: - rng = np.random.default_rng(42) - cell_id_prefix = rng.integers(0, 2**32, size=10_000, dtype=np.uint32) - dataset_suffix = rng.integers(0, 10, size=10_000) - - result = cell_id_str_from_prefix_suffix_uint32(cell_id_prefix, dataset_suffix) - reference = _cell_id_str_from_prefix_suffix_uint32_reference(cell_id_prefix, dataset_suffix) - assert np.array_equal(result, reference) - - -def test_prefix_suffix_uint32_from_cell_id_str() -> None: - cell_id_str = np.array(["aaaaaaab-1", "ffkpbaba-1", "ffkpbabb-2"]) - - cell_id_prefix, dataset_suffix = prefix_suffix_uint32_from_cell_id_str(cell_id_str) - assert np.array_equal(cell_id_prefix, np.array([1, 1437536272, 1437536273], dtype=np.uint32)) - assert np.array_equal(dataset_suffix, np.array([1, 1, 2])) - - -def test_roundtrip_with_data_limits() -> None: - # min and max values for uint32 - cell_id_prefix = np.array([0, 4294967295], dtype=np.uint32) - dataset_suffix = np.array([1, 1]) - cell_id_str = np.array(["aaaaaaaa-1", "pppppppp-1"]) - f0 = cell_id_str_from_prefix_suffix_uint32 - f1 = prefix_suffix_uint32_from_cell_id_str - assert np.array_equal(cell_id_prefix, f1(f0(cell_id_prefix, dataset_suffix))[0]) - assert np.array_equal(dataset_suffix, f1(f0(cell_id_prefix, dataset_suffix))[1]) - assert np.array_equal(cell_id_str, f0(*f1(cell_id_str))) +from spatialdata_io.readers.xenium import xenium # See https://github.com/scverse/spatialdata-io/blob/main/.github/workflows/prepare_test_data.yaml for instructions on # how to download and place the data on disk # TODO: add tests for Xenium 3.0.0 -@skip_if_below_python_version() @pytest.mark.parametrize( "dataset,expected", [ ( - "Xenium_V1_human_Breast_2fov_outs", + "xenium_breast", "{'y': (0, 3529), 'x': (0, 5792), 'z': (10, 25)}", ), ( - "Xenium_V1_human_Lung_2fov_outs", + "xenium_lung", "{'y': (0, 3553), 'x': (0, 5793), 'z': (7, 32)}", ), ( - "Xenium_V1_Protein_Human_Kidney_tiny_outs", + "xenium_protein_kidney", "{'y': (0, 6915), 'x': (0, 2963), 'z': (6, 22)}", ), ], + ids=["xenium_breast", "xenium_lung", "xenium_protein_kidney"], ) -def test_example_data_data_extent(dataset: str, expected: str) -> None: - f = Path("./data") / dataset - assert f.is_dir() +def test_example_data_data_extent(dataset: str, expected: str, require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) sdata = xenium(f, cells_as_circles=False) from spatialdata import get_extent @@ -93,17 +45,19 @@ def test_example_data_data_extent(dataset: str, expected: str) -> None: # TODO: add tests for Xenium 3.0.0 -@skip_if_below_python_version() @pytest.mark.parametrize( "dataset", - ["Xenium_V1_human_Breast_2fov_outs", "Xenium_V1_human_Lung_2fov_outs", "Xenium_V1_Protein_Human_Kidney_tiny_outs"], + [ + pytest.param("xenium_breast", id="xenium_breast"), + pytest.param("xenium_lung", id="xenium_lung"), + pytest.param("xenium_protein_kidney", id="xenium_protein_kidney"), + ], ) -def test_example_data_index_integrity(dataset: str) -> None: - f = Path("./data") / dataset - assert f.is_dir() +def test_example_data_index_integrity(dataset: str, require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) sdata = xenium(f, cells_as_circles=False) - if dataset == "Xenium_V1_human_Breast_2fov_outs": + if dataset == "xenium_breast": # fmt: off # test elements assert sdata["morphology_focus"]["scale0"]["image"].sel(c="DAPI", y=20.5, x=20.5).data.compute() == 94 @@ -130,7 +84,7 @@ def test_example_data_index_integrity(dataset: str) -> None: "aaaljapa-1", "aabhbgmg-1", ] - elif dataset == "Xenium_V1_human_Lung_2fov_outs": + elif dataset == "xenium_lung": # fmt: off # test elements assert sdata["morphology_focus"]["scale0"]["image"].sel(c="DAPI", y=0.5, x=2215.5).data.compute() == 1 @@ -158,7 +112,7 @@ def test_example_data_index_integrity(dataset: str) -> None: "aabdiein-1", ] else: - assert dataset == "Xenium_V1_Protein_Human_Kidney_tiny_outs" + assert dataset == "xenium_protein_kidney" # fmt: off # test elements assert sdata["morphology_focus"]["scale0"]["image"].sel(c="VISTA", y=2876.5, x=32.5).data.compute() == 99 @@ -188,14 +142,16 @@ def test_example_data_index_integrity(dataset: str) -> None: # TODO: add tests for Xenium 3.0.0 -@skip_if_below_python_version() @pytest.mark.parametrize( "dataset", - ["Xenium_V1_human_Breast_2fov_outs", "Xenium_V1_human_Lung_2fov_outs", "Xenium_V1_Protein_Human_Kidney_tiny_outs"], + [ + pytest.param("xenium_breast", id="xenium_breast"), + pytest.param("xenium_lung", id="xenium_lung"), + pytest.param("xenium_protein_kidney", id="xenium_protein_kidney"), + ], ) -def test_cli_xenium(runner: CliRunner, dataset: str) -> None: - f = Path("./data") / dataset - assert f.is_dir() +def test_cli_xenium(runner: CliRunner, dataset: str, require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) with TemporaryDirectory() as tmpdir: output_zarr = Path(tmpdir) / "data.zarr" result = runner.invoke( @@ -211,30 +167,28 @@ def test_cli_xenium(runner: CliRunner, dataset: str) -> None: _ = read_zarr(output_zarr) -@skip_if_below_python_version() @pytest.mark.parametrize( ( "dataset", "gex_only", ), [ - ("Xenium_V1_human_Lung_2fov_outs", False), - ("Xenium_V1_human_Lung_2fov_outs", True), - ("Xenium_V1_Human_Ovary_tiny_outs", False), - ("Xenium_V1_Human_Ovary_tiny_outs", True), - ("Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs", False), - ("Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs", True), - ("Xenium_V1_Protein_Human_Kidney_tiny_outs", False), - ("Xenium_V1_Protein_Human_Kidney_tiny_outs", True), + pytest.param("xenium_lung", False, id="xenium_lung_all_features"), + pytest.param("xenium_lung", True, id="xenium_lung_gex_only"), + pytest.param("xenium_ovary", False, id="xenium_ovary_all_features"), + pytest.param("xenium_ovary", True, id="xenium_ovary_gex_only"), + pytest.param("xenium_multicell_ovary", False, id="xenium_multicell_ovary_all_features"), + pytest.param("xenium_multicell_ovary", True, id="xenium_multicell_ovary_gex_only"), + pytest.param("xenium_protein_kidney", False, id="xenium_protein_kidney_all_features"), + pytest.param("xenium_protein_kidney", True, id="xenium_protein_kidney_gex_only"), ], ) -def test_xenium_other_feature_types(dataset: str, gex_only: bool) -> None: - f = Path("./data") / dataset - assert f.is_dir() +def test_xenium_other_feature_types(dataset: str, gex_only: bool, require_test_dataset: Callable[[str], Path]) -> None: + f = require_test_dataset(dataset) sdata = xenium(f, cells_as_circles=False, gex_only=gex_only) if gex_only: assert set(sdata["table"].var["feature_types"]) == {"Gene Expression"} - elif dataset == "Xenium_V1_human_Lung_2fov_outs": + elif dataset == "xenium_lung": assert set(sdata["table"].var["feature_types"]) == { "Deprecated Codeword", "Gene Expression", @@ -242,7 +196,7 @@ def test_xenium_other_feature_types(dataset: str, gex_only: bool) -> None: "Negative Control Probe", "Unassigned Codeword", } - elif dataset in {"Xenium_V1_Human_Ovary_tiny_outs", "Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs"}: + elif dataset in {"xenium_ovary", "xenium_multicell_ovary"}: assert set(sdata["table"].var["feature_types"]) == { "Gene Expression", "Genomic Control", @@ -250,7 +204,7 @@ def test_xenium_other_feature_types(dataset: str, gex_only: bool) -> None: "Negative Control Probe", "Unassigned Codeword", } - elif dataset == "Xenium_V1_Protein_Human_Kidney_tiny_outs": + elif dataset == "xenium_protein_kidney": assert set(sdata["table"].var["feature_types"]) == { "Gene Expression", "Genomic Control", @@ -270,57 +224,3 @@ def test_xenium_other_feature_types(dataset: str, gex_only: bool) -> None: else: assert ValueError(f"Unexpected dataset {dataset}") - - -# ── CLI JSON kwargs tests (no real data needed) ─────────────────────────────── - - -@pytest.mark.parametrize( - "kwarg_name", - ["--imread-kwargs", "--image-models-kwargs", "--labels-models-kwargs"], -) -def test_cli_xenium_invalid_json_rejected(runner: CliRunner, tmp_path: Path, kwarg_name: str) -> None: - """Invalid JSON for any kwargs option must produce a non-zero exit and a clear error.""" - result = runner.invoke( - xenium_wrapper, - [ - "--input", - str(tmp_path), - "--output", - str(tmp_path / "out.zarr"), - kwarg_name, - "not-valid-json{", - ], - ) - assert result.exit_code != 0 - assert "Invalid JSON" in result.output - - -@pytest.mark.parametrize( - ("kwarg_name", "kwarg_param"), - [ - ("--imread-kwargs", "imread_kwargs"), - ("--image-models-kwargs", "image_models_kwargs"), - ("--labels-models-kwargs", "labels_models_kwargs"), - ], -) -def test_cli_xenium_valid_json_forwarded( - runner: CliRunner, tmp_path: Path, mocker: MockerFixture, kwarg_name: str, kwarg_param: str -) -> None: - """Valid JSON kwargs must be parsed and forwarded to the xenium reader as a dict.""" - mock_xenium = mocker.patch("spatialdata_io.readers.xenium.xenium") - mock_xenium.return_value = mocker.MagicMock() - result = runner.invoke( - xenium_wrapper, - [ - "--input", - str(tmp_path), - "--output", - str(tmp_path / "out.zarr"), - kwarg_name, - '{"chunks": 512}', - ], - ) - assert result.exit_code == 0, result.output - call_kwargs = mock_xenium.call_args.kwargs - assert call_kwargs[kwarg_param] == {"chunks": 512} diff --git a/tests/test_cli_alignment.py b/tests/unit/cli/test_cli_alignment.py similarity index 100% rename from tests/test_cli_alignment.py rename to tests/unit/cli/test_cli_alignment.py diff --git a/tests/converters/__init__.py b/tests/unit/converters/__init__.py similarity index 100% rename from tests/converters/__init__.py rename to tests/unit/converters/__init__.py diff --git a/tests/converters/test_legacy_anndata.py b/tests/unit/converters/test_legacy_anndata.py similarity index 100% rename from tests/converters/test_legacy_anndata.py rename to tests/unit/converters/test_legacy_anndata.py diff --git a/tests/test_macsima.py b/tests/unit/readers/test_macsima.py similarity index 66% rename from tests/test_macsima.py rename to tests/unit/readers/test_macsima.py index 9be7a3c6..7f0634db 100644 --- a/tests/test_macsima.py +++ b/tests/unit/readers/test_macsima.py @@ -1,17 +1,11 @@ import contextlib -import math -import os -import shutil from copy import deepcopy from pathlib import Path -from tempfile import TemporaryDirectory from typing import Any import dask.array as da import numpy as np -import pandas as pd import pytest -from click.testing import CliRunner from ome_types import OME from ome_types.model import ( Image, @@ -26,11 +20,8 @@ StructuredAnnotations, Well, ) -from spatialdata import read_zarr -from spatialdata.models import get_channel_names from tifffile import imwrite -from spatialdata_io.__main__ import macsima_wrapper from spatialdata_io.readers.macsima import ( ChannelMetadata, MultiChannelImage, @@ -46,14 +37,6 @@ RNG = da.random.default_rng(seed=0) -if not (Path("./data/OMAP10_small").exists() or Path("./data/OMAP23_small").exists()): - pytest.skip( - "Requires the OMAP10 or OMAP23 datasets. " - "The small OMAP10 dataset can be downloaded from https://zenodo.org/api/records/18196366/files-archive, for the full data see https://zenodo.org/records/7875938" - "The small OMAP23 dataset can be downloaded from https://zenodo.org/api/records/18196452/files-archive, for the full data set see https://zenodo.org/records/14008816", - allow_module_level=True, - ) - # Helper to create ChannelMetadata with some defaults def make_ChannelMetadata( @@ -81,25 +64,6 @@ def make_ChannelMetadata( ) -def test_images_with_invalid_ome_metadata_are_excluded(tmp_path: Path) -> None: - # Write a tiff file without metadata - # Use same dimensions as OMAP10_small, which we will use as a positive example - height = 77 - width = 94 - arr = np.zeros((height, width, 1), dtype=np.uint16) - path_no_metadata = Path(tmp_path) / "tiff_no_metadata.tiff" - imwrite(path_no_metadata, arr, metadata=None, description=None, software=None, datetime=None) - - # Copy 1 image from OMAP10 small - omap_10_image_path = Path("./data") / "OMAP10_small" / "C-001_S-000_S_APC_R-01_W-C-1_ROI-01_A-CD15_C-VIMC6.tif" - shutil.copy(omap_10_image_path, Path(tmp_path)) - - sdata = macsima(tmp_path) - el = sdata[list(sdata.images.keys())[0]] - channels = get_channel_names(el) - assert channels == ["CD15"] - - def test_exception_on_no_valid_files(tmp_path: Path) -> None: # Write a tiff file without metadata height = 10 @@ -112,16 +76,6 @@ def test_exception_on_no_valid_files(tmp_path: Path) -> None: macsima(tmp_path) -def test_multiple_subfolder_parsing_skips_emtpy_folders(tmp_path: Path) -> None: - parent_folder = tmp_path / "test_folder" - shutil.copytree("./data/OMAP23_small", parent_folder / "OMAP23_small") - os.makedirs(parent_folder / "empty_folder") - - with pytest.warns(UserWarning, match="No tif files found in .* skipping it"): - sdata = macsima(parent_folder, parsing_style="processed_multiple_folders") - assert len(sdata.images.keys()) == 1 - - @pytest.mark.parametrize( "dimensions,expected", [ @@ -173,9 +127,7 @@ def test_padding_on_differing_dimensions() -> None: for height, width in zip(heights, widths, strict=True): arr = da.from_array(np.ones((1, height, width), dtype=np.uint16)) imgs.append(arr) - channel_metadata = channel_metadata = [ - make_ChannelMetadata(name="test", cycle=1, translation_x=100, translation_y=100) - ] * 4 + channel_metadata = [make_ChannelMetadata(name="test", cycle=1, translation_x=100, translation_y=100)] * 4 with pytest.warns(UserWarning, match="Padding images with 0s to same size of \\(20, 20\\)"): imgs_padded = MultiChannelImage._pad_images(imgs, channel_metadata) for img in imgs_padded: @@ -190,7 +142,7 @@ def test_padding_on_differing_dimensions() -> None: for height, width in zip(heights, widths, strict=True): arr = da.from_array(np.ones((1, height, width), dtype=np.uint16)) imgs.append(arr) - channel_metadata = channel_metadata = [ + channel_metadata = [ make_ChannelMetadata(name="test", cycle=1, translation_x=2, translation_y=3), make_ChannelMetadata(name="test", cycle=1, translation_x=0, translation_y=0), ] @@ -210,7 +162,7 @@ def test_padding_on_differing_dimensions() -> None: for height, width in zip(heights, widths, strict=True): arr = da.from_array(np.ones((1, height, width), dtype=np.uint16)) imgs.append(arr) - channel_metadata = channel_metadata = [ + channel_metadata = [ make_ChannelMetadata(name="test", cycle=1, translation_x=2, translation_y=3), make_ChannelMetadata(name="test", cycle=1, translation_x=5, translation_y=5), ] @@ -220,212 +172,11 @@ def test_padding_on_differing_dimensions() -> None: assert img.shape == (1, 17, 18) -@pytest.mark.parametrize( - "dataset,expected", - [ - ("OMAP10_small", {"y": (0, 77), "x": (0, 94)}), - ("OMAP23_small", {"y": (0, 77), "x": (0, 93)}), - ], -) -def test_image_size(dataset: str, expected: dict[str, Any]) -> None: - from spatialdata import get_extent - - f = Path("./data") / dataset - assert f.is_dir() - sdata = macsima(f, transformations=False) # Do not transform to make it easier to compare against pixel dimensions - el = sdata[list(sdata.images.keys())[0]] - cs = sdata.coordinate_systems[0] - - extent: dict[str, tuple[float, float]] = get_extent(el, coordinate_system=cs) - extent = {ax: (math.floor(extent[ax][0]), math.ceil(extent[ax][1])) for ax in extent} - assert extent == expected - - -@pytest.mark.parametrize( - "dataset,expected", - [("OMAP10_small", 4), ("OMAP23_small", 5)], -) -def test_total_channels(dataset: str, expected: int) -> None: - f = Path("./data") / dataset - assert f.is_dir() - sdata = macsima(f) - el = sdata[list(sdata.images.keys())[0]] - - # get the number of channels - channels: int = len(get_channel_names(el)) - assert channels == expected - - -@pytest.mark.parametrize( - "dataset,expected", - [ - ("OMAP10_small", ["R1 CD15", "R1 DAPI", "R2 Bcl 2", "R2 CD1c"]), - ( - "OMAP23_small", - ["R1 CD3", "R1 DAPI", "R2 CD279", "R4 CD66b", "R15 DAPI_background"], - ), - ], -) -def test_channel_names_with_cycle_in_name(dataset: str, expected: list[str]) -> None: - f = Path("./data") / dataset - assert f.is_dir() - sdata = macsima(f, include_cycle_in_channel_name=True) - el = sdata[list(sdata.images.keys())[0]] - - # get the channel names - channels = get_channel_names(el) - assert list(channels) == expected - - -@pytest.mark.parametrize( - "dataset,expected", - [ - ("OMAP10_small", 2), - ("OMAP23_small", 15), - ], -) -def test_total_rounds(dataset: str, expected: list[int]) -> None: - f = Path("./data") / dataset - assert f.is_dir() - sdata = macsima(f) - table = sdata[list(sdata.tables)[0]] - max_cycle = table.var["cycle"].max() - assert max_cycle == expected - - -@pytest.mark.parametrize( - "dataset,skip_rounds,expected", - [ - ("OMAP10_small", list(range(2, 4)), ["CD15", "DAPI"]), - ( - "OMAP23_small", - list(range(2, 16)), - ["CD3", "DAPI"], - ), - ], -) -def test_skip_rounds(dataset: str, skip_rounds: list[int], expected: list[str]) -> None: - f = Path("./data") / dataset - assert f.is_dir() - sdata = macsima(f, skip_rounds=skip_rounds) - el = sdata[list(sdata.images.keys())[0]] - - # get the channel names - channels = get_channel_names(el) - assert list(channels) == expected, f"Expected {expected}, got {list(channels)}" - - def test_unsupported_parsing_styles() -> None: with pytest.raises(ValueError, match="Invalid option `not_a_parsing_style` for `MACSimaParsingStyle`."): macsima(Path(), parsing_style="not_a_parsing_style") -def test_processed_single_folder_parsing_returns_a_single_image_stack(tmp_path: Path) -> None: - omap10_path = Path("./data/OMAP10_small") - shutil.copytree(omap10_path, tmp_path / "OMAP10_small_1") - shutil.copytree(omap10_path, tmp_path / "OMAP10_small_2") - - sdata = macsima(tmp_path, parsing_style="processed_single_folder") - - assert len(sdata.images) == 1 - # omap10_small has 4 channels, so we expect 8 here - el = sdata[list(sdata.images.keys())[0]] - assert len(get_channel_names(el)) == 8 - assert len(sdata.tables) == 1 - - -def test_processed_single_folder_parsing_warns_when_specifying_filtered_folders(tmp_path: Path) -> None: - omap10_path = Path("./data/OMAP10_small") - shutil.copytree(omap10_path, tmp_path / "OMAP10_small_1") - shutil.copytree(omap10_path, tmp_path / "OMAP10_small_2") - with pytest.warns(UserWarning, match="filtering only happens for processed_multi_folders"): - macsima(tmp_path, parsing_style="processed_single_folder", filter_folder_names=["OMAP10_small_2"]) - - -def test_processed_multiple_folders_returns_an_image_stack_per_subfolder(tmp_path: Path) -> None: - omap10_path = Path("./data/OMAP10_small") - shutil.copytree(omap10_path, tmp_path / "OMAP10_small_1") - shutil.copytree(omap10_path, tmp_path / "OMAP10_small_2") - - sdata = macsima(tmp_path, parsing_style="processed_multiple_folders") - - assert len(sdata.images) == 2 - for el in sdata.images.keys(): - assert len(get_channel_names(sdata[el])) == 4 - assert len(sdata.tables) == 2 - - -def test_processed_multiple_folders_skips_filtered_folder_names(tmp_path: Path) -> None: - shutil.copytree(Path("./data/OMAP10_small"), tmp_path / "OMAP10_small") - shutil.copytree(Path("./data/OMAP23_small"), tmp_path / "OMAP23_small") - - sdata = macsima(tmp_path, parsing_style="processed_multiple_folders", filter_folder_names=["OMAP10_small"]) - assert len(sdata.images) == 1 - assert list(sdata.images.keys()) == ["OMAP23_small_image"] - assert len(sdata.tables) == 1 - assert list(sdata.tables.keys()) == ["OMAP23_small_table"] - - -METADATA_COLUMN_ORDER = [ - "cycle", - "imagetype", - "well", - "ROI", - "fluorophore", - "clone", - "exposure", -] - -EXPECTED_METADATA_OMAP10 = pd.DataFrame( - { - "name": ["CD15", "DAPI", "Bcl 2", "CD1c"], - "cycle": [1, 1, 2, 2], - "imagetype": ["stain", "stain", "stain", "stain"], - "well": ["C-1", "C-1", "C-1", "C-1"], - "ROI": [1, 1, 1, 1], - "fluorophore": ["APC", "DAPI", "FITC", "PE"], - "clone": ["VIMC6", pd.NA, "REA872", "REA694"], - "exposure": [2304.0, 40.0, 96.0, 144.0], - }, - index=["CD15", "DAPI", "Bcl 2", "CD1c"], - columns=METADATA_COLUMN_ORDER, -) - -EXPECTED_METADATA_OMAP23 = pd.DataFrame( - { - "name": ["CD3", "DAPI", "CD279", "CD66b", "DAPI_background"], - "cycle": [1, 1, 2, 4, 15], - "imagetype": ["stain", "stain", "stain", "stain", "bleach"], - "well": ["D01", "D01", "D01", "D01", "D01"], - "ROI": [1, 1, 1, 1, 1], - "fluorophore": ["APC", "DAPI", "PE", "FITC", "DAPI"], - "clone": ["REA1151", pd.NA, "REA1165", "REA306", pd.NA], - "exposure": [1212.52, 51.0, 322.12, 856.68, 51.0], - }, - index=["CD3", "DAPI", "CD279", "CD66b", "DAPI_background"], - columns=METADATA_COLUMN_ORDER, -) - - -@pytest.mark.parametrize( - "dataset,expected_df", - [ - ("OMAP10_small", EXPECTED_METADATA_OMAP10), - ("OMAP23_small", EXPECTED_METADATA_OMAP23), - ], -) -def test_metadata_table(dataset: str, expected_df: pd.DataFrame) -> None: - f = Path("./data") / dataset - assert f.is_dir() - sdata = macsima(f) - table = sdata[list(sdata.tables.keys())[0]] - - # Convert table.var to a DataFrame and align to expected columns - actual = table.var[METADATA_COLUMN_ORDER] - - pd.testing.assert_frame_equal(actual, expected_df) - - def test_mci_sort_by_channel() -> None: sizes = [100, 200, 300] c_names = ["test11", "test3", "test2"] @@ -474,31 +225,6 @@ def test_mci_array_reference() -> None: assert da.all(mci.data[0] == orig_arr1) -@pytest.mark.parametrize("dataset", ["OMAP10_small", "OMAP23_small"]) -def test_cli_macsima(runner: CliRunner, dataset: str) -> None: - f = Path("./data") / dataset - assert f.is_dir() - with TemporaryDirectory() as tmpdir: - output_zarr = Path(tmpdir) / "data.zarr" - result = runner.invoke( - macsima_wrapper, - [ - "--input", - f, - "--output", - output_zarr, - "--subset", - 500, - "--c-subset", - 1, - "--multiscale", - False, - ], - ) - assert result.exit_code == 0, result.output - _ = read_zarr(output_zarr) - - def test_collect_map_annotation_values_with_no_duplicate_keys() -> None: ome = OME( structured_annotations=StructuredAnnotations( @@ -538,8 +264,6 @@ def test_collect_map_annotations_values_with_duplicate_keys_different_values() - ] ) ) - import re - result = _collect_map_annotation_values(ome) # The parser should return only the first found value. diff --git a/tests/readers/test_utils_image.py b/tests/unit/readers/test_utils_image.py similarity index 100% rename from tests/readers/test_utils_image.py rename to tests/unit/readers/test_utils_image.py diff --git a/tests/unit/readers/test_visium_hd.py b/tests/unit/readers/test_visium_hd.py new file mode 100644 index 00000000..677ab5d0 --- /dev/null +++ b/tests/unit/readers/test_visium_hd.py @@ -0,0 +1,32 @@ +import numpy as np + +from spatialdata_io.readers.visium_hd import ( + _decompose_projective_matrix, + _projective_matrix_is_affine, +) + +# --- UNIT TESTS FOR HELPER FUNCTIONS --- + + +def test_projective_matrix_is_affine() -> None: + """Test the affine matrix check function.""" + # An affine matrix should have [0, 0, 1] as its last row + affine_matrix = np.array([[2, 0.5, 10], [0.5, 2, 20], [0, 0, 1]]) + assert _projective_matrix_is_affine(affine_matrix) + + # A projective matrix is not affine if the last row is different + projective_matrix = np.array([[2, 0.5, 10], [0.5, 2, 20], [0.01, 0.02, 1]]) + assert not _projective_matrix_is_affine(projective_matrix) + + +def test_decompose_projective_matrix() -> None: + """Test the decomposition of a projective matrix into affine and shift components.""" + projective_matrix = np.array([[1, 2, 3], [4, 5, 6], [0.1, 0.2, 1]]) + affine, shift = _decompose_projective_matrix(projective_matrix) + + expected_affine = np.array([[1, 2, 3], [4, 5, 6], [0, 0, 1]]) + + # The affine component should be correctly extracted + assert np.allclose(affine, expected_affine) + # Recomposing the affine and shift matrices should yield the original projective matrix + assert np.allclose(affine @ shift, projective_matrix) diff --git a/tests/unit/readers/test_xenium.py b/tests/unit/readers/test_xenium.py new file mode 100644 index 00000000..943c63bd --- /dev/null +++ b/tests/unit/readers/test_xenium.py @@ -0,0 +1,105 @@ +from pathlib import Path + +import numpy as np +import pytest +from click.testing import CliRunner +from pytest_mock import MockerFixture + +from spatialdata_io.__main__ import xenium_wrapper +from spatialdata_io.readers.xenium import ( + _cell_id_str_from_prefix_suffix_uint32_reference, + cell_id_str_from_prefix_suffix_uint32, + prefix_suffix_uint32_from_cell_id_str, +) + + +def test_cell_id_str_from_prefix_suffix_uint32() -> None: + cell_id_prefix = np.array([1, 1437536272, 1437536273], dtype=np.uint32) + dataset_suffix = np.array([1, 1, 2]) + expected = np.array(["aaaaaaab-1", "ffkpbaba-1", "ffkpbabb-2"]) + + result = cell_id_str_from_prefix_suffix_uint32(cell_id_prefix, dataset_suffix) + reference = _cell_id_str_from_prefix_suffix_uint32_reference(cell_id_prefix, dataset_suffix) + assert np.array_equal(result, expected) + assert np.array_equal(reference, expected) + + +def test_cell_id_str_optimized_matches_reference() -> None: + rng = np.random.default_rng(42) + cell_id_prefix = rng.integers(0, 2**32, size=10_000, dtype=np.uint32) + dataset_suffix = rng.integers(0, 10, size=10_000) + + result = cell_id_str_from_prefix_suffix_uint32(cell_id_prefix, dataset_suffix) + reference = _cell_id_str_from_prefix_suffix_uint32_reference(cell_id_prefix, dataset_suffix) + assert np.array_equal(result, reference) + + +def test_prefix_suffix_uint32_from_cell_id_str() -> None: + cell_id_str = np.array(["aaaaaaab-1", "ffkpbaba-1", "ffkpbabb-2"]) + + cell_id_prefix, dataset_suffix = prefix_suffix_uint32_from_cell_id_str(cell_id_str) + assert np.array_equal(cell_id_prefix, np.array([1, 1437536272, 1437536273], dtype=np.uint32)) + assert np.array_equal(dataset_suffix, np.array([1, 1, 2])) + + +def test_roundtrip_with_data_limits() -> None: + # min and max values for uint32 + cell_id_prefix = np.array([0, 4294967295], dtype=np.uint32) + dataset_suffix = np.array([1, 1]) + cell_id_str = np.array(["aaaaaaaa-1", "pppppppp-1"]) + f0 = cell_id_str_from_prefix_suffix_uint32 + f1 = prefix_suffix_uint32_from_cell_id_str + assert np.array_equal(cell_id_prefix, f1(f0(cell_id_prefix, dataset_suffix))[0]) + assert np.array_equal(dataset_suffix, f1(f0(cell_id_prefix, dataset_suffix))[1]) + assert np.array_equal(cell_id_str, f0(*f1(cell_id_str))) + + +@pytest.mark.parametrize( + "kwarg_name", + ["--imread-kwargs", "--image-models-kwargs", "--labels-models-kwargs"], +) +def test_cli_xenium_invalid_json_rejected(runner: CliRunner, tmp_path: Path, kwarg_name: str) -> None: + """Invalid JSON for any kwargs option must produce a non-zero exit and a clear error.""" + result = runner.invoke( + xenium_wrapper, + [ + "--input", + str(tmp_path), + "--output", + str(tmp_path / "out.zarr"), + kwarg_name, + "not-valid-json{", + ], + ) + assert result.exit_code != 0 + assert "Invalid JSON" in result.output + + +@pytest.mark.parametrize( + ("kwarg_name", "kwarg_param"), + [ + ("--imread-kwargs", "imread_kwargs"), + ("--image-models-kwargs", "image_models_kwargs"), + ("--labels-models-kwargs", "labels_models_kwargs"), + ], +) +def test_cli_xenium_valid_json_forwarded( + runner: CliRunner, tmp_path: Path, mocker: MockerFixture, kwarg_name: str, kwarg_param: str +) -> None: + """Valid JSON kwargs must be parsed and forwarded to the xenium reader as a dict.""" + mock_xenium = mocker.patch("spatialdata_io.readers.xenium.xenium") + mock_xenium.return_value = mocker.MagicMock() + result = runner.invoke( + xenium_wrapper, + [ + "--input", + str(tmp_path), + "--output", + str(tmp_path / "out.zarr"), + kwarg_name, + '{"chunks": 512}', + ], + ) + assert result.exit_code == 0, result.output + call_kwargs = mock_xenium.call_args.kwargs + assert call_kwargs[kwarg_param] == {"chunks": 512} diff --git a/tests/unit/test_download_test_data.py b/tests/unit/test_download_test_data.py new file mode 100644 index 00000000..307a3bf4 --- /dev/null +++ b/tests/unit/test_download_test_data.py @@ -0,0 +1,203 @@ +from __future__ import annotations + +import importlib.util +import io +import sys +import urllib.error +import urllib.request +import zipfile +from email.message import Message +from pathlib import Path +from typing import TYPE_CHECKING, Any + +import pytest + +if TYPE_CHECKING: + from types import ModuleType + + +def _load_download_script() -> ModuleType: + module_path = Path(__file__).parents[2] / "scripts" / "download_test_data.py" + spec = importlib.util.spec_from_file_location("download_test_data", module_path) + if spec is None or spec.loader is None: + raise RuntimeError(f"Could not import {module_path}") + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +download_test_data = _load_download_script() + + +def _make_dataset(key: str = "example") -> Any: + return download_test_data.TestDataset( + key=key, + group="group", + url=f"https://example.com/{key}.zip", + archive_name=f"{key}.zip", + extracted_dir=key, + source="example", + ) + + +def _write_zip(path: Path) -> None: + with zipfile.ZipFile(path, "w") as archive: + archive.writestr("payload.txt", "ok") + + +class _BytesResponse(io.BytesIO): + def __enter__(self) -> _BytesResponse: + return self + + def __exit__(self, exc_type: object, exc: object, traceback: object) -> None: + self.close() + + +class TestDownload: + def test_uses_dataset_manifest_module(self) -> None: + assert download_test_data.TestDataset.__module__ == "download_test_data_datasets" + assert all(isinstance(dataset, download_test_data.TestDataset) for dataset in download_test_data.DATASETS) + + def test_sends_explicit_headers(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + seen_request: urllib.request.Request | None = None + + def urlopen(request: urllib.request.Request, timeout: int) -> _BytesResponse: + nonlocal seen_request + seen_request = request + assert timeout == download_test_data.DOWNLOAD_TIMEOUT_SEC + return _BytesResponse(b"payload") + + monkeypatch.setattr(download_test_data.urllib.request, "urlopen", urlopen) + + download_test_data._download(_make_dataset(), tmp_path / "archive.zip") + + assert seen_request is not None + assert seen_request.get_header("User-agent") == "curl/8.0.0" + assert seen_request.get_header("Accept") == "*/*" + + def test_reports_http_403_with_actionable_message(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + dataset = _make_dataset("forbidden") + + def urlopen(request: object, timeout: int) -> None: + raise urllib.error.HTTPError(dataset.url, 403, "Forbidden", hdrs=Message(), fp=None) + + monkeypatch.setattr(download_test_data.urllib.request, "urlopen", urlopen) + + with pytest.raises(download_test_data.DatasetDownloadError, match="HTTP 403 Forbidden") as exc_info: + download_test_data._download(dataset, tmp_path / dataset.archive_name) + + message = str(exc_info.value) + assert dataset.key in message + assert dataset.url in message + assert "rejected the request" in message + + def test_retries_transient_failures(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + dataset = _make_dataset("retry") + attempts = 0 + + def urlopen(request: object, timeout: int) -> _BytesResponse: + nonlocal attempts + attempts += 1 + if attempts == 1: + raise urllib.error.HTTPError(dataset.url, 503, "Service Unavailable", hdrs=Message(), fp=None) + return _BytesResponse(b"payload") + + monkeypatch.setattr(download_test_data.urllib.request, "urlopen", urlopen) + monkeypatch.setattr(download_test_data.time, "sleep", lambda seconds: None) + + download_test_data._download(dataset, tmp_path / dataset.archive_name) + + assert attempts == 2 + + +class TestDatasetManifest: + def test_returns_dataset_by_key(self) -> None: + dataset = download_test_data.get_dataset("seqfish") + + assert dataset.key == "seqfish" + assert dataset.extracted_dir == "seqfish-2-test-dataset" + + def test_reports_unknown_dataset_key(self) -> None: + with pytest.raises(KeyError, match="Unknown test dataset key 'missing'"): + download_test_data.get_dataset("missing") + + def test_test_path_defaults_to_extracted_directory_root(self) -> None: + dataset = download_test_data.get_dataset("xenium_breast") + + assert dataset.test_path == "" + + def test_can_filter_datasets_by_group(self) -> None: + datasets = download_test_data.datasets_by_group("macsima") + + assert {dataset.key for dataset in datasets} == {"macsima_omap10", "macsima_omap23"} + + +class TestExtractZip: + def test_reports_invalid_archive(self, tmp_path: Path) -> None: + archive_path = tmp_path / "bad.zip" + archive_path.write_bytes(b"not a zip") + + with pytest.raises(download_test_data.DatasetDownloadError, match="not a valid zip archive"): + download_test_data._extract_zip(_make_dataset(), archive_path, tmp_path / "extract") + + +class TestDownloadDataset: + def test_skips_existing_dataset_without_force( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] + ) -> None: + dataset = _make_dataset("existing") + target = tmp_path / dataset.extracted_dir + target.mkdir() + + def fail_download(dataset: object, archive_path: Path) -> None: + raise AssertionError("download should have been skipped") + + monkeypatch.setattr(download_test_data, "_download", fail_download) + + download_test_data.download_dataset(dataset, tmp_path, force=False) + + assert "Skipping existing" in capsys.readouterr().out + + def test_replaces_existing_dataset_with_force(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + dataset = _make_dataset("force") + target = tmp_path / dataset.extracted_dir + target.mkdir() + (target / "old.txt").write_text("old", encoding="utf-8") + + def write_archive(dataset: object, archive_path: Path) -> None: + _write_zip(archive_path) + + monkeypatch.setattr(download_test_data, "_download", write_archive) + + download_test_data.download_dataset(dataset, tmp_path, force=True) + + assert not (target / "old.txt").exists() + assert (target / "payload.txt").read_text(encoding="utf-8") == "ok" + + +class TestMain: + def test_continues_after_failure_then_exits_nonzero( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] + ) -> None: + first = _make_dataset("first") + second = _make_dataset("second") + attempted: list[str] = [] + + def download_dataset(dataset: Any, output: Path, force: bool) -> None: + attempted.append(dataset.key) + if dataset == first: + raise download_test_data.DatasetDownloadError(dataset, "download", "HTTP 403 Forbidden") + + monkeypatch.setattr(download_test_data, "DATASETS", (first, second)) + monkeypatch.setattr(download_test_data, "download_dataset", download_dataset) + monkeypatch.setattr(sys, "argv", ["download_test_data.py", "--output", str(tmp_path)]) + + with pytest.raises(SystemExit) as exc_info: + download_test_data.main() + + assert exc_info.value.code == 1 + assert attempted == ["first", "second"] + output = capsys.readouterr().out + assert "Failed to download 1 dataset(s)" in output + assert "first" in output diff --git a/tests/test_init.py b/tests/unit/test_init.py similarity index 100% rename from tests/test_init.py rename to tests/unit/test_init.py From 0bbe30828b898cb0ac28db6cd76f592adf1b5409 Mon Sep 17 00:00:00 2001 From: Kilian Paul <81327374+Darlokt@users.noreply.github.com> Date: Sat, 16 May 2026 05:28:24 +0200 Subject: [PATCH 2/7] Improve test structure and test marking This commit refactors the test structure further into separate subdirectories for each reader under both `tests/integration/readers/` and `tests/unit/readers/`. In preparation for further test/reader improvements/refactoring to split the monolithic readers/tests into separate modules for more modularity, maintainability, testability, and clarity. Changes: - Move reader-specific integration tests into subdirectories under `tests/integration/readers/` (e.g. `tests/integration/readers/xenium/`). - Move reader-specific unit tests into subdirectories under `tests/unit/readers/` (e.g. `tests/unit/readers/xenium/`). - Update test imports and references to reflect the new directory structure. - Added per reader pytest markers (e.g. `@pytest.mark.xenium`) to allow for more flexible test selection. - Update contributing documentation to clarify the distinction between unit and integration tests, and to specify the use of dataset keys for integration tests that require external datasets. --- docs/contributing.md | 6 ++ pyproject.toml | 15 ++++ tests/conftest.py | 27 ++++++- .../readers/{ => generic}/test_generic.py | 0 .../readers/{ => macsima}/test_macsima.py | 0 .../readers/{ => seqfish}/test_seqfish.py | 0 .../readers/{ => visium_hd}/test_visium_hd.py | 70 +++++++++++-------- .../readers/{ => xenium}/test_xenium.py | 6 +- tests/unit/cli/test_cli_alignment.py | 33 +++++---- .../readers/{ => macsima}/test_macsima.py | 0 .../readers/{ => utils}/test_utils_image.py | 0 .../readers/{ => visium_hd}/test_visium_hd.py | 0 .../unit/readers/{ => xenium}/test_xenium.py | 0 13 files changed, 109 insertions(+), 48 deletions(-) rename tests/integration/readers/{ => generic}/test_generic.py (100%) rename tests/integration/readers/{ => macsima}/test_macsima.py (100%) rename tests/integration/readers/{ => seqfish}/test_seqfish.py (100%) rename tests/integration/readers/{ => visium_hd}/test_visium_hd.py (80%) rename tests/integration/readers/{ => xenium}/test_xenium.py (97%) rename tests/unit/readers/{ => macsima}/test_macsima.py (100%) rename tests/unit/readers/{ => utils}/test_utils_image.py (100%) rename tests/unit/readers/{ => visium_hd}/test_visium_hd.py (100%) rename tests/unit/readers/{ => xenium}/test_xenium.py (100%) diff --git a/docs/contributing.md b/docs/contributing.md index b18b0ded..37e0cc5e 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -129,6 +129,8 @@ Tests are split by scope: - Integration tests live in `tests/integration/` and cover reader workflows, CLI commands, file I/O, and zarr roundtrips. - Integration tests that require external datasets use dataset keys from `scripts/download_test_data_datasets.py`. They resolve data under `SPATIALDATA_IO_TEST_DATA_DIR` when set, otherwise `data/` in the repository root. If the required dataset is unavailable, the test should skip with a clear message. +- Reader tests are marked by reader name. When modifying one reader, use `pytest -m ` to run the tests + specific to that reader, including shared parametrized checks for that reader. Useful local commands: @@ -136,6 +138,10 @@ Useful local commands: pytest tests/unit pytest tests/integration pytest -m "integration and data" +pytest -m xenium +pytest -m "xenium and data" +pytest -m "xenium and not slow" +pytest -m "xenium and cli" python scripts/download_test_data.py --group xenium SPATIALDATA_IO_TEST_DATA_DIR=/path/to/data pytest -m data ``` diff --git a/pyproject.toml b/pyproject.toml index 9f7a131e..038b5b30 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -90,6 +90,21 @@ markers = [ "data: tests that require optional downloaded test datasets", "slow: tests with comparatively high runtime", "cli: command-line interface tests", + "codex: tests for the codex reader", + "cosmx: tests for the cosmx reader", + "curio: tests for the curio reader", + "dbit: tests for the dbit reader", + "generic: tests for the generic reader module", + "iss: tests for the iss reader", + "macsima: tests for the macsima reader", + "mcmicro: tests for the mcmicro reader", + "merscope: tests for the merscope reader", + "seqfish: tests for the seqfish reader", + "steinbock: tests for the steinbock reader", + "stereoseq: tests for the stereoseq reader", + "visium: tests for the visium reader", + "visium_hd: tests for the visium_hd reader", + "xenium: tests for the xenium reader", ] [tool.ruff] diff --git a/tests/conftest.py b/tests/conftest.py index 54fe4686..eedcbe1a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -29,6 +29,24 @@ def _load_dataset_manifest() -> ModuleType: _DATASET_MANIFEST = _load_dataset_manifest() +_READER_MARKS = { + "codex", + "cosmx", + "curio", + "dbit", + "generic", + "iss", + "macsima", + "mcmicro", + "merscope", + "seqfish", + "steinbock", + "stereoseq", + "visium", + "visium_hd", + "xenium", +} + @pytest.fixture def runner() -> CliRunner: @@ -47,9 +65,9 @@ def require_test_dataset(test_data_dir: Path) -> Callable[[str], Path]: def _require_test_dataset(dataset_key: str) -> Path: dataset = cast("TestDatasetType", _DATASET_MANIFEST.get_dataset(dataset_key)) - path = test_data_dir / dataset.extracted_dir + path: Path = test_data_dir / dataset.extracted_dir if dataset.test_path: - path /= dataset.test_path + path = path / dataset.test_path if not path.is_dir(): pytest.skip( f"Test data for {dataset_key!r} not found at {path!s}. " @@ -62,7 +80,7 @@ def _require_test_dataset(dataset_key: str) -> Path: def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: - """Apply tier markers from the test path.""" + """Apply scope and reader markers from the test path.""" for item in items: path = Path(str(item.fspath)) parts = path.parts @@ -74,3 +92,6 @@ def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: item.add_marker(pytest.mark.cli) if "require_test_dataset" in getattr(item, "fixturenames", ()): item.add_marker(pytest.mark.data) + for part in parts: + if part in _READER_MARKS: + item.add_marker(getattr(pytest.mark, part)) diff --git a/tests/integration/readers/test_generic.py b/tests/integration/readers/generic/test_generic.py similarity index 100% rename from tests/integration/readers/test_generic.py rename to tests/integration/readers/generic/test_generic.py diff --git a/tests/integration/readers/test_macsima.py b/tests/integration/readers/macsima/test_macsima.py similarity index 100% rename from tests/integration/readers/test_macsima.py rename to tests/integration/readers/macsima/test_macsima.py diff --git a/tests/integration/readers/test_seqfish.py b/tests/integration/readers/seqfish/test_seqfish.py similarity index 100% rename from tests/integration/readers/test_seqfish.py rename to tests/integration/readers/seqfish/test_seqfish.py diff --git a/tests/integration/readers/test_visium_hd.py b/tests/integration/readers/visium_hd/test_visium_hd.py similarity index 80% rename from tests/integration/readers/test_visium_hd.py rename to tests/integration/readers/visium_hd/test_visium_hd.py index 5104f48e..a1b6357b 100644 --- a/tests/integration/readers/test_visium_hd.py +++ b/tests/integration/readers/visium_hd/test_visium_hd.py @@ -20,6 +20,7 @@ DATASET_ID = "visium_hd_tiny" +@pytest.mark.slow def test_visium_hd_data_extent(require_test_dataset: Callable[[str], Path]) -> None: """Check the spatial extent of the loaded Visium HD data.""" f = require_test_dataset(DATASET_KEY) @@ -37,37 +38,49 @@ def test_visium_hd_data_extent(require_test_dataset: Callable[[str], Path]) -> N "params", [ # Test case 1: Default binned data loading (squares) - { - "load_segmentations_only": False, - "load_nucleus_segmentations": False, - "bins_as_squares": True, - "annotate_table_by_labels": False, - "load_all_images": False, - }, + pytest.param( + { + "load_segmentations_only": False, + "load_nucleus_segmentations": False, + "bins_as_squares": True, + "annotate_table_by_labels": False, + "load_all_images": False, + }, + marks=pytest.mark.slow, + ), # Test case 2: Binned data as circles - { - "load_segmentations_only": False, - "load_nucleus_segmentations": False, - "bins_as_squares": False, - "annotate_table_by_labels": False, - "load_all_images": False, - }, + pytest.param( + { + "load_segmentations_only": False, + "load_nucleus_segmentations": False, + "bins_as_squares": False, + "annotate_table_by_labels": False, + "load_all_images": False, + }, + marks=pytest.mark.slow, + ), # Test case 3: Binned data with tables annotating labels instead of shapes - { - "load_segmentations_only": False, - "load_nucleus_segmentations": False, - "bins_as_squares": True, - "annotate_table_by_labels": True, - "load_all_images": False, - }, + pytest.param( + { + "load_segmentations_only": False, + "load_nucleus_segmentations": False, + "bins_as_squares": True, + "annotate_table_by_labels": True, + "load_all_images": False, + }, + marks=pytest.mark.slow, + ), # Test case 4: Load binned data AND all segmentations (cell + nucleus) - { - "load_segmentations_only": False, - "load_nucleus_segmentations": True, - "bins_as_squares": True, - "annotate_table_by_labels": False, - "load_all_images": False, - }, + pytest.param( + { + "load_segmentations_only": False, + "load_nucleus_segmentations": True, + "bins_as_squares": True, + "annotate_table_by_labels": False, + "load_all_images": False, + }, + marks=pytest.mark.slow, + ), # Test case 5: Load cell segmentations only { "load_segmentations_only": True, @@ -148,6 +161,7 @@ def test_visium_hd_data_integrity(params: dict[str, bool], require_test_dataset: "dataset", [pytest.param("visium_hd_tiny", id="visium_hd_tiny")], ) +@pytest.mark.slow def test_cli_visium_hd(runner: CliRunner, dataset: str, require_test_dataset: Callable[[str], Path]) -> None: """Test the command-line interface for the Visium HD reader.""" f = require_test_dataset(dataset) diff --git a/tests/integration/readers/test_xenium.py b/tests/integration/readers/xenium/test_xenium.py similarity index 97% rename from tests/integration/readers/test_xenium.py rename to tests/integration/readers/xenium/test_xenium.py index a5ae278a..70b07e4c 100644 --- a/tests/integration/readers/test_xenium.py +++ b/tests/integration/readers/xenium/test_xenium.py @@ -145,9 +145,9 @@ def test_example_data_index_integrity(dataset: str, require_test_dataset: Callab @pytest.mark.parametrize( "dataset", [ - pytest.param("xenium_breast", id="xenium_breast"), - pytest.param("xenium_lung", id="xenium_lung"), - pytest.param("xenium_protein_kidney", id="xenium_protein_kidney"), + pytest.param("xenium_breast", marks=pytest.mark.slow, id="xenium_breast"), + pytest.param("xenium_lung", marks=pytest.mark.slow, id="xenium_lung"), + pytest.param("xenium_protein_kidney", marks=pytest.mark.slow, id="xenium_protein_kidney"), ], ) def test_cli_xenium(runner: CliRunner, dataset: str, require_test_dataset: Callable[[str], Path]) -> None: diff --git a/tests/unit/cli/test_cli_alignment.py b/tests/unit/cli/test_cli_alignment.py index cab2c96a..9ea1345a 100644 --- a/tests/unit/cli/test_cli_alignment.py +++ b/tests/unit/cli/test_cli_alignment.py @@ -34,22 +34,27 @@ import pytest + +def _reader_case(module_path: str, reader_name: str, wrapper_name: str) -> Any: + return pytest.param(module_path, reader_name, wrapper_name, marks=getattr(pytest.mark, reader_name), id=reader_name) + + # (reader_module_path, reader_func_name, cli_wrapper_func_name) _READERS = [ - ("spatialdata_io.readers.codex", "codex", "codex_wrapper"), - ("spatialdata_io.readers.cosmx", "cosmx", "cosmx_wrapper"), - ("spatialdata_io.readers.curio", "curio", "curio_wrapper"), - ("spatialdata_io.readers.dbit", "dbit", "dbit_wrapper"), - ("spatialdata_io.readers.iss", "iss", "iss_wrapper"), - ("spatialdata_io.readers.macsima", "macsima", "macsima_wrapper"), - ("spatialdata_io.readers.mcmicro", "mcmicro", "mcmicro_wrapper"), - ("spatialdata_io.readers.merscope", "merscope", "merscope_wrapper"), - ("spatialdata_io.readers.seqfish", "seqfish", "seqfish_wrapper"), - ("spatialdata_io.readers.steinbock", "steinbock", "steinbock_wrapper"), - ("spatialdata_io.readers.stereoseq", "stereoseq", "stereoseq_wrapper"), - ("spatialdata_io.readers.visium", "visium", "visium_wrapper"), - ("spatialdata_io.readers.visium_hd", "visium_hd", "visium_hd_wrapper"), - ("spatialdata_io.readers.xenium", "xenium", "xenium_wrapper"), + _reader_case("spatialdata_io.readers.codex", "codex", "codex_wrapper"), + _reader_case("spatialdata_io.readers.cosmx", "cosmx", "cosmx_wrapper"), + _reader_case("spatialdata_io.readers.curio", "curio", "curio_wrapper"), + _reader_case("spatialdata_io.readers.dbit", "dbit", "dbit_wrapper"), + _reader_case("spatialdata_io.readers.iss", "iss", "iss_wrapper"), + _reader_case("spatialdata_io.readers.macsima", "macsima", "macsima_wrapper"), + _reader_case("spatialdata_io.readers.mcmicro", "mcmicro", "mcmicro_wrapper"), + _reader_case("spatialdata_io.readers.merscope", "merscope", "merscope_wrapper"), + _reader_case("spatialdata_io.readers.seqfish", "seqfish", "seqfish_wrapper"), + _reader_case("spatialdata_io.readers.steinbock", "steinbock", "steinbock_wrapper"), + _reader_case("spatialdata_io.readers.stereoseq", "stereoseq", "stereoseq_wrapper"), + _reader_case("spatialdata_io.readers.visium", "visium", "visium_wrapper"), + _reader_case("spatialdata_io.readers.visium_hd", "visium_hd", "visium_hd_wrapper"), + _reader_case("spatialdata_io.readers.xenium", "xenium", "xenium_wrapper"), ] # Parameters to skip in the reader (first positional path arg, and **kwargs catch-alls) diff --git a/tests/unit/readers/test_macsima.py b/tests/unit/readers/macsima/test_macsima.py similarity index 100% rename from tests/unit/readers/test_macsima.py rename to tests/unit/readers/macsima/test_macsima.py diff --git a/tests/unit/readers/test_utils_image.py b/tests/unit/readers/utils/test_utils_image.py similarity index 100% rename from tests/unit/readers/test_utils_image.py rename to tests/unit/readers/utils/test_utils_image.py diff --git a/tests/unit/readers/test_visium_hd.py b/tests/unit/readers/visium_hd/test_visium_hd.py similarity index 100% rename from tests/unit/readers/test_visium_hd.py rename to tests/unit/readers/visium_hd/test_visium_hd.py diff --git a/tests/unit/readers/test_xenium.py b/tests/unit/readers/xenium/test_xenium.py similarity index 100% rename from tests/unit/readers/test_xenium.py rename to tests/unit/readers/xenium/test_xenium.py From 90f651b08fdee7f7739e2ab89b3d5b960d81890b Mon Sep 17 00:00:00 2001 From: Kilian Paul <81327374+Darlokt@users.noreply.github.com> Date: Sat, 16 May 2026 06:00:40 +0200 Subject: [PATCH 3/7] Improve CI and make test data handling more robust This commit focuses on improvements to the CI workflow and test data management. Changes: - Updated the `prepare_test_data.yaml` - Adjusted the cron schedule to run on the first day of every other month at midnight, ensuring that test data is refreshed regularly while preventing unnecessary runs. - Increased the retention period for test data artifacts to 64 days, providing a longer window for access and reducing the likelihood of data expiration before it can be used. - Added a condition to update the artifact if the data handler script/dataset list has changed, ensuring that the test data is always up-to-date with the latest changes in the codebase. - Added comments and docstrings to the data handler scripts to improve code readability and maintainability. - Added small checker to DatasetList to ensure that all datasets have full integrity before collection. - Improved testing --- .github/workflows/prepare_test_data.yaml | 48 ++++++++++-- scripts/download_test_data.py | 65 ++++++++++++++-- scripts/download_test_data_datasets.py | 49 +++++++++++- tests/unit/test_download_test_data.py | 96 ++++++++++++++++++++++-- 4 files changed, 237 insertions(+), 21 deletions(-) diff --git a/.github/workflows/prepare_test_data.yaml b/.github/workflows/prepare_test_data.yaml index ba9e5a21..7e5d0524 100644 --- a/.github/workflows/prepare_test_data.yaml +++ b/.github/workflows/prepare_test_data.yaml @@ -2,13 +2,31 @@ name: Prepare test data on: schedule: - - cron: "0 0 1 * *" # run once a month to prevent artifact expiration + - cron: "0 0 1 */2 *" # Run on the first day of every other month at midnight workflow_dispatch: - # Uncomment and adjust the branch name if you need to add new datasets to the artifact. - # It needs to be a branch in the spatialdata-io origin repository, not from a fork. - # push: - # branches: - # - main + inputs: + force_all: + description: "Download all registered datasets. Set to false to use dataset_keys." + required: true + type: boolean + default: true + dataset_keys: + description: "Dataset keys to download when force_all is false. Separate keys with spaces or commas." + required: false + type: string + default: "" + force_redownload: + description: "Redownload and replace existing selected datasets." + required: true + type: boolean + default: false + push: + branches: + - main + paths: + - ".github/workflows/prepare_test_data.yaml" + - "scripts/download_test_data.py" + - "scripts/download_test_data_datasets.py" jobs: prepare-data: @@ -25,7 +43,22 @@ jobs: - name: Download test datasets run: | - python scripts/download_test_data.py --output ./data + args=(--output ./data) + if [[ "${{ github.event_name }}" == "workflow_dispatch" && "${{ inputs.force_all }}" == "false" ]]; then + dataset_keys="${{ inputs.dataset_keys }}" + if [[ -z "${dataset_keys}" ]]; then + echo "::error::dataset_keys must be provided when force_all is false." + exit 1 + fi + dataset_keys="${dataset_keys//,/ }" + for dataset_key in ${dataset_keys}; do + args+=(--dataset "${dataset_key}") + done + fi + if [[ "${{ github.event_name }}" == "workflow_dispatch" && "${{ inputs.force_redownload }}" == "true" ]]; then + args+=(--force) + fi + python scripts/download_test_data.py "${args[@]}" - name: Upload artifacts uses: actions/upload-artifact@v7 @@ -33,3 +66,4 @@ jobs: name: data path: ./data if-no-files-found: error + retention-days: 64 diff --git a/scripts/download_test_data.py b/scripts/download_test_data.py index d493fed5..2e53847d 100644 --- a/scripts/download_test_data.py +++ b/scripts/download_test_data.py @@ -1,3 +1,10 @@ +"""Download optional external datasets used by integration tests. + +The script is intentionally standalone so it can run from CI before the package +itself is installed. Dataset metadata lives in ``download_test_data_datasets.py`` +to keep the manifest reusable from tests and workflow helpers. +""" + from __future__ import annotations import argparse @@ -13,12 +20,13 @@ from typing import TYPE_CHECKING if TYPE_CHECKING: + from collections.abc import Sequence from types import ModuleType from download_test_data_datasets import TestDataset as TestDatasetType -# This script uses a custom user agent, because some providers block requests with the default Python user agent. +# Some providers reject Python's default user agent even for public files. REQUEST_HEADERS = { "User-Agent": "curl/8.0.0", "Accept": "*/*", @@ -30,21 +38,25 @@ def _load_dataset_manifest() -> ModuleType: + """Load the sibling dataset manifest without relying on package imports.""" manifest_path = Path(__file__).with_name("download_test_data_datasets.py") spec = importlib.util.spec_from_file_location("download_test_data_datasets", manifest_path) if spec is None or spec.loader is None: raise RuntimeError(f"Could not import {manifest_path}") module = importlib.util.module_from_spec(spec) + # Register before execution so dataclasses and annotations resolve the module by its stable name. sys.modules[spec.name] = module spec.loader.exec_module(module) return module +# Re-export selected manifest helpers so tests and callers can import this script as one unit. _DATASET_MANIFEST = _load_dataset_manifest() TestDataset = _DATASET_MANIFEST.TestDataset DATASETS: tuple[TestDatasetType, ...] = _DATASET_MANIFEST.DATASETS get_dataset = _DATASET_MANIFEST.get_dataset datasets_by_group = _DATASET_MANIFEST.datasets_by_group +validate_datasets = _DATASET_MANIFEST.validate_datasets class DatasetDownloadError(RuntimeError): @@ -61,7 +73,8 @@ def __init__(self, dataset: TestDatasetType, action: str, reason: str, suggestio super().__init__(message) -def _parse_args() -> argparse.Namespace: +def _parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: + """Parse command-line arguments for selecting and downloading datasets.""" dataset_keys = sorted(dataset.key for dataset in DATASETS) groups = sorted({dataset.group for dataset in DATASETS}) parser = argparse.ArgumentParser(description="Download optional test datasets used by spatialdata-io CI.") @@ -80,10 +93,11 @@ def _parse_args() -> argparse.Namespace: ) parser.add_argument("--force", action="store_true", help="Redownload and replace existing selected datasets.") parser.add_argument("--list", action="store_true", help="List available datasets and exit.") - return parser.parse_args() + return parser.parse_args(argv) def _selected_datasets(dataset_keys: list[str] | None, groups: list[str] | None) -> list[TestDatasetType]: + """Return manifest entries selected by explicit keys, groups, or all datasets.""" selected_keys = set(dataset_keys or ()) selected_groups = set(groups or ()) if not selected_keys and not selected_groups: @@ -92,12 +106,14 @@ def _selected_datasets(dataset_keys: list[str] | None, groups: list[str] | None) def _is_transient_error(error: BaseException) -> bool: + """Return whether a download error is worth retrying.""" if isinstance(error, urllib.error.HTTPError): return error.code in TRANSIENT_HTTP_STATUS_CODES or 500 <= error.code < 600 return isinstance(error, urllib.error.URLError | TimeoutError) def _download_error(dataset: TestDatasetType, error: BaseException) -> DatasetDownloadError: + """Translate urllib and timeout failures into dataset-aware errors.""" if isinstance(error, urllib.error.HTTPError): reason = f"HTTP {error.code} {error.reason}" suggestion = None @@ -115,6 +131,7 @@ def _download_error(dataset: TestDatasetType, error: BaseException) -> DatasetDo def _download(dataset: TestDatasetType, archive_path: Path) -> None: + """Download ``dataset`` to ``archive_path`` with bounded retries.""" print(f"Downloading {dataset.key} from {dataset.url}") for attempt in range(1, MAX_DOWNLOAD_ATTEMPTS + 1): request = urllib.request.Request(dataset.url, headers=REQUEST_HEADERS) @@ -132,14 +149,42 @@ def _download(dataset: TestDatasetType, archive_path: Path) -> None: def _extract_zip(dataset: TestDatasetType, archive_path: Path, destination: Path) -> None: + """Extract ``archive_path`` into ``destination`` after validating member paths.""" try: with zipfile.ZipFile(archive_path) as archive: + destination_root = destination.resolve() + for member in archive.infolist(): + # Guard against zip-slip archives that contain absolute paths or ``..`` components. + member_path = (destination / member.filename).resolve() + if not member_path.is_relative_to(destination_root): + raise DatasetDownloadError( + dataset, + "extract", + f"archive member {member.filename!r} would be extracted outside {destination}", + ) archive.extractall(destination) except zipfile.BadZipFile as exc: raise DatasetDownloadError(dataset, "extract", "downloaded file is not a valid zip archive") from exc def download_dataset(dataset: TestDatasetType, output: Path, force: bool) -> None: + """Download and extract a single dataset. + + Parameters + ---------- + dataset : TestDataset + Manifest entry describing the dataset archive and expected output + directory. + output : Path + Parent directory where the extracted dataset directory should live. + force : bool + If ``True``, replace an existing extracted dataset directory. + + Raises + ------ + DatasetDownloadError + If the archive cannot be downloaded, extracted, or validated. + """ target = output / dataset.extracted_dir if target.exists() and not force: print(f"Skipping {dataset.key}: {target} already exists") @@ -152,17 +197,27 @@ def download_dataset(dataset: TestDatasetType, output: Path, force: bool) -> Non extracted_path = tmp_path / dataset.extracted_dir extracted_path.mkdir() + # Stage work in a temporary directory so interrupted downloads do not leave partial datasets. _download(dataset, archive_path) _extract_zip(dataset, archive_path, extracted_path) + if not extracted_path.is_dir() or not any(extracted_path.iterdir()): + raise DatasetDownloadError( + dataset, + "extract", + f"archive did not produce expected directory {dataset.extracted_dir!r}", + ) if target.exists(): shutil.rmtree(target) + # Move the fully validated extraction into place only after all checks pass. shutil.move(str(extracted_path), target) print(f"Downloaded {dataset.key} to {target}") -def main() -> None: - args = _parse_args() +def main(argv: Sequence[str] | None = None) -> None: + """Run the dataset downloader command-line interface.""" + validate_datasets(DATASETS) + args = _parse_args(argv) if args.list: for dataset in DATASETS: print(f"{dataset.key}\t{dataset.group}\t{dataset.extracted_dir}\t{dataset.source}") diff --git a/scripts/download_test_data_datasets.py b/scripts/download_test_data_datasets.py index e6e42c2a..4e29bfca 100644 --- a/scripts/download_test_data_datasets.py +++ b/scripts/download_test_data_datasets.py @@ -1,7 +1,14 @@ +"""Dataset manifest for optional test data downloads. + +The downloader imports this module directly from ``scripts/`` so CI jobs and +tests can share one source of truth for dataset keys, archive names, and groups. +""" + from __future__ import annotations from dataclasses import dataclass from functools import cache +from pathlib import PurePath @dataclass(frozen=True) @@ -38,6 +45,7 @@ class TestDataset: test_path: str = "" +# Keep keys and groups stable because CI workflows and pytest markers may refer to them. DATASETS = ( TestDataset( key="xenium_breast", @@ -123,8 +131,47 @@ class TestDataset: ) +def validate_datasets(datasets: tuple[TestDataset, ...] = DATASETS) -> None: + """Validate that the dataset manifest is internally consistent. + + Parameters + ---------- + datasets : tuple[TestDataset, ...] + Dataset entries to validate. Defaults to the module-level manifest. + + Raises + ------ + ValueError + If a required field is empty, a key or extracted directory is + duplicated, or ``test_path`` points outside the extracted directory. + """ + seen_keys: set[str] = set() + seen_extracted_dirs: set[str] = set() + + for dataset in datasets: + for field_name in ("key", "group", "url", "archive_name", "extracted_dir", "source"): + value = getattr(dataset, field_name) + if not value.strip(): + raise ValueError(f"Dataset {dataset.key!r} has empty {field_name}.") + test_path = PurePath(dataset.test_path) + # Test paths are appended to extracted_dir by tests, so they must stay inside that directory. + if dataset.test_path and (test_path.is_absolute() or ".." in test_path.parts): + raise ValueError(f"Dataset {dataset.key!r} test_path must be a relative path inside extracted_dir.") + if dataset.key in seen_keys: + raise ValueError(f"Duplicate test dataset key: {dataset.key!r}.") + if dataset.extracted_dir in seen_extracted_dirs: + raise ValueError(f"Duplicate test dataset extracted_dir: {dataset.extracted_dir!r}.") + seen_keys.add(dataset.key) + seen_extracted_dirs.add(dataset.extracted_dir) + + +# Fail fast when the manifest is imported, before any network work starts. +validate_datasets() + + @cache def _datasets_by_key() -> dict[str, TestDataset]: + """Return cached lookup table for dataset keys.""" return {dataset.key: dataset for dataset in DATASETS} @@ -144,5 +191,5 @@ def get_dataset(key: str) -> TestDataset: def datasets_by_group(group: str) -> tuple[TestDataset, ...]: - """Return datasets registered for ``group``.""" + """Return datasets registered for ``group`` in manifest order.""" return tuple(dataset for dataset in DATASETS if dataset.group == group) diff --git a/tests/unit/test_download_test_data.py b/tests/unit/test_download_test_data.py index 307a3bf4..07bf5b3c 100644 --- a/tests/unit/test_download_test_data.py +++ b/tests/unit/test_download_test_data.py @@ -30,14 +30,24 @@ def _load_download_script() -> ModuleType: download_test_data = _load_download_script() -def _make_dataset(key: str = "example") -> Any: +def _make_dataset( + key: str = "example", + *, + group: str = "group", + url: str | None = None, + archive_name: str | None = None, + extracted_dir: str | None = None, + source: str = "example", + test_path: str = "", +) -> Any: return download_test_data.TestDataset( key=key, - group="group", - url=f"https://example.com/{key}.zip", - archive_name=f"{key}.zip", - extracted_dir=key, - source="example", + group=group, + url=url or f"https://example.com/{key}.zip", + archive_name=archive_name or f"{key}.zip", + extracted_dir=extracted_dir or key, + source=source, + test_path=test_path, ) @@ -112,6 +122,9 @@ def urlopen(request: object, timeout: int) -> _BytesResponse: class TestDatasetManifest: + def test_manifest_is_valid(self) -> None: + download_test_data.validate_datasets(download_test_data.DATASETS) + def test_returns_dataset_by_key(self) -> None: dataset = download_test_data.get_dataset("seqfish") @@ -132,6 +145,32 @@ def test_can_filter_datasets_by_group(self) -> None: assert {dataset.key for dataset in datasets} == {"macsima_omap10", "macsima_omap23"} + def test_rejects_duplicate_dataset_keys(self) -> None: + first = _make_dataset("duplicate", extracted_dir="first") + second = _make_dataset("duplicate", extracted_dir="second") + + with pytest.raises(ValueError, match="Duplicate test dataset key: 'duplicate'"): + download_test_data.validate_datasets((first, second)) + + def test_rejects_duplicate_extracted_dirs(self) -> None: + first = _make_dataset("first", extracted_dir="duplicate") + second = _make_dataset("second", extracted_dir="duplicate") + + with pytest.raises(ValueError, match="Duplicate test dataset extracted_dir: 'duplicate'"): + download_test_data.validate_datasets((first, second)) + + def test_rejects_empty_required_manifest_fields(self) -> None: + dataset = _make_dataset("missing-group", group="") + + with pytest.raises(ValueError, match="Dataset 'missing-group' has empty group"): + download_test_data.validate_datasets((dataset,)) + + def test_rejects_test_path_outside_extracted_dir(self) -> None: + dataset = _make_dataset("unsafe-test-path", test_path="../outside") + + with pytest.raises(ValueError, match="test_path must be a relative path"): + download_test_data.validate_datasets((dataset,)) + class TestExtractZip: def test_reports_invalid_archive(self, tmp_path: Path) -> None: @@ -141,6 +180,16 @@ def test_reports_invalid_archive(self, tmp_path: Path) -> None: with pytest.raises(download_test_data.DatasetDownloadError, match="not a valid zip archive"): download_test_data._extract_zip(_make_dataset(), archive_path, tmp_path / "extract") + def test_rejects_archive_members_outside_destination(self, tmp_path: Path) -> None: + archive_path = tmp_path / "unsafe.zip" + with zipfile.ZipFile(archive_path, "w") as archive: + archive.writestr("../escape.txt", "bad") + + with pytest.raises(download_test_data.DatasetDownloadError, match="outside"): + download_test_data._extract_zip(_make_dataset(), archive_path, tmp_path / "extract") + + assert not (tmp_path / "escape.txt").exists() + class TestDownloadDataset: def test_skips_existing_dataset_without_force( @@ -175,8 +224,40 @@ def write_archive(dataset: object, archive_path: Path) -> None: assert not (target / "old.txt").exists() assert (target / "payload.txt").read_text(encoding="utf-8") == "ok" + def test_reports_archive_without_expected_contents(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + dataset = _make_dataset("empty") + + def write_empty_archive(dataset: object, archive_path: Path) -> None: + with zipfile.ZipFile(archive_path, "w"): + pass + + monkeypatch.setattr(download_test_data, "_download", write_empty_archive) + + with pytest.raises(download_test_data.DatasetDownloadError, match="expected directory"): + download_test_data.download_dataset(dataset, tmp_path, force=False) + + assert not (tmp_path / dataset.extracted_dir).exists() + class TestMain: + def test_downloads_multiple_selected_datasets(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + first = _make_dataset("first") + second = _make_dataset("second") + third = _make_dataset("third") + attempted: list[str] = [] + + def download_dataset(dataset: Any, output: Path, force: bool) -> None: + attempted.append(dataset.key) + assert output == tmp_path + assert not force + + monkeypatch.setattr(download_test_data, "DATASETS", (first, second, third)) + monkeypatch.setattr(download_test_data, "download_dataset", download_dataset) + + download_test_data.main(["--output", str(tmp_path), "--dataset", "first", "--dataset", "third"]) + + assert attempted == ["first", "third"] + def test_continues_after_failure_then_exits_nonzero( self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: @@ -191,10 +272,9 @@ def download_dataset(dataset: Any, output: Path, force: bool) -> None: monkeypatch.setattr(download_test_data, "DATASETS", (first, second)) monkeypatch.setattr(download_test_data, "download_dataset", download_dataset) - monkeypatch.setattr(sys, "argv", ["download_test_data.py", "--output", str(tmp_path)]) with pytest.raises(SystemExit) as exc_info: - download_test_data.main() + download_test_data.main(["--output", str(tmp_path)]) assert exc_info.value.code == 1 assert attempted == ["first", "second"] From 75694082aba624885ccb77706594a69eaaf928b1 Mon Sep 17 00:00:00 2001 From: Kilian Paul <81327374+Darlokt@users.noreply.github.com> Date: Sat, 16 May 2026 06:22:20 +0200 Subject: [PATCH 4/7] Clean up test data download scripts and workflow This commit refactors the test data download scripts and splits the dataset list into a separate `.toml` file. Changes: - Moved the dataset list from `download_test_data.py` to `datasets.toml`. - Updated the download script to read from the new `.toml` file. - Updated the GitHub Actions workflow to reflect the changes in the download script. - Restructured the test data downloader into a subdirectory/script package for better organization. --- .github/workflows/prepare_test_data.yaml | 5 +- docs/contributing.md | 8 +- scripts/download_test_data_datasets.py | 195 ------------------ scripts/test_data_downloader/__main__.py | 8 + scripts/test_data_downloader/datasets.toml | 80 +++++++ .../downloader.py} | 192 +++++++---------- scripts/test_data_downloader/manifest.py | 164 +++++++++++++++ tests/conftest.py | 8 +- tests/unit/test_download_test_data.py | 144 +++++++++++-- 9 files changed, 470 insertions(+), 334 deletions(-) delete mode 100644 scripts/download_test_data_datasets.py create mode 100644 scripts/test_data_downloader/__main__.py create mode 100644 scripts/test_data_downloader/datasets.toml rename scripts/{download_test_data.py => test_data_downloader/downloader.py} (80%) create mode 100644 scripts/test_data_downloader/manifest.py diff --git a/.github/workflows/prepare_test_data.yaml b/.github/workflows/prepare_test_data.yaml index 7e5d0524..92d20666 100644 --- a/.github/workflows/prepare_test_data.yaml +++ b/.github/workflows/prepare_test_data.yaml @@ -25,8 +25,7 @@ on: - main paths: - ".github/workflows/prepare_test_data.yaml" - - "scripts/download_test_data.py" - - "scripts/download_test_data_datasets.py" + - "scripts/test_data_downloader/**" jobs: prepare-data: @@ -58,7 +57,7 @@ jobs: if [[ "${{ github.event_name }}" == "workflow_dispatch" && "${{ inputs.force_redownload }}" == "true" ]]; then args+=(--force) fi - python scripts/download_test_data.py "${args[@]}" + python scripts/test_data_downloader "${args[@]}" - name: Upload artifacts uses: actions/upload-artifact@v7 diff --git a/docs/contributing.md b/docs/contributing.md index 37e0cc5e..372f671c 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -127,7 +127,7 @@ Tests are split by scope: - Unit tests live in `tests/unit/` and should not require downloaded test data. - Integration tests live in `tests/integration/` and cover reader workflows, CLI commands, file I/O, and zarr roundtrips. -- Integration tests that require external datasets use dataset keys from `scripts/download_test_data_datasets.py`. +- Integration tests that require external datasets use dataset keys from `scripts/test_data_downloader/datasets.toml`. They resolve data under `SPATIALDATA_IO_TEST_DATA_DIR` when set, otherwise `data/` in the repository root. If the required dataset is unavailable, the test should skip with a clear message. - Reader tests are marked by reader name. When modifying one reader, use `pytest -m ` to run the tests specific to that reader, including shared parametrized checks for that reader. @@ -142,18 +142,18 @@ pytest -m xenium pytest -m "xenium and data" pytest -m "xenium and not slow" pytest -m "xenium and cli" -python scripts/download_test_data.py --group xenium +python scripts/test_data_downloader --group xenium SPATIALDATA_IO_TEST_DATA_DIR=/path/to/data pytest -m data ``` To download the same optional datasets used by CI, run: ```bash -python scripts/download_test_data.py +python scripts/test_data_downloader ``` By default, the downloader skips datasets that already exist. Use `--force` to redownload selected datasets, `--dataset` for a single dataset key, and `--list` to show the available keys. -The dataset registry lives in `scripts/download_test_data_datasets.py`; append new entries there when adding or updating test datasets. +The dataset registry lives in `scripts/test_data_downloader/datasets.toml`; append new entries there when adding or updating test datasets. ### Testing multiple versions diff --git a/scripts/download_test_data_datasets.py b/scripts/download_test_data_datasets.py deleted file mode 100644 index 4e29bfca..00000000 --- a/scripts/download_test_data_datasets.py +++ /dev/null @@ -1,195 +0,0 @@ -"""Dataset manifest for optional test data downloads. - -The downloader imports this module directly from ``scripts/`` so CI jobs and -tests can share one source of truth for dataset keys, archive names, and groups. -""" - -from __future__ import annotations - -from dataclasses import dataclass -from functools import cache -from pathlib import PurePath - - -@dataclass(frozen=True) -class TestDataset: - """Test dataset to be downloaded. - - Parameters - ---------- - key : str - Unique identifier for the dataset, used for referencing in tests and - the CLI. - group : str - Logical grouping of the dataset, for example ``"xenium"``, - ``"visium_hd"``, ``"seqfish"``, or ``"macsima"``. - url : str - Direct URL to the dataset archive, for example a ZIP file. - archive_name : str - Expected filename of the downloaded archive. - extracted_dir : str - Expected name of the directory created when the archive is extracted. - source : str - Human-readable description of the dataset source and license. - test_path : str - Optional path inside the extracted directory that should be passed to - integration tests. - """ - - key: str - group: str - url: str - archive_name: str - extracted_dir: str - source: str - test_path: str = "" - - -# Keep keys and groups stable because CI workflows and pytest markers may refer to them. -DATASETS = ( - TestDataset( - key="xenium_breast", - group="xenium", - url="https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Breast_2fov/Xenium_V1_human_Breast_2fov_outs.zip", - archive_name="Xenium_V1_human_Breast_2fov_outs.zip", - extracted_dir="Xenium_V1_human_Breast_2fov_outs", - source="10x Genomics Xenium 2.0.0, CC BY 4.0", - ), - TestDataset( - key="xenium_lung", - group="xenium", - url="https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Lung_2fov/Xenium_V1_human_Lung_2fov_outs.zip", - archive_name="Xenium_V1_human_Lung_2fov_outs.zip", - extracted_dir="Xenium_V1_human_Lung_2fov_outs", - source="10x Genomics Xenium 2.0.0, CC BY 4.0", - ), - TestDataset( - key="xenium_prime_mouse_ileum", - group="xenium", - url="https://cf.10xgenomics.com/samples/xenium/3.0.0/Xenium_Prime_Mouse_Ileum_tiny/Xenium_Prime_Mouse_Ileum_tiny_outs.zip", - archive_name="Xenium_Prime_Mouse_Ileum_tiny_outs.zip", - extracted_dir="Xenium_Prime_Mouse_Ileum_tiny_outs", - source="10x Genomics Xenium 3.0.0, CC BY 4.0", - ), - TestDataset( - key="xenium_ovary", - group="xenium", - url="https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Human_Ovary_tiny/Xenium_V1_Human_Ovary_tiny_outs.zip", - archive_name="Xenium_V1_Human_Ovary_tiny_outs.zip", - extracted_dir="Xenium_V1_Human_Ovary_tiny_outs", - source="10x Genomics Xenium 4.0.0, CC BY 4.0", - ), - TestDataset( - key="xenium_multicell_ovary", - group="xenium", - url="https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_MultiCellSeg_Human_Ovary_tiny/Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip", - archive_name="Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip", - extracted_dir="Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs", - source="10x Genomics Xenium 4.0.0, CC BY 4.0", - ), - TestDataset( - key="xenium_protein_kidney", - group="xenium", - url="https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Protein_Human_Kidney_tiny/Xenium_V1_Protein_Human_Kidney_tiny_outs.zip", - archive_name="Xenium_V1_Protein_Human_Kidney_tiny_outs.zip", - extracted_dir="Xenium_V1_Protein_Human_Kidney_tiny_outs", - source="10x Genomics Xenium 4.0.0, CC BY 4.0", - ), - TestDataset( - key="visium_hd_tiny", - group="visium_hd", - url="https://cf.10xgenomics.com/samples/spatial-exp/4.0.1/Visium_HD_Tiny_3prime_Dataset/Visium_HD_Tiny_3prime_Dataset_outs.zip", - archive_name="Visium_HD_Tiny_3prime_Dataset_outs.zip", - extracted_dir="Visium_HD_Tiny_3prime_Dataset_outs", - source="10x Genomics Visium HD 4.0.1, CC BY 4.0", - ), - TestDataset( - key="seqfish", - group="seqfish", - url="https://s3.embl.de/spatialdata/raw_data/seqfish-2-test-dataset.zip", - archive_name="seqfish-2-test-dataset.zip", - extracted_dir="seqfish-2-test-dataset", - source="Spatial Genomics seqFISH v2, public test data", - test_path="instrument 2 official", - ), - TestDataset( - key="macsima_omap23", - group="macsima", - url="https://zenodo.org/api/records/18196452/files-archive", - archive_name="OMAP23_small.zip", - extracted_dir="OMAP23_small", - source="MACSima OMAP23, CC BY 4.0", - ), - TestDataset( - key="macsima_omap10", - group="macsima", - url="https://zenodo.org/api/records/18196366/files-archive", - archive_name="OMAP10_small.zip", - extracted_dir="OMAP10_small", - source="MACSima OMAP10, CC BY 4.0", - ), -) - - -def validate_datasets(datasets: tuple[TestDataset, ...] = DATASETS) -> None: - """Validate that the dataset manifest is internally consistent. - - Parameters - ---------- - datasets : tuple[TestDataset, ...] - Dataset entries to validate. Defaults to the module-level manifest. - - Raises - ------ - ValueError - If a required field is empty, a key or extracted directory is - duplicated, or ``test_path`` points outside the extracted directory. - """ - seen_keys: set[str] = set() - seen_extracted_dirs: set[str] = set() - - for dataset in datasets: - for field_name in ("key", "group", "url", "archive_name", "extracted_dir", "source"): - value = getattr(dataset, field_name) - if not value.strip(): - raise ValueError(f"Dataset {dataset.key!r} has empty {field_name}.") - test_path = PurePath(dataset.test_path) - # Test paths are appended to extracted_dir by tests, so they must stay inside that directory. - if dataset.test_path and (test_path.is_absolute() or ".." in test_path.parts): - raise ValueError(f"Dataset {dataset.key!r} test_path must be a relative path inside extracted_dir.") - if dataset.key in seen_keys: - raise ValueError(f"Duplicate test dataset key: {dataset.key!r}.") - if dataset.extracted_dir in seen_extracted_dirs: - raise ValueError(f"Duplicate test dataset extracted_dir: {dataset.extracted_dir!r}.") - seen_keys.add(dataset.key) - seen_extracted_dirs.add(dataset.extracted_dir) - - -# Fail fast when the manifest is imported, before any network work starts. -validate_datasets() - - -@cache -def _datasets_by_key() -> dict[str, TestDataset]: - """Return cached lookup table for dataset keys.""" - return {dataset.key: dataset for dataset in DATASETS} - - -def get_dataset(key: str) -> TestDataset: - """Return the dataset registered for ``key``. - - Raises - ------ - KeyError - If ``key`` is not a registered dataset key. - """ - try: - return _datasets_by_key()[key] - except KeyError as exc: - available = ", ".join(sorted(_datasets_by_key())) - raise KeyError(f"Unknown test dataset key {key!r}. Available keys: {available}") from exc - - -def datasets_by_group(group: str) -> tuple[TestDataset, ...]: - """Return datasets registered for ``group`` in manifest order.""" - return tuple(dataset for dataset in DATASETS if dataset.group == group) diff --git a/scripts/test_data_downloader/__main__.py b/scripts/test_data_downloader/__main__.py new file mode 100644 index 00000000..b63fb357 --- /dev/null +++ b/scripts/test_data_downloader/__main__.py @@ -0,0 +1,8 @@ +"""Command-line entrypoint for the optional test data downloader.""" + +from __future__ import annotations + +from downloader import main + +if __name__ == "__main__": + main() diff --git a/scripts/test_data_downloader/datasets.toml b/scripts/test_data_downloader/datasets.toml new file mode 100644 index 00000000..9daaffb2 --- /dev/null +++ b/scripts/test_data_downloader/datasets.toml @@ -0,0 +1,80 @@ +[[datasets]] +key = "xenium_breast" +group = "xenium" +url = "https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Breast_2fov/Xenium_V1_human_Breast_2fov_outs.zip" +archive_name = "Xenium_V1_human_Breast_2fov_outs.zip" +extracted_dir = "Xenium_V1_human_Breast_2fov_outs" +source = "10x Genomics Xenium 2.0.0, CC BY 4.0" + +[[datasets]] +key = "xenium_lung" +group = "xenium" +url = "https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Lung_2fov/Xenium_V1_human_Lung_2fov_outs.zip" +archive_name = "Xenium_V1_human_Lung_2fov_outs.zip" +extracted_dir = "Xenium_V1_human_Lung_2fov_outs" +source = "10x Genomics Xenium 2.0.0, CC BY 4.0" + +[[datasets]] +key = "xenium_prime_mouse_ileum" +group = "xenium" +url = "https://cf.10xgenomics.com/samples/xenium/3.0.0/Xenium_Prime_Mouse_Ileum_tiny/Xenium_Prime_Mouse_Ileum_tiny_outs.zip" +archive_name = "Xenium_Prime_Mouse_Ileum_tiny_outs.zip" +extracted_dir = "Xenium_Prime_Mouse_Ileum_tiny_outs" +source = "10x Genomics Xenium 3.0.0, CC BY 4.0" + +[[datasets]] +key = "xenium_ovary" +group = "xenium" +url = "https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Human_Ovary_tiny/Xenium_V1_Human_Ovary_tiny_outs.zip" +archive_name = "Xenium_V1_Human_Ovary_tiny_outs.zip" +extracted_dir = "Xenium_V1_Human_Ovary_tiny_outs" +source = "10x Genomics Xenium 4.0.0, CC BY 4.0" + +[[datasets]] +key = "xenium_multicell_ovary" +group = "xenium" +url = "https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_MultiCellSeg_Human_Ovary_tiny/Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip" +archive_name = "Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip" +extracted_dir = "Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs" +source = "10x Genomics Xenium 4.0.0, CC BY 4.0" + +[[datasets]] +key = "xenium_protein_kidney" +group = "xenium" +url = "https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Protein_Human_Kidney_tiny/Xenium_V1_Protein_Human_Kidney_tiny_outs.zip" +archive_name = "Xenium_V1_Protein_Human_Kidney_tiny_outs.zip" +extracted_dir = "Xenium_V1_Protein_Human_Kidney_tiny_outs" +source = "10x Genomics Xenium 4.0.0, CC BY 4.0" + +[[datasets]] +key = "visium_hd_tiny" +group = "visium_hd" +url = "https://cf.10xgenomics.com/samples/spatial-exp/4.0.1/Visium_HD_Tiny_3prime_Dataset/Visium_HD_Tiny_3prime_Dataset_outs.zip" +archive_name = "Visium_HD_Tiny_3prime_Dataset_outs.zip" +extracted_dir = "Visium_HD_Tiny_3prime_Dataset_outs" +source = "10x Genomics Visium HD 4.0.1, CC BY 4.0" + +[[datasets]] +key = "seqfish" +group = "seqfish" +url = "https://s3.embl.de/spatialdata/raw_data/seqfish-2-test-dataset.zip" +archive_name = "seqfish-2-test-dataset.zip" +extracted_dir = "seqfish-2-test-dataset" +source = "Spatial Genomics seqFISH v2, public test data" +test_path = "instrument 2 official" + +[[datasets]] +key = "macsima_omap23" +group = "macsima" +url = "https://zenodo.org/api/records/18196452/files-archive" +archive_name = "OMAP23_small.zip" +extracted_dir = "OMAP23_small" +source = "MACSima OMAP23, CC BY 4.0" + +[[datasets]] +key = "macsima_omap10" +group = "macsima" +url = "https://zenodo.org/api/records/18196366/files-archive" +archive_name = "OMAP10_small.zip" +extracted_dir = "OMAP10_small" +source = "MACSima OMAP10, CC BY 4.0" diff --git a/scripts/download_test_data.py b/scripts/test_data_downloader/downloader.py similarity index 80% rename from scripts/download_test_data.py rename to scripts/test_data_downloader/downloader.py index 2e53847d..67fabd82 100644 --- a/scripts/download_test_data.py +++ b/scripts/test_data_downloader/downloader.py @@ -1,16 +1,9 @@ -"""Download optional external datasets used by integration tests. - -The script is intentionally standalone so it can run from CI before the package -itself is installed. Dataset metadata lives in ``download_test_data_datasets.py`` -to keep the manifest reusable from tests and workflow helpers. -""" +"""Download optional external datasets used by integration tests.""" from __future__ import annotations import argparse -import importlib.util import shutil -import sys import tempfile import time import urllib.error @@ -19,11 +12,10 @@ from pathlib import Path from typing import TYPE_CHECKING +from manifest import DATASETS, TestDataset, validate_datasets + if TYPE_CHECKING: from collections.abc import Sequence - from types import ModuleType - - from download_test_data_datasets import TestDataset as TestDatasetType # Some providers reject Python's default user agent even for public files. @@ -37,32 +29,10 @@ TRANSIENT_HTTP_STATUS_CODES = {408, 429} -def _load_dataset_manifest() -> ModuleType: - """Load the sibling dataset manifest without relying on package imports.""" - manifest_path = Path(__file__).with_name("download_test_data_datasets.py") - spec = importlib.util.spec_from_file_location("download_test_data_datasets", manifest_path) - if spec is None or spec.loader is None: - raise RuntimeError(f"Could not import {manifest_path}") - module = importlib.util.module_from_spec(spec) - # Register before execution so dataclasses and annotations resolve the module by its stable name. - sys.modules[spec.name] = module - spec.loader.exec_module(module) - return module - - -# Re-export selected manifest helpers so tests and callers can import this script as one unit. -_DATASET_MANIFEST = _load_dataset_manifest() -TestDataset = _DATASET_MANIFEST.TestDataset -DATASETS: tuple[TestDatasetType, ...] = _DATASET_MANIFEST.DATASETS -get_dataset = _DATASET_MANIFEST.get_dataset -datasets_by_group = _DATASET_MANIFEST.datasets_by_group -validate_datasets = _DATASET_MANIFEST.validate_datasets - - class DatasetDownloadError(RuntimeError): """Error raised when a test dataset cannot be downloaded or extracted.""" - def __init__(self, dataset: TestDatasetType, action: str, reason: str, suggestion: str | None = None) -> None: + def __init__(self, dataset: TestDataset, action: str, reason: str, suggestion: str | None = None) -> None: self.dataset = dataset self.action = action self.reason = reason @@ -73,6 +43,77 @@ def __init__(self, dataset: TestDatasetType, action: str, reason: str, suggestio super().__init__(message) +def download_dataset(dataset: TestDataset, output: Path, force: bool) -> None: + """Download and extract a single dataset. + + Parameters + ---------- + dataset : TestDataset + Manifest entry describing the dataset archive and expected output + directory. + output : Path + Parent directory where the extracted dataset directory should live. + force : bool + If ``True``, replace an existing extracted dataset directory. + + Raises + ------ + DatasetDownloadError + If the archive cannot be downloaded, extracted, or validated. + """ + target = output / dataset.extracted_dir + if target.exists() and not force: + print(f"Skipping {dataset.key}: {target} already exists") + return + + output.mkdir(parents=True, exist_ok=True) + with tempfile.TemporaryDirectory(prefix=f"{dataset.key}-", dir=output) as tmpdir: + tmp_path = Path(tmpdir) + archive_path = tmp_path / dataset.archive_name + extracted_path = tmp_path / dataset.extracted_dir + extracted_path.mkdir() + + # Stage work in a temporary directory so interrupted downloads do not leave partial datasets. + _download(dataset, archive_path) + _extract_zip(dataset, archive_path, extracted_path) + if not extracted_path.is_dir() or not any(extracted_path.iterdir()): + raise DatasetDownloadError( + dataset, + "extract", + f"archive did not produce expected directory {dataset.extracted_dir!r}", + ) + + if target.exists(): + shutil.rmtree(target) + # Move the fully validated extraction into place only after all checks pass. + shutil.move(str(extracted_path), target) + print(f"Downloaded {dataset.key} to {target}") + + +def main(argv: Sequence[str] | None = None) -> None: + """Run the dataset downloader command-line interface.""" + validate_datasets(DATASETS) + args = _parse_args(argv) + if args.list: + for dataset in DATASETS: + print(f"{dataset.key}\t{dataset.group}\t{dataset.extracted_dir}\t{dataset.source}") + return + + failures: list[DatasetDownloadError] = [] + for dataset in _selected_datasets(args.dataset, args.group): + try: + download_dataset(dataset, args.output, args.force) + except DatasetDownloadError as exc: + failures.append(exc) + print(f"ERROR: {exc}") + + if failures: + print(f"Failed to download {len(failures)} dataset(s):") + for failure in failures: + print(f"- {failure}") + raise SystemExit(1) + + def _parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: """Parse command-line arguments for selecting and downloading datasets.""" dataset_keys = sorted(dataset.key for dataset in DATASETS) @@ -96,7 +137,7 @@ def _parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: return parser.parse_args(argv) -def _selected_datasets(dataset_keys: list[str] | None, groups: list[str] | None) -> list[TestDatasetType]: +def _selected_datasets(dataset_keys: list[str] | None, groups: list[str] | None) -> list[TestDataset]: """Return manifest entries selected by explicit keys, groups, or all datasets.""" selected_keys = set(dataset_keys or ()) selected_groups = set(groups or ()) @@ -112,7 +153,7 @@ def _is_transient_error(error: BaseException) -> bool: return isinstance(error, urllib.error.URLError | TimeoutError) -def _download_error(dataset: TestDatasetType, error: BaseException) -> DatasetDownloadError: +def _download_error(dataset: TestDataset, error: BaseException) -> DatasetDownloadError: """Translate urllib and timeout failures into dataset-aware errors.""" if isinstance(error, urllib.error.HTTPError): reason = f"HTTP {error.code} {error.reason}" @@ -130,7 +171,7 @@ def _download_error(dataset: TestDatasetType, error: BaseException) -> DatasetDo return DatasetDownloadError(dataset, "download", str(error)) -def _download(dataset: TestDatasetType, archive_path: Path) -> None: +def _download(dataset: TestDataset, archive_path: Path) -> None: """Download ``dataset`` to ``archive_path`` with bounded retries.""" print(f"Downloading {dataset.key} from {dataset.url}") for attempt in range(1, MAX_DOWNLOAD_ATTEMPTS + 1): @@ -148,7 +189,7 @@ def _download(dataset: TestDatasetType, archive_path: Path) -> None: raise _download_error(dataset, exc) from exc -def _extract_zip(dataset: TestDatasetType, archive_path: Path, destination: Path) -> None: +def _extract_zip(dataset: TestDataset, archive_path: Path, destination: Path) -> None: """Extract ``archive_path`` into ``destination`` after validating member paths.""" try: with zipfile.ZipFile(archive_path) as archive: @@ -165,78 +206,3 @@ def _extract_zip(dataset: TestDatasetType, archive_path: Path, destination: Path archive.extractall(destination) except zipfile.BadZipFile as exc: raise DatasetDownloadError(dataset, "extract", "downloaded file is not a valid zip archive") from exc - - -def download_dataset(dataset: TestDatasetType, output: Path, force: bool) -> None: - """Download and extract a single dataset. - - Parameters - ---------- - dataset : TestDataset - Manifest entry describing the dataset archive and expected output - directory. - output : Path - Parent directory where the extracted dataset directory should live. - force : bool - If ``True``, replace an existing extracted dataset directory. - - Raises - ------ - DatasetDownloadError - If the archive cannot be downloaded, extracted, or validated. - """ - target = output / dataset.extracted_dir - if target.exists() and not force: - print(f"Skipping {dataset.key}: {target} already exists") - return - - output.mkdir(parents=True, exist_ok=True) - with tempfile.TemporaryDirectory(prefix=f"{dataset.key}-", dir=output) as tmpdir: - tmp_path = Path(tmpdir) - archive_path = tmp_path / dataset.archive_name - extracted_path = tmp_path / dataset.extracted_dir - extracted_path.mkdir() - - # Stage work in a temporary directory so interrupted downloads do not leave partial datasets. - _download(dataset, archive_path) - _extract_zip(dataset, archive_path, extracted_path) - if not extracted_path.is_dir() or not any(extracted_path.iterdir()): - raise DatasetDownloadError( - dataset, - "extract", - f"archive did not produce expected directory {dataset.extracted_dir!r}", - ) - - if target.exists(): - shutil.rmtree(target) - # Move the fully validated extraction into place only after all checks pass. - shutil.move(str(extracted_path), target) - print(f"Downloaded {dataset.key} to {target}") - - -def main(argv: Sequence[str] | None = None) -> None: - """Run the dataset downloader command-line interface.""" - validate_datasets(DATASETS) - args = _parse_args(argv) - if args.list: - for dataset in DATASETS: - print(f"{dataset.key}\t{dataset.group}\t{dataset.extracted_dir}\t{dataset.source}") - return - - failures: list[DatasetDownloadError] = [] - for dataset in _selected_datasets(args.dataset, args.group): - try: - download_dataset(dataset, args.output, args.force) - except DatasetDownloadError as exc: - failures.append(exc) - print(f"ERROR: {exc}") - - if failures: - print(f"Failed to download {len(failures)} dataset(s):") - for failure in failures: - print(f"- {failure}") - raise SystemExit(1) - - -if __name__ == "__main__": - main() diff --git a/scripts/test_data_downloader/manifest.py b/scripts/test_data_downloader/manifest.py new file mode 100644 index 00000000..843564f5 --- /dev/null +++ b/scripts/test_data_downloader/manifest.py @@ -0,0 +1,164 @@ +"""Dataset manifest loading and validation for optional test data downloads.""" + +from __future__ import annotations + +import tomllib +from dataclasses import dataclass +from functools import cache +from pathlib import Path, PurePath +from typing import Any + +DATASETS_TOML = Path(__file__).with_name("datasets.toml") +REQUIRED_FIELDS = frozenset({"key", "group", "url", "archive_name", "extracted_dir", "source"}) +OPTIONAL_FIELDS = frozenset({"test_path"}) +ALLOWED_FIELDS = REQUIRED_FIELDS | OPTIONAL_FIELDS + + +@dataclass(frozen=True) +class TestDataset: + """Test dataset to be downloaded. + + Parameters + ---------- + key : str + Unique identifier for the dataset, used for referencing in tests and + the CLI. + group : str + Logical grouping of the dataset, for example ``"xenium"``, + ``"visium_hd"``, ``"seqfish"``, or ``"macsima"``. + url : str + Direct URL to the dataset archive, for example a ZIP file. + archive_name : str + Expected filename of the downloaded archive. + extracted_dir : str + Expected name of the directory created when the archive is extracted. + source : str + Human-readable description of the dataset source and license. + test_path : str + Optional path inside the extracted directory that should be passed to + integration tests. + """ + + key: str + group: str + url: str + archive_name: str + extracted_dir: str + source: str + test_path: str = "" + + +def load_datasets(path: str | Path = DATASETS_TOML) -> tuple[TestDataset, ...]: + """Load dataset entries from a TOML manifest.""" + manifest_path = Path(path) + try: + raw_manifest = tomllib.loads(manifest_path.read_text(encoding="utf-8")) + except tomllib.TOMLDecodeError as exc: + raise ValueError(f"Invalid dataset manifest TOML in {manifest_path}: {exc}") from exc + + unknown_root_fields = set(raw_manifest) - {"datasets"} + if unknown_root_fields: + unknown = ", ".join(sorted(unknown_root_fields)) + raise ValueError(f"Dataset manifest has unknown root field(s): {unknown}.") + + raw_datasets = raw_manifest.get("datasets") + if not isinstance(raw_datasets, list): + raise ValueError("Dataset manifest must define a [[datasets]] array.") + + datasets = tuple(_parse_dataset(raw_dataset, index) for index, raw_dataset in enumerate(raw_datasets)) + validate_datasets(datasets) + return datasets + + +def validate_datasets(datasets: tuple[TestDataset, ...] | None = None) -> None: + """Validate that dataset entries are internally consistent. + + Parameters + ---------- + datasets : tuple[TestDataset, ...] + Dataset entries to validate. + + Raises + ------ + ValueError + If a required field is empty, a key or extracted directory is + duplicated, or ``test_path`` points outside the extracted directory. + """ + if datasets is None: + datasets = DATASETS + + seen_keys: set[str] = set() + seen_extracted_dirs: set[str] = set() + + for dataset in datasets: + for field_name in REQUIRED_FIELDS: + value = getattr(dataset, field_name) + if not isinstance(value, str): + raise ValueError(f"Dataset {dataset.key!r} field {field_name!r} must be a string.") + if not value.strip(): + raise ValueError(f"Dataset {dataset.key!r} has empty {field_name}.") + if not isinstance(dataset.test_path, str): + raise ValueError(f"Dataset {dataset.key!r} field 'test_path' must be a string.") + + test_path = PurePath(dataset.test_path) + # Test paths are appended to extracted_dir by tests, so they must stay inside that directory. + if dataset.test_path and (test_path.is_absolute() or ".." in test_path.parts): + raise ValueError(f"Dataset {dataset.key!r} test_path must be a relative path inside extracted_dir.") + if dataset.key in seen_keys: + raise ValueError(f"Duplicate test dataset key: {dataset.key!r}.") + if dataset.extracted_dir in seen_extracted_dirs: + raise ValueError(f"Duplicate test dataset extracted_dir: {dataset.extracted_dir!r}.") + seen_keys.add(dataset.key) + seen_extracted_dirs.add(dataset.extracted_dir) + + +def get_dataset(key: str) -> TestDataset: + """Return the dataset registered for ``key``. + + Raises + ------ + KeyError + If ``key`` is not a registered dataset key. + """ + try: + return _datasets_by_key()[key] + except KeyError as exc: + available = ", ".join(sorted(_datasets_by_key())) + raise KeyError(f"Unknown test dataset key {key!r}. Available keys: {available}") from exc + + +def datasets_by_group(group: str) -> tuple[TestDataset, ...]: + """Return datasets registered for ``group`` in manifest order.""" + return tuple(dataset for dataset in DATASETS if dataset.group == group) + + +def _parse_dataset(raw_dataset: object, index: int) -> TestDataset: + if not isinstance(raw_dataset, dict): + raise ValueError(f"Dataset manifest entry at index {index} must be a table.") + + dataset_fields = set(raw_dataset) + unknown_fields = dataset_fields - ALLOWED_FIELDS + if unknown_fields: + unknown = ", ".join(sorted(unknown_fields)) + raise ValueError(f"Dataset manifest entry at index {index} has unknown field(s): {unknown}.") + + missing_fields = REQUIRED_FIELDS - dataset_fields + if missing_fields: + missing = ", ".join(sorted(missing_fields)) + raise ValueError(f"Dataset manifest entry at index {index} is missing required field(s): {missing}.") + + values: dict[str, Any] = dict(raw_dataset) + values.setdefault("test_path", "") + dataset = TestDataset(**values) + validate_datasets((dataset,)) + return dataset + + +# Keep keys and groups stable because CI workflows and pytest markers may refer to them. +DATASETS = load_datasets() + + +@cache +def _datasets_by_key() -> dict[str, TestDataset]: + """Return cached lookup table for dataset keys.""" + return {dataset.key: dataset for dataset in DATASETS} diff --git a/tests/conftest.py b/tests/conftest.py index eedcbe1a..278a208a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,12 +13,12 @@ from collections.abc import Callable from types import ModuleType - from download_test_data_datasets import TestDataset as TestDatasetType + from manifest import TestDataset as TestDatasetType def _load_dataset_manifest() -> ModuleType: - manifest_path = Path(__file__).parents[1] / "scripts" / "download_test_data_datasets.py" - spec = importlib.util.spec_from_file_location("download_test_data_datasets", manifest_path) + manifest_path = Path(__file__).parents[1] / "scripts" / "test_data_downloader" / "manifest.py" + spec = importlib.util.spec_from_file_location("manifest", manifest_path) if spec is None or spec.loader is None: raise RuntimeError(f"Could not import {manifest_path}") module = importlib.util.module_from_spec(spec) @@ -71,7 +71,7 @@ def _require_test_dataset(dataset_key: str) -> Path: if not path.is_dir(): pytest.skip( f"Test data for {dataset_key!r} not found at {path!s}. " - f"Download it with `uv run python scripts/download_test_data.py --dataset {dataset_key}` or set " + f"Download it with `uv run python scripts/test_data_downloader --dataset {dataset_key}` or set " "SPATIALDATA_IO_TEST_DATA_DIR." ) return path diff --git a/tests/unit/test_download_test_data.py b/tests/unit/test_download_test_data.py index 07bf5b3c..70b7c505 100644 --- a/tests/unit/test_download_test_data.py +++ b/tests/unit/test_download_test_data.py @@ -16,9 +16,11 @@ from types import ModuleType -def _load_download_script() -> ModuleType: - module_path = Path(__file__).parents[2] / "scripts" / "download_test_data.py" - spec = importlib.util.spec_from_file_location("download_test_data", module_path) +SCRIPT_DIR = Path(__file__).parents[2] / "scripts" / "test_data_downloader" + + +def _load_module(module_name: str, module_path: Path) -> ModuleType: + spec = importlib.util.spec_from_file_location(module_name, module_path) if spec is None or spec.loader is None: raise RuntimeError(f"Could not import {module_path}") module = importlib.util.module_from_spec(spec) @@ -27,7 +29,8 @@ def _load_download_script() -> ModuleType: return module -download_test_data = _load_download_script() +manifest = _load_module("manifest", SCRIPT_DIR / "manifest.py") +download_test_data = _load_module("downloader", SCRIPT_DIR / "downloader.py") def _make_dataset( @@ -66,8 +69,8 @@ def __exit__(self, exc_type: object, exc: object, traceback: object) -> None: class TestDownload: def test_uses_dataset_manifest_module(self) -> None: - assert download_test_data.TestDataset.__module__ == "download_test_data_datasets" - assert all(isinstance(dataset, download_test_data.TestDataset) for dataset in download_test_data.DATASETS) + assert download_test_data.TestDataset.__module__ == "manifest" + assert all(isinstance(dataset, manifest.TestDataset) for dataset in download_test_data.DATASETS) def test_sends_explicit_headers(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: seen_request: urllib.request.Request | None = None @@ -123,53 +126,153 @@ def urlopen(request: object, timeout: int) -> _BytesResponse: class TestDatasetManifest: def test_manifest_is_valid(self) -> None: - download_test_data.validate_datasets(download_test_data.DATASETS) + manifest.validate_datasets() def test_returns_dataset_by_key(self) -> None: - dataset = download_test_data.get_dataset("seqfish") + dataset = manifest.get_dataset("seqfish") assert dataset.key == "seqfish" assert dataset.extracted_dir == "seqfish-2-test-dataset" def test_reports_unknown_dataset_key(self) -> None: with pytest.raises(KeyError, match="Unknown test dataset key 'missing'"): - download_test_data.get_dataset("missing") + manifest.get_dataset("missing") def test_test_path_defaults_to_extracted_directory_root(self) -> None: - dataset = download_test_data.get_dataset("xenium_breast") + dataset = manifest.get_dataset("xenium_breast") assert dataset.test_path == "" def test_can_filter_datasets_by_group(self) -> None: - datasets = download_test_data.datasets_by_group("macsima") + datasets = manifest.datasets_by_group("macsima") assert {dataset.key for dataset in datasets} == {"macsima_omap10", "macsima_omap23"} + def test_loads_datasets_from_toml(self, tmp_path: Path) -> None: + manifest_path = tmp_path / "datasets.toml" + manifest_path.write_text( + """ +[[datasets]] +key = "example" +group = "group" +url = "https://example.com/example.zip" +archive_name = "example.zip" +extracted_dir = "example" +source = "example source" +test_path = "nested" +""", + encoding="utf-8", + ) + + datasets = manifest.load_datasets(manifest_path) + + assert datasets == ( + manifest.TestDataset( + key="example", + group="group", + url="https://example.com/example.zip", + archive_name="example.zip", + extracted_dir="example", + source="example source", + test_path="nested", + ), + ) + def test_rejects_duplicate_dataset_keys(self) -> None: first = _make_dataset("duplicate", extracted_dir="first") second = _make_dataset("duplicate", extracted_dir="second") with pytest.raises(ValueError, match="Duplicate test dataset key: 'duplicate'"): - download_test_data.validate_datasets((first, second)) + manifest.validate_datasets((first, second)) def test_rejects_duplicate_extracted_dirs(self) -> None: first = _make_dataset("first", extracted_dir="duplicate") second = _make_dataset("second", extracted_dir="duplicate") with pytest.raises(ValueError, match="Duplicate test dataset extracted_dir: 'duplicate'"): - download_test_data.validate_datasets((first, second)) + manifest.validate_datasets((first, second)) def test_rejects_empty_required_manifest_fields(self) -> None: dataset = _make_dataset("missing-group", group="") with pytest.raises(ValueError, match="Dataset 'missing-group' has empty group"): - download_test_data.validate_datasets((dataset,)) + manifest.validate_datasets((dataset,)) def test_rejects_test_path_outside_extracted_dir(self) -> None: dataset = _make_dataset("unsafe-test-path", test_path="../outside") with pytest.raises(ValueError, match="test_path must be a relative path"): - download_test_data.validate_datasets((dataset,)) + manifest.validate_datasets((dataset,)) + + def test_rejects_manifest_without_datasets_array(self, tmp_path: Path) -> None: + manifest_path = tmp_path / "datasets.toml" + manifest_path.write_text("datasets = { key = 'example' }\n", encoding="utf-8") + + with pytest.raises(ValueError, match=r"\[\[datasets\]\] array"): + manifest.load_datasets(manifest_path) + + def test_rejects_invalid_toml(self, tmp_path: Path) -> None: + manifest_path = tmp_path / "datasets.toml" + manifest_path.write_text("[[datasets]\n", encoding="utf-8") + + with pytest.raises(ValueError, match="Invalid dataset manifest TOML"): + manifest.load_datasets(manifest_path) + + def test_rejects_unknown_root_manifest_field(self, tmp_path: Path) -> None: + manifest_path = tmp_path / "datasets.toml" + manifest_path.write_text( + """ +version = 1 + +[[datasets]] +key = "example" +group = "group" +url = "https://example.com/example.zip" +archive_name = "example.zip" +extracted_dir = "example" +source = "example source" +""", + encoding="utf-8", + ) + + with pytest.raises(ValueError, match="unknown root field"): + manifest.load_datasets(manifest_path) + + def test_rejects_missing_required_manifest_field(self, tmp_path: Path) -> None: + manifest_path = tmp_path / "datasets.toml" + manifest_path.write_text( + """ +[[datasets]] +key = "example" +group = "group" +url = "https://example.com/example.zip" +archive_name = "example.zip" +extracted_dir = "example" +""", + encoding="utf-8", + ) + + with pytest.raises(ValueError, match="missing required field"): + manifest.load_datasets(manifest_path) + + def test_rejects_unknown_manifest_field(self, tmp_path: Path) -> None: + manifest_path = tmp_path / "datasets.toml" + manifest_path.write_text( + """ +[[datasets]] +key = "example" +group = "group" +url = "https://example.com/example.zip" +archive_name = "example.zip" +extracted_dir = "example" +source = "example source" +checksum = "unexpected" +""", + encoding="utf-8", + ) + + with pytest.raises(ValueError, match="unknown field"): + manifest.load_datasets(manifest_path) class TestExtractZip: @@ -258,6 +361,17 @@ def download_dataset(dataset: Any, output: Path, force: bool) -> None: assert attempted == ["first", "third"] + def test_lists_available_datasets( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + dataset = _make_dataset("listed", group="group", extracted_dir="listed-dir", source="listed source") + + monkeypatch.setattr(download_test_data, "DATASETS", (dataset,)) + + download_test_data.main(["--list"]) + + assert capsys.readouterr().out == "listed\tgroup\tlisted-dir\tlisted source\n" + def test_continues_after_failure_then_exits_nonzero( self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: From ae792eec5daf1a330ee04b9028401f02e2b626c0 Mon Sep 17 00:00:00 2001 From: Kilian Paul <81327374+Darlokt@users.noreply.github.com> Date: Sat, 16 May 2026 07:23:50 +0200 Subject: [PATCH 5/7] Preserver the per dataset comments This commit updates the datasets.toml file to include comments for each dataset from the original CI. Changes: - Added comments for each dataset in the datasets.toml file to preserve the information from the original CI. --- scripts/test_data_downloader/datasets.toml | 26 ++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/scripts/test_data_downloader/datasets.toml b/scripts/test_data_downloader/datasets.toml index 9daaffb2..b578489d 100644 --- a/scripts/test_data_downloader/datasets.toml +++ b/scripts/test_data_downloader/datasets.toml @@ -1,3 +1,8 @@ +# ------- +# the Xenium datasets are licensed as CC BY 4.0, as shown here +# https://www.10xgenomics.com/support/software/xenium-onboard-analysis/latest/resources/xenium-example-data + +# 10x Genomics Xenium 2.0.0 [[datasets]] key = "xenium_breast" group = "xenium" @@ -6,6 +11,7 @@ archive_name = "Xenium_V1_human_Breast_2fov_outs.zip" extracted_dir = "Xenium_V1_human_Breast_2fov_outs" source = "10x Genomics Xenium 2.0.0, CC BY 4.0" +# 10x Genomics Xenium 2.0.0 [[datasets]] key = "xenium_lung" group = "xenium" @@ -14,6 +20,11 @@ archive_name = "Xenium_V1_human_Lung_2fov_outs.zip" extracted_dir = "Xenium_V1_human_Lung_2fov_outs" source = "10x Genomics Xenium 2.0.0, CC BY 4.0" +# 10x Genomics Xenium 3.0.0 (5K) Mouse ileum, multimodal cell segmentation +# this file seems to be corrupted; skipping it for now +# https://cf.10xgenomics.com/samples/xenium/3.0.0/Xenium_Prime_MultiCellSeg_Mouse_Ileum_tiny/Xenium_Prime_MultiCellSeg_Mouse_Ileum_tiny.zip + +# 10x Genomics Xenium 3.0.0 (5K) Mouse ileum, nuclear expansion [[datasets]] key = "xenium_prime_mouse_ileum" group = "xenium" @@ -22,6 +33,7 @@ archive_name = "Xenium_Prime_Mouse_Ileum_tiny_outs.zip" extracted_dir = "Xenium_Prime_Mouse_Ileum_tiny_outs" source = "10x Genomics Xenium 3.0.0, CC BY 4.0" +# 10x Genomics Xenium 4.0.0 (v1) Human ovary, nuclear expansion [[datasets]] key = "xenium_ovary" group = "xenium" @@ -30,6 +42,7 @@ archive_name = "Xenium_V1_Human_Ovary_tiny_outs.zip" extracted_dir = "Xenium_V1_Human_Ovary_tiny_outs" source = "10x Genomics Xenium 4.0.0, CC BY 4.0" +# 10x Genomics Xenium 4.0.0 (v1) Human ovary, multimodal cell segmentation [[datasets]] key = "xenium_multicell_ovary" group = "xenium" @@ -38,6 +51,7 @@ archive_name = "Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip" extracted_dir = "Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs" source = "10x Genomics Xenium 4.0.0, CC BY 4.0" +# 10x Genomics Xenium 4.0.0 (v1+Protein) Human kidney, multimodal cell segmentation [[datasets]] key = "xenium_protein_kidney" group = "xenium" @@ -46,6 +60,11 @@ archive_name = "Xenium_V1_Protein_Human_Kidney_tiny_outs.zip" extracted_dir = "Xenium_V1_Protein_Human_Kidney_tiny_outs" source = "10x Genomics Xenium 4.0.0, CC BY 4.0" +# ------- +# the Visium HD dataset is licensed as CC BY 4.0, as shown here +# https://www.10xgenomics.com/support/software/space-ranger/latest/resources/visium-hd-example-data + +# 10x Genomics Visium HD 4.0.1 3' Mouse Brain Chunk [[datasets]] key = "visium_hd_tiny" group = "visium_hd" @@ -54,6 +73,9 @@ archive_name = "Visium_HD_Tiny_3prime_Dataset_outs.zip" extracted_dir = "Visium_HD_Tiny_3prime_Dataset_outs" source = "10x Genomics Visium HD 4.0.1, CC BY 4.0" +# ------- +# we received written permission to make the following dataset public and integrate it in the CI system of spatialdata-io +# Spatial Genomics seqFISH v2 [[datasets]] key = "seqfish" group = "seqfish" @@ -63,6 +85,9 @@ extracted_dir = "seqfish-2-test-dataset" source = "Spatial Genomics seqFISH v2, public test data" test_path = "instrument 2 official" +# ------- +# MACSima OMAP datasets are licensed as CC BY 4.0 +# OMAP23 for format v1.x.x [[datasets]] key = "macsima_omap23" group = "macsima" @@ -71,6 +96,7 @@ archive_name = "OMAP23_small.zip" extracted_dir = "OMAP23_small" source = "MACSima OMAP23, CC BY 4.0" +# OMAP10 for format v0.x.x [[datasets]] key = "macsima_omap10" group = "macsima" From e38e0901163f0a4812524b24ed70ecbfc567328b Mon Sep 17 00:00:00 2001 From: Kilian Paul <81327374+Darlokt@users.noreply.github.com> Date: Sun, 30 Aug 2026 22:00:12 +0200 Subject: [PATCH 6/7] Rewrite downloader around pooch Rewrite downloader around pooch to align with scverse-misc but allowing integration into test framework, safe typing through TOML, and downloading of arbitrary data as needed for Data formats being tested, as it is not meant ot load data for a loader like the spatialdata loader, but load the data as a whole for testing etc. --- .github/workflows/prepare_test_data.yaml | 7 +- docs/contributing.md | 7 +- pyproject.toml | 3 + scripts/test_data_downloader/datasets.toml | 23 +- scripts/test_data_downloader/downloader.py | 121 +++----- scripts/test_data_downloader/manifest.py | 62 +++-- tests/unit/test_download_test_data.py | 305 ++++++++++++--------- 7 files changed, 284 insertions(+), 244 deletions(-) diff --git a/.github/workflows/prepare_test_data.yaml b/.github/workflows/prepare_test_data.yaml index 2e095fdd..0f4d2a81 100644 --- a/.github/workflows/prepare_test_data.yaml +++ b/.github/workflows/prepare_test_data.yaml @@ -25,6 +25,7 @@ on: - main paths: - ".github/workflows/prepare_test_data.yaml" + - "pyproject.toml" - "scripts/test_data_downloader/**" permissions: @@ -43,6 +44,10 @@ jobs: - name: Install uv uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 + with: + enable-cache: true + cache-dependency-glob: pyproject.toml + python-version: "3.13" - name: Download test datasets env: @@ -63,7 +68,7 @@ jobs: if [[ "${{ github.event_name }}" == "workflow_dispatch" && "${{ inputs.force_redownload }}" == "true" ]]; then args+=(--force) fi - uv run --python 3.13 python scripts/test_data_downloader "${args[@]}" + uv run --only-group dev python scripts/test_data_downloader "${args[@]}" - name: Upload artifacts uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 diff --git a/docs/contributing.md b/docs/contributing.md index ebda3dae..75f8f0ae 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -492,7 +492,12 @@ python scripts/test_data_downloader ``` By default, the downloader skips datasets that already exist. Use `--force` to redownload selected datasets, `--dataset` for a single dataset key, and `--list` to show the available keys. -The dataset registry lives in `scripts/test_data_downloader/datasets.toml`; append new entries there when adding or updating test datasets. +The downloader verifies every downloaded file with [Pooch](https://www.fatiando.org/pooch/). The dataset manifest lives in +`scripts/test_data_downloader/datasets.toml`; append new entries there when adding or updating test datasets. This manifest +stores project-specific metadata such as dataset keys, groups, output directories, and sources; it is not a separate Pooch +registry. Prefer a stable repository DOI when one is available. For DOI entries, Pooch loads the repository's per-file hashes +at runtime and verifies every downloaded file. Otherwise, register the archive URL and its SHA-256 hash as +`known_hash = "sha256:..."`. #### Testing multiple versions diff --git a/pyproject.toml b/pyproject.toml index 47e4851d..29c5d81d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,7 +52,9 @@ urls.Source = "https://github.com/scverse/spatialdata-io" dev = [ "mypy", "pandas-stubs", + "pooch>=1.9", "prek", + "requests", "scipy-stubs", "twine>=4.0.2", "types-geopandas", @@ -154,6 +156,7 @@ module = [ "dask_image.*", "h5py.*", "multiscale_spatial_image.*", + "pooch.*", "pyarrow.*", "rasterio.*", "readfcs.*", diff --git a/scripts/test_data_downloader/datasets.toml b/scripts/test_data_downloader/datasets.toml index b578489d..778e5679 100644 --- a/scripts/test_data_downloader/datasets.toml +++ b/scripts/test_data_downloader/datasets.toml @@ -7,7 +7,7 @@ key = "xenium_breast" group = "xenium" url = "https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Breast_2fov/Xenium_V1_human_Breast_2fov_outs.zip" -archive_name = "Xenium_V1_human_Breast_2fov_outs.zip" +known_hash = "sha256:cc1e987b06aa748a6b24d3d6f51fc0d6765daa4836c483f14ec4f1bd18b1779b" extracted_dir = "Xenium_V1_human_Breast_2fov_outs" source = "10x Genomics Xenium 2.0.0, CC BY 4.0" @@ -16,7 +16,7 @@ source = "10x Genomics Xenium 2.0.0, CC BY 4.0" key = "xenium_lung" group = "xenium" url = "https://cf.10xgenomics.com/samples/xenium/2.0.0/Xenium_V1_human_Lung_2fov/Xenium_V1_human_Lung_2fov_outs.zip" -archive_name = "Xenium_V1_human_Lung_2fov_outs.zip" +known_hash = "sha256:acc353069871eeda9977fc80f0d11e81eef8b7a683212819cd892d983c4e4d91" extracted_dir = "Xenium_V1_human_Lung_2fov_outs" source = "10x Genomics Xenium 2.0.0, CC BY 4.0" @@ -29,7 +29,7 @@ source = "10x Genomics Xenium 2.0.0, CC BY 4.0" key = "xenium_prime_mouse_ileum" group = "xenium" url = "https://cf.10xgenomics.com/samples/xenium/3.0.0/Xenium_Prime_Mouse_Ileum_tiny/Xenium_Prime_Mouse_Ileum_tiny_outs.zip" -archive_name = "Xenium_Prime_Mouse_Ileum_tiny_outs.zip" +known_hash = "sha256:72dc2353825f6049959fa85e3c0617abd2a7cfb1c19531a2bba2d246fa276cd3" extracted_dir = "Xenium_Prime_Mouse_Ileum_tiny_outs" source = "10x Genomics Xenium 3.0.0, CC BY 4.0" @@ -38,7 +38,7 @@ source = "10x Genomics Xenium 3.0.0, CC BY 4.0" key = "xenium_ovary" group = "xenium" url = "https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Human_Ovary_tiny/Xenium_V1_Human_Ovary_tiny_outs.zip" -archive_name = "Xenium_V1_Human_Ovary_tiny_outs.zip" +known_hash = "sha256:72b9a6d73ec428dd823e9a0d1e3b70d6f0538ec79f67fa7591f034af0894c764" extracted_dir = "Xenium_V1_Human_Ovary_tiny_outs" source = "10x Genomics Xenium 4.0.0, CC BY 4.0" @@ -47,7 +47,7 @@ source = "10x Genomics Xenium 4.0.0, CC BY 4.0" key = "xenium_multicell_ovary" group = "xenium" url = "https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_MultiCellSeg_Human_Ovary_tiny/Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip" -archive_name = "Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs.zip" +known_hash = "sha256:9b154a3c08360fe690c83e0e6a6710d0c5656ec93a6e3ff969b3950d291479b0" extracted_dir = "Xenium_V1_MultiCellSeg_Human_Ovary_tiny_outs" source = "10x Genomics Xenium 4.0.0, CC BY 4.0" @@ -56,7 +56,7 @@ source = "10x Genomics Xenium 4.0.0, CC BY 4.0" key = "xenium_protein_kidney" group = "xenium" url = "https://cf.10xgenomics.com/samples/xenium/4.0.0/Xenium_V1_Protein_Human_Kidney_tiny/Xenium_V1_Protein_Human_Kidney_tiny_outs.zip" -archive_name = "Xenium_V1_Protein_Human_Kidney_tiny_outs.zip" +known_hash = "sha256:abd7e8f7fd047dcc6afdb1e9eece90d4533d3ead053c6f05c482be050bdf79d2" extracted_dir = "Xenium_V1_Protein_Human_Kidney_tiny_outs" source = "10x Genomics Xenium 4.0.0, CC BY 4.0" @@ -69,7 +69,7 @@ source = "10x Genomics Xenium 4.0.0, CC BY 4.0" key = "visium_hd_tiny" group = "visium_hd" url = "https://cf.10xgenomics.com/samples/spatial-exp/4.0.1/Visium_HD_Tiny_3prime_Dataset/Visium_HD_Tiny_3prime_Dataset_outs.zip" -archive_name = "Visium_HD_Tiny_3prime_Dataset_outs.zip" +known_hash = "sha256:38be766fc4fa077f083b74d9a6746ab6db985838dbbba3006b5a36c51a3c5a50" extracted_dir = "Visium_HD_Tiny_3prime_Dataset_outs" source = "10x Genomics Visium HD 4.0.1, CC BY 4.0" @@ -80,19 +80,19 @@ source = "10x Genomics Visium HD 4.0.1, CC BY 4.0" key = "seqfish" group = "seqfish" url = "https://s3.embl.de/spatialdata/raw_data/seqfish-2-test-dataset.zip" -archive_name = "seqfish-2-test-dataset.zip" +known_hash = "sha256:553208b2bab71e45fe0fd3799478aa03894270c8e447cecc6366c0bd412256c0" extracted_dir = "seqfish-2-test-dataset" source = "Spatial Genomics seqFISH v2, public test data" test_path = "instrument 2 official" # ------- # MACSima OMAP datasets are licensed as CC BY 4.0 +# Pooch loads and verifies the published per-file MD5 checksums from each stable DOI at runtime. # OMAP23 for format v1.x.x [[datasets]] key = "macsima_omap23" group = "macsima" -url = "https://zenodo.org/api/records/18196452/files-archive" -archive_name = "OMAP23_small.zip" +doi = "10.5281/zenodo.18196452" extracted_dir = "OMAP23_small" source = "MACSima OMAP23, CC BY 4.0" @@ -100,7 +100,6 @@ source = "MACSima OMAP23, CC BY 4.0" [[datasets]] key = "macsima_omap10" group = "macsima" -url = "https://zenodo.org/api/records/18196366/files-archive" -archive_name = "OMAP10_small.zip" +doi = "10.5281/zenodo.18196366" extracted_dir = "OMAP10_small" source = "MACSima OMAP10, CC BY 4.0" diff --git a/scripts/test_data_downloader/downloader.py b/scripts/test_data_downloader/downloader.py index 67fabd82..546c28c6 100644 --- a/scripts/test_data_downloader/downloader.py +++ b/scripts/test_data_downloader/downloader.py @@ -5,42 +5,28 @@ import argparse import shutil import tempfile -import time -import urllib.error -import urllib.request import zipfile from pathlib import Path from typing import TYPE_CHECKING +import pooch +import requests from manifest import DATASETS, TestDataset, validate_datasets if TYPE_CHECKING: from collections.abc import Sequence -# Some providers reject Python's default user agent even for public files. -REQUEST_HEADERS = { - "User-Agent": "curl/8.0.0", - "Accept": "*/*", -} -DOWNLOAD_TIMEOUT_SEC = 60 -MAX_DOWNLOAD_ATTEMPTS = 3 -RETRY_DELAY_SEC = 2.0 -TRANSIENT_HTTP_STATUS_CODES = {408, 429} +DOWNLOAD_RETRIES = 2 class DatasetDownloadError(RuntimeError): """Error raised when a test dataset cannot be downloaded or extracted.""" - def __init__(self, dataset: TestDataset, action: str, reason: str, suggestion: str | None = None) -> None: + def __init__(self, dataset: TestDataset, reason: str) -> None: self.dataset = dataset - self.action = action self.reason = reason - self.suggestion = suggestion - message = f"{dataset.key}: failed to {action} {dataset.url}: {reason}" - if suggestion is not None: - message = f"{message}. {suggestion}" - super().__init__(message) + super().__init__(f"{dataset.key}: {reason}") def download_dataset(dataset: TestDataset, output: Path, force: bool) -> None: @@ -59,7 +45,7 @@ def download_dataset(dataset: TestDataset, output: Path, force: bool) -> None: Raises ------ DatasetDownloadError - If the archive cannot be downloaded, extracted, or validated. + If the dataset files cannot be downloaded, extracted, or validated. """ target = output / dataset.extracted_dir if target.exists() and not force: @@ -69,18 +55,19 @@ def download_dataset(dataset: TestDataset, output: Path, force: bool) -> None: output.mkdir(parents=True, exist_ok=True) with tempfile.TemporaryDirectory(prefix=f"{dataset.key}-", dir=output) as tmpdir: tmp_path = Path(tmpdir) - archive_path = tmp_path / dataset.archive_name extracted_path = tmp_path / dataset.extracted_dir extracted_path.mkdir() # Stage work in a temporary directory so interrupted downloads do not leave partial datasets. - _download(dataset, archive_path) - _extract_zip(dataset, archive_path, extracted_path) + try: + _fetch_dataset(dataset, tmp_path, extracted_path) + except (requests.exceptions.RequestException, ValueError, zipfile.BadZipFile, OSError) as exc: + raise DatasetDownloadError(dataset, str(exc)) from exc + if not extracted_path.is_dir() or not any(extracted_path.iterdir()): raise DatasetDownloadError( dataset, - "extract", - f"archive did not produce expected directory {dataset.extracted_dir!r}", + f"download did not produce expected directory {dataset.extracted_dir!r}", ) if target.exists(): @@ -146,63 +133,29 @@ def _selected_datasets(dataset_keys: list[str] | None, groups: list[str] | None) return [dataset for dataset in DATASETS if dataset.key in selected_keys or dataset.group in selected_groups] -def _is_transient_error(error: BaseException) -> bool: - """Return whether a download error is worth retrying.""" - if isinstance(error, urllib.error.HTTPError): - return error.code in TRANSIENT_HTTP_STATUS_CODES or 500 <= error.code < 600 - return isinstance(error, urllib.error.URLError | TimeoutError) - +def _fetch_dataset(dataset: TestDataset, temporary_path: Path, extracted_path: Path) -> None: + """Fetch and verify a dataset using its Pooch registry.""" + if dataset.doi: + manager = pooch.create( + path=extracted_path, + base_url=f"doi:{dataset.doi}/", + registry={}, + retry_if_failed=DOWNLOAD_RETRIES, + ) + # DOI repositories publish a per-file registry; Pooch loads its hashes and verifies each fetch. + manager.load_registry_from_doi() + if not manager.registry_files: + raise ValueError(f"DOI repository {dataset.doi!r} contains no files") + for file_name in manager.registry_files: + manager.fetch(file_name) + return -def _download_error(dataset: TestDataset, error: BaseException) -> DatasetDownloadError: - """Translate urllib and timeout failures into dataset-aware errors.""" - if isinstance(error, urllib.error.HTTPError): - reason = f"HTTP {error.code} {error.reason}" - suggestion = None - if error.code == 403: - suggestion = ( - "The server rejected the request. The downloader sends a curl-like User-Agent; " - "if this persists, verify the URL in a browser or with curl and check whether the provider changed access rules" - ) - return DatasetDownloadError(dataset, "download", reason, suggestion) - if isinstance(error, urllib.error.URLError): - return DatasetDownloadError(dataset, "download", f"URL error: {error.reason}") - if isinstance(error, TimeoutError): - return DatasetDownloadError(dataset, "download", f"timed out after {DOWNLOAD_TIMEOUT_SEC} seconds") - return DatasetDownloadError(dataset, "download", str(error)) - - -def _download(dataset: TestDataset, archive_path: Path) -> None: - """Download ``dataset`` to ``archive_path`` with bounded retries.""" - print(f"Downloading {dataset.key} from {dataset.url}") - for attempt in range(1, MAX_DOWNLOAD_ATTEMPTS + 1): - request = urllib.request.Request(dataset.url, headers=REQUEST_HEADERS) - try: - with urllib.request.urlopen(request, timeout=DOWNLOAD_TIMEOUT_SEC) as response: - with archive_path.open("wb") as handle: - shutil.copyfileobj(response, handle) - return - except (urllib.error.HTTPError, urllib.error.URLError, TimeoutError) as exc: - if attempt < MAX_DOWNLOAD_ATTEMPTS and _is_transient_error(exc): - print(f"Retrying {dataset.key} after download failure ({attempt}/{MAX_DOWNLOAD_ATTEMPTS}): {exc}") - time.sleep(RETRY_DELAY_SEC) - continue - raise _download_error(dataset, exc) from exc - - -def _extract_zip(dataset: TestDataset, archive_path: Path, destination: Path) -> None: - """Extract ``archive_path`` into ``destination`` after validating member paths.""" - try: - with zipfile.ZipFile(archive_path) as archive: - destination_root = destination.resolve() - for member in archive.infolist(): - # Guard against zip-slip archives that contain absolute paths or ``..`` components. - member_path = (destination / member.filename).resolve() - if not member_path.is_relative_to(destination_root): - raise DatasetDownloadError( - dataset, - "extract", - f"archive member {member.filename!r} would be extracted outside {destination}", - ) - archive.extractall(destination) - except zipfile.BadZipFile as exc: - raise DatasetDownloadError(dataset, "extract", "downloaded file is not a valid zip archive") from exc + archive_name = f"{dataset.key}.zip" + manager = pooch.create( + path=temporary_path, + base_url="", + registry={archive_name: dataset.known_hash}, + urls={archive_name: dataset.url}, + retry_if_failed=DOWNLOAD_RETRIES, + ) + manager.fetch(archive_name, processor=pooch.Unzip(extract_dir=dataset.extracted_dir)) diff --git a/scripts/test_data_downloader/manifest.py b/scripts/test_data_downloader/manifest.py index 843564f5..8a176cf9 100644 --- a/scripts/test_data_downloader/manifest.py +++ b/scripts/test_data_downloader/manifest.py @@ -2,6 +2,7 @@ from __future__ import annotations +import re import tomllib from dataclasses import dataclass from functools import cache @@ -9,9 +10,10 @@ from typing import Any DATASETS_TOML = Path(__file__).with_name("datasets.toml") -REQUIRED_FIELDS = frozenset({"key", "group", "url", "archive_name", "extracted_dir", "source"}) -OPTIONAL_FIELDS = frozenset({"test_path"}) -ALLOWED_FIELDS = REQUIRED_FIELDS | OPTIONAL_FIELDS +COMMON_REQUIRED_FIELDS = frozenset({"key", "group", "extracted_dir", "source"}) +OPTIONAL_FIELDS = frozenset({"test_path", "url", "known_hash", "doi"}) +ALLOWED_FIELDS = COMMON_REQUIRED_FIELDS | OPTIONAL_FIELDS +KNOWN_HASH_PATTERN = re.compile(r"sha256:[0-9a-fA-F]{64}") @dataclass(frozen=True) @@ -26,26 +28,31 @@ class TestDataset: group : str Logical grouping of the dataset, for example ``"xenium"``, ``"visium_hd"``, ``"seqfish"``, or ``"macsima"``. - url : str - Direct URL to the dataset archive, for example a ZIP file. - archive_name : str - Expected filename of the downloaded archive. extracted_dir : str - Expected name of the directory created when the archive is extracted. + Name of the final directory containing the downloaded dataset. source : str Human-readable description of the dataset source and license. test_path : str Optional path inside the extracted directory that should be passed to integration tests. + url : str + Direct URL to a ZIP archive. Used together with ``known_hash`` when a + DOI is unavailable. + known_hash : str + Pooch-compatible SHA-256 hash of the dataset archive. + doi : str + DOI for a repository whose files and published hashes should be loaded + through Pooch instead of downloading a single archive. """ key: str group: str - url: str - archive_name: str extracted_dir: str source: str test_path: str = "" + url: str = "" + known_hash: str = "" + doi: str = "" def load_datasets(path: str | Path = DATASETS_TOML) -> tuple[TestDataset, ...]: @@ -81,8 +88,9 @@ def validate_datasets(datasets: tuple[TestDataset, ...] | None = None) -> None: Raises ------ ValueError - If a required field is empty, a key or extracted directory is - duplicated, or ``test_path`` points outside the extracted directory. + If a required field is empty, a hash or path is invalid, a key or + extracted directory is duplicated, or the download source is + incomplete or ambiguous. """ if datasets is None: datasets = DATASETS @@ -91,14 +99,28 @@ def validate_datasets(datasets: tuple[TestDataset, ...] | None = None) -> None: seen_extracted_dirs: set[str] = set() for dataset in datasets: - for field_name in REQUIRED_FIELDS: + for field_name in COMMON_REQUIRED_FIELDS: value = getattr(dataset, field_name) if not isinstance(value, str): raise ValueError(f"Dataset {dataset.key!r} field {field_name!r} must be a string.") if not value.strip(): raise ValueError(f"Dataset {dataset.key!r} has empty {field_name}.") - if not isinstance(dataset.test_path, str): - raise ValueError(f"Dataset {dataset.key!r} field 'test_path' must be a string.") + + for field_name in OPTIONAL_FIELDS: + if not isinstance(getattr(dataset, field_name), str): + raise ValueError(f"Dataset {dataset.key!r} field {field_name!r} must be a string.") + + has_url = bool(dataset.url.strip()) + has_hash = bool(dataset.known_hash.strip()) + has_doi = bool(dataset.doi.strip()) + if has_doi and (has_url or has_hash): + raise ValueError(f"Dataset {dataset.key!r} cannot define both an archive and a DOI.") + if not has_doi and not (has_url and has_hash): + raise ValueError(f"Dataset {dataset.key!r} must define either doi or both url and known_hash.") + if has_url: + _validate_known_hash(dataset.key, dataset.known_hash) + if has_doi and (not dataset.doi.startswith("10.") or any(character.isspace() for character in dataset.doi)): + raise ValueError(f"Dataset {dataset.key!r} doi must be a DOI without a 'doi:' prefix.") test_path = PurePath(dataset.test_path) # Test paths are appended to extracted_dir by tests, so they must stay inside that directory. @@ -142,18 +164,24 @@ def _parse_dataset(raw_dataset: object, index: int) -> TestDataset: unknown = ", ".join(sorted(unknown_fields)) raise ValueError(f"Dataset manifest entry at index {index} has unknown field(s): {unknown}.") - missing_fields = REQUIRED_FIELDS - dataset_fields + missing_fields = COMMON_REQUIRED_FIELDS - dataset_fields if missing_fields: missing = ", ".join(sorted(missing_fields)) raise ValueError(f"Dataset manifest entry at index {index} is missing required field(s): {missing}.") values: dict[str, Any] = dict(raw_dataset) - values.setdefault("test_path", "") + for field_name in OPTIONAL_FIELDS: + values.setdefault(field_name, "") dataset = TestDataset(**values) validate_datasets((dataset,)) return dataset +def _validate_known_hash(dataset_key: str, known_hash: str) -> None: + if not KNOWN_HASH_PATTERN.fullmatch(known_hash): + raise ValueError(f"Dataset {dataset_key!r} known_hash must be formatted as 'sha256:<64 hex characters>'.") + + # Keep keys and groups stable because CI workflows and pytest markers may refer to them. DATASETS = load_datasets() diff --git a/tests/unit/test_download_test_data.py b/tests/unit/test_download_test_data.py index 70b7c505..90daa586 100644 --- a/tests/unit/test_download_test_data.py +++ b/tests/unit/test_download_test_data.py @@ -1,22 +1,20 @@ from __future__ import annotations import importlib.util -import io import sys -import urllib.error -import urllib.request -import zipfile -from email.message import Message from pathlib import Path from typing import TYPE_CHECKING, Any +import pooch import pytest +import requests if TYPE_CHECKING: from types import ModuleType SCRIPT_DIR = Path(__file__).parents[2] / "scripts" / "test_data_downloader" +SHA256 = "sha256:" + "0" * 64 def _load_module(module_name: str, module_path: Path) -> ModuleType: @@ -38,90 +36,114 @@ def _make_dataset( *, group: str = "group", url: str | None = None, - archive_name: str | None = None, extracted_dir: str | None = None, source: str = "example", test_path: str = "", + doi: str = "", ) -> Any: return download_test_data.TestDataset( key=key, group=group, - url=url or f"https://example.com/{key}.zip", - archive_name=archive_name or f"{key}.zip", extracted_dir=extracted_dir or key, source=source, test_path=test_path, + url="" if doi else url or f"https://example.com/{key}.zip", + known_hash="" if doi else SHA256, + doi=doi, ) -def _write_zip(path: Path) -> None: - with zipfile.ZipFile(path, "w") as archive: - archive.writestr("payload.txt", "ok") - - -class _BytesResponse(io.BytesIO): - def __enter__(self) -> _BytesResponse: - return self - - def __exit__(self, exc_type: object, exc: object, traceback: object) -> None: - self.close() - - -class TestDownload: +class TestFetchDataset: def test_uses_dataset_manifest_module(self) -> None: assert download_test_data.TestDataset.__module__ == "manifest" assert all(isinstance(dataset, manifest.TestDataset) for dataset in download_test_data.DATASETS) - def test_sends_explicit_headers(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: - seen_request: urllib.request.Request | None = None - - def urlopen(request: urllib.request.Request, timeout: int) -> _BytesResponse: - nonlocal seen_request - seen_request = request - assert timeout == download_test_data.DOWNLOAD_TIMEOUT_SEC - return _BytesResponse(b"payload") + def test_fetches_archive_with_pooch_registry_and_unzip( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + dataset = _make_dataset() + seen_create: dict[str, Any] = {} + seen_fetch: dict[str, Any] = {} + + class Manager: + def fetch(self, file_name: str, processor: Any = None) -> None: + seen_fetch.update(file_name=file_name, processor=processor) + + def create(**kwargs: Any) -> Manager: + seen_create.update(kwargs) + return Manager() + + monkeypatch.setattr(download_test_data.pooch, "create", create) + + download_test_data._fetch_dataset(dataset, tmp_path, tmp_path / dataset.extracted_dir) + + archive_name = f"{dataset.key}.zip" + assert seen_create == { + "path": tmp_path, + "base_url": "", + "registry": {archive_name: dataset.known_hash}, + "urls": {archive_name: dataset.url}, + "retry_if_failed": download_test_data.DOWNLOAD_RETRIES, + } + assert seen_fetch["file_name"] == archive_name + assert isinstance(seen_fetch["processor"], pooch.Unzip) + assert seen_fetch["processor"].extract_dir == dataset.extracted_dir + + def test_loads_hashes_and_fetches_all_files_from_doi_registry( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + dataset = _make_dataset("doi", doi="10.5281/zenodo.123") + fetched: list[str] = [] + registry_loaded = False - monkeypatch.setattr(download_test_data.urllib.request, "urlopen", urlopen) + class Manager: + registry: dict[str, str] = {} - download_test_data._download(_make_dataset(), tmp_path / "archive.zip") + @property + def registry_files(self) -> list[str]: + return list(self.registry) - assert seen_request is not None - assert seen_request.get_header("User-agent") == "curl/8.0.0" - assert seen_request.get_header("Accept") == "*/*" + def load_registry_from_doi(self) -> None: + nonlocal registry_loaded + registry_loaded = True + self.registry = { + "first.tif": "md5:" + "0" * 32, + "second.tif": "md5:" + "1" * 32, + } - def test_reports_http_403_with_actionable_message(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: - dataset = _make_dataset("forbidden") + def fetch(self, file_name: str) -> None: + assert self.registry[file_name].startswith("md5:") + fetched.append(file_name) - def urlopen(request: object, timeout: int) -> None: - raise urllib.error.HTTPError(dataset.url, 403, "Forbidden", hdrs=Message(), fp=None) + def create(**kwargs: Any) -> Manager: + assert kwargs == { + "path": tmp_path / dataset.extracted_dir, + "base_url": "doi:10.5281/zenodo.123/", + "registry": {}, + "retry_if_failed": download_test_data.DOWNLOAD_RETRIES, + } + return Manager() - monkeypatch.setattr(download_test_data.urllib.request, "urlopen", urlopen) + monkeypatch.setattr(download_test_data.pooch, "create", create) - with pytest.raises(download_test_data.DatasetDownloadError, match="HTTP 403 Forbidden") as exc_info: - download_test_data._download(dataset, tmp_path / dataset.archive_name) + download_test_data._fetch_dataset(dataset, tmp_path, tmp_path / dataset.extracted_dir) - message = str(exc_info.value) - assert dataset.key in message - assert dataset.url in message - assert "rejected the request" in message + assert registry_loaded + assert fetched == ["first.tif", "second.tif"] - def test_retries_transient_failures(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: - dataset = _make_dataset("retry") - attempts = 0 + def test_rejects_empty_doi_registry(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + dataset = _make_dataset("doi", doi="10.5281/zenodo.123") - def urlopen(request: object, timeout: int) -> _BytesResponse: - nonlocal attempts - attempts += 1 - if attempts == 1: - raise urllib.error.HTTPError(dataset.url, 503, "Service Unavailable", hdrs=Message(), fp=None) - return _BytesResponse(b"payload") + class Manager: + registry_files: list[str] = [] - monkeypatch.setattr(download_test_data.urllib.request, "urlopen", urlopen) - monkeypatch.setattr(download_test_data.time, "sleep", lambda seconds: None) + def load_registry_from_doi(self) -> None: + return None - download_test_data._download(dataset, tmp_path / dataset.archive_name) + monkeypatch.setattr(download_test_data.pooch, "create", lambda **kwargs: Manager()) - assert attempts == 2 + with pytest.raises(ValueError, match="contains no files"): + download_test_data._fetch_dataset(dataset, tmp_path, tmp_path / dataset.extracted_dir) class TestDatasetManifest: @@ -131,35 +153,31 @@ def test_manifest_is_valid(self) -> None: def test_returns_dataset_by_key(self) -> None: dataset = manifest.get_dataset("seqfish") - assert dataset.key == "seqfish" assert dataset.extracted_dir == "seqfish-2-test-dataset" + assert dataset.known_hash.startswith("sha256:") def test_reports_unknown_dataset_key(self) -> None: with pytest.raises(KeyError, match="Unknown test dataset key 'missing'"): manifest.get_dataset("missing") - def test_test_path_defaults_to_extracted_directory_root(self) -> None: - dataset = manifest.get_dataset("xenium_breast") - - assert dataset.test_path == "" - def test_can_filter_datasets_by_group(self) -> None: datasets = manifest.datasets_by_group("macsima") assert {dataset.key for dataset in datasets} == {"macsima_omap10", "macsima_omap23"} + assert all(dataset.doi for dataset in datasets) - def test_loads_datasets_from_toml(self, tmp_path: Path) -> None: + def test_loads_archive_dataset_from_toml(self, tmp_path: Path) -> None: manifest_path = tmp_path / "datasets.toml" manifest_path.write_text( - """ + f""" [[datasets]] key = "example" group = "group" -url = "https://example.com/example.zip" -archive_name = "example.zip" extracted_dir = "example" source = "example source" test_path = "nested" +url = "https://example.com/example.zip" +known_hash = "{SHA256}" """, encoding="utf-8", ) @@ -170,14 +188,34 @@ def test_loads_datasets_from_toml(self, tmp_path: Path) -> None: manifest.TestDataset( key="example", group="group", - url="https://example.com/example.zip", - archive_name="example.zip", extracted_dir="example", source="example source", test_path="nested", + url="https://example.com/example.zip", + known_hash=SHA256, ), ) + def test_loads_doi_dataset_from_toml(self, tmp_path: Path) -> None: + manifest_path = tmp_path / "datasets.toml" + manifest_path.write_text( + """ +[[datasets]] +key = "example" +group = "group" +extracted_dir = "example" +source = "example source" +doi = "10.5281/zenodo.123" +""", + encoding="utf-8", + ) + + (dataset,) = manifest.load_datasets(manifest_path) + + assert dataset.url == "" + assert dataset.known_hash == "" + assert dataset.doi == "10.5281/zenodo.123" + def test_rejects_duplicate_dataset_keys(self) -> None: first = _make_dataset("duplicate", extracted_dir="first") second = _make_dataset("duplicate", extracted_dir="second") @@ -204,6 +242,43 @@ def test_rejects_test_path_outside_extracted_dir(self) -> None: with pytest.raises(ValueError, match="test_path must be a relative path"): manifest.validate_datasets((dataset,)) + def test_rejects_invalid_known_hash(self) -> None: + dataset = _make_dataset("invalid-hash") + dataset = manifest.TestDataset( + key=dataset.key, + group=dataset.group, + extracted_dir=dataset.extracted_dir, + source=dataset.source, + url=dataset.url, + known_hash="sha256:not-a-hash", + ) + + with pytest.raises(ValueError, match="known_hash"): + manifest.validate_datasets((dataset,)) + + @pytest.mark.parametrize( + ("url", "known_hash", "doi"), + [ + ("", "", ""), + ("https://example.com/example.zip", "", ""), + ("", SHA256, ""), + ("https://example.com/example.zip", SHA256, "10.5281/zenodo.123"), + ], + ) + def test_rejects_incomplete_or_ambiguous_download_source(self, url: str, known_hash: str, doi: str) -> None: + dataset = manifest.TestDataset( + key="example", + group="group", + extracted_dir="example", + source="example", + url=url, + known_hash=known_hash, + doi=doi, + ) + + with pytest.raises(ValueError, match="either doi|both an archive"): + manifest.validate_datasets((dataset,)) + def test_rejects_manifest_without_datasets_array(self, tmp_path: Path) -> None: manifest_path = tmp_path / "datasets.toml" manifest_path.write_text("datasets = { key = 'example' }\n", encoding="utf-8") @@ -220,20 +295,7 @@ def test_rejects_invalid_toml(self, tmp_path: Path) -> None: def test_rejects_unknown_root_manifest_field(self, tmp_path: Path) -> None: manifest_path = tmp_path / "datasets.toml" - manifest_path.write_text( - """ -version = 1 - -[[datasets]] -key = "example" -group = "group" -url = "https://example.com/example.zip" -archive_name = "example.zip" -extracted_dir = "example" -source = "example source" -""", - encoding="utf-8", - ) + manifest_path.write_text("version = 1\ndatasets = []\n", encoding="utf-8") with pytest.raises(ValueError, match="unknown root field"): manifest.load_datasets(manifest_path) @@ -241,13 +303,13 @@ def test_rejects_unknown_root_manifest_field(self, tmp_path: Path) -> None: def test_rejects_missing_required_manifest_field(self, tmp_path: Path) -> None: manifest_path = tmp_path / "datasets.toml" manifest_path.write_text( - """ + f""" [[datasets]] key = "example" group = "group" -url = "https://example.com/example.zip" -archive_name = "example.zip" extracted_dir = "example" +url = "https://example.com/example.zip" +known_hash = "{SHA256}" """, encoding="utf-8", ) @@ -258,14 +320,14 @@ def test_rejects_missing_required_manifest_field(self, tmp_path: Path) -> None: def test_rejects_unknown_manifest_field(self, tmp_path: Path) -> None: manifest_path = tmp_path / "datasets.toml" manifest_path.write_text( - """ + f""" [[datasets]] key = "example" group = "group" -url = "https://example.com/example.zip" -archive_name = "example.zip" extracted_dir = "example" -source = "example source" +source = "example" +url = "https://example.com/example.zip" +known_hash = "{SHA256}" checksum = "unexpected" """, encoding="utf-8", @@ -275,37 +337,17 @@ def test_rejects_unknown_manifest_field(self, tmp_path: Path) -> None: manifest.load_datasets(manifest_path) -class TestExtractZip: - def test_reports_invalid_archive(self, tmp_path: Path) -> None: - archive_path = tmp_path / "bad.zip" - archive_path.write_bytes(b"not a zip") - - with pytest.raises(download_test_data.DatasetDownloadError, match="not a valid zip archive"): - download_test_data._extract_zip(_make_dataset(), archive_path, tmp_path / "extract") - - def test_rejects_archive_members_outside_destination(self, tmp_path: Path) -> None: - archive_path = tmp_path / "unsafe.zip" - with zipfile.ZipFile(archive_path, "w") as archive: - archive.writestr("../escape.txt", "bad") - - with pytest.raises(download_test_data.DatasetDownloadError, match="outside"): - download_test_data._extract_zip(_make_dataset(), archive_path, tmp_path / "extract") - - assert not (tmp_path / "escape.txt").exists() - - class TestDownloadDataset: def test_skips_existing_dataset_without_force( self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: dataset = _make_dataset("existing") - target = tmp_path / dataset.extracted_dir - target.mkdir() - - def fail_download(dataset: object, archive_path: Path) -> None: - raise AssertionError("download should have been skipped") - - monkeypatch.setattr(download_test_data, "_download", fail_download) + (tmp_path / dataset.extracted_dir).mkdir() + monkeypatch.setattr( + download_test_data, + "_fetch_dataset", + lambda *args: pytest.fail("download should have been skipped"), + ) download_test_data.download_dataset(dataset, tmp_path, force=False) @@ -317,24 +359,30 @@ def test_replaces_existing_dataset_with_force(self, monkeypatch: pytest.MonkeyPa target.mkdir() (target / "old.txt").write_text("old", encoding="utf-8") - def write_archive(dataset: object, archive_path: Path) -> None: - _write_zip(archive_path) + def fetch(dataset: object, temporary_path: Path, extracted_path: Path) -> None: + (extracted_path / "payload.txt").write_text("ok", encoding="utf-8") - monkeypatch.setattr(download_test_data, "_download", write_archive) + monkeypatch.setattr(download_test_data, "_fetch_dataset", fetch) download_test_data.download_dataset(dataset, tmp_path, force=True) assert not (target / "old.txt").exists() assert (target / "payload.txt").read_text(encoding="utf-8") == "ok" - def test_reports_archive_without_expected_contents(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: - dataset = _make_dataset("empty") + def test_wraps_pooch_download_errors(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + dataset = _make_dataset("error") - def write_empty_archive(dataset: object, archive_path: Path) -> None: - with zipfile.ZipFile(archive_path, "w"): - pass + def fetch(dataset: object, temporary_path: Path, extracted_path: Path) -> None: + raise requests.HTTPError("403 Client Error") - monkeypatch.setattr(download_test_data, "_download", write_empty_archive) + monkeypatch.setattr(download_test_data, "_fetch_dataset", fetch) + + with pytest.raises(download_test_data.DatasetDownloadError, match="error: 403 Client Error"): + download_test_data.download_dataset(dataset, tmp_path, force=False) + + def test_reports_download_without_expected_contents(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + dataset = _make_dataset("empty") + monkeypatch.setattr(download_test_data, "_fetch_dataset", lambda *args: None) with pytest.raises(download_test_data.DatasetDownloadError, match="expected directory"): download_test_data.download_dataset(dataset, tmp_path, force=False) @@ -365,7 +413,6 @@ def test_lists_available_datasets( self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: dataset = _make_dataset("listed", group="group", extracted_dir="listed-dir", source="listed source") - monkeypatch.setattr(download_test_data, "DATASETS", (dataset,)) download_test_data.main(["--list"]) @@ -382,7 +429,7 @@ def test_continues_after_failure_then_exits_nonzero( def download_dataset(dataset: Any, output: Path, force: bool) -> None: attempted.append(dataset.key) if dataset == first: - raise download_test_data.DatasetDownloadError(dataset, "download", "HTTP 403 Forbidden") + raise download_test_data.DatasetDownloadError(dataset, "HTTP 403 Forbidden") monkeypatch.setattr(download_test_data, "DATASETS", (first, second)) monkeypatch.setattr(download_test_data, "download_dataset", download_dataset) From 30deedf863be7ef24c57b07c29e289231490d0ac Mon Sep 17 00:00:00 2001 From: Kilian Paul <81327374+Darlokt@users.noreply.github.com> Date: Wed, 2 Sep 2026 02:37:37 +0200 Subject: [PATCH 7/7] Expand optional test data downloader support --- docs/contributing.md | 10 +- scripts/test_data_downloader/README.md | 203 ++++++++++++++ scripts/test_data_downloader/downloader.py | 142 +++++++--- scripts/test_data_downloader/manifest.py | 304 +++++++++++++++------ tests/unit/test_download_test_data.py | 212 +++++++++++++- 5 files changed, 750 insertions(+), 121 deletions(-) create mode 100644 scripts/test_data_downloader/README.md diff --git a/docs/contributing.md b/docs/contributing.md index 75f8f0ae..deca9807 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -390,7 +390,7 @@ We recommend studying existing readers and reusing code from them. A few technic - Large raster or points data is usually loaded from disk lazily (e.g. with `dask_image.imread()`), which allows returning a `SpatialData` object quickly and defers computation when saving the object to disk in the SpatialData Zarr format, with `sdata.write()`. - When the raw data has multiple samples, we recommend adding a coordinate system for each sample, and if the samples are aligned in space, one common coordinate system. A single table containing the annotation for all samples is preferred. See an example in the [`cosmx()`](https://github.com/scverse/spatialdata-io/blob/main/src/spatialdata_io/readers/cosmx.py) reader. -- Small images should be represented as single-scale images (`xarray.DataArray`), large images as multiscale images (`xarray.DataTree`). The scale factors and chunk shape (`chunks`) should lead to chunks that fit in memory. See an example in [`visium()`](https://github.com/scverse/spatialdata-io/blob/main/src/spatialdata_io/readers/visium.py). +- Small images should be represented as single-scale images (`xarray.DataArray`), large images as multiscale images (`xarray.DataTree`). The scale factors and chunk shape (`chunks`) should lead to chunks that fit in memory. See an example in [`visium()`](https://github.com/scverse/spatialdata-io/blob/main/src/spatialdata_io/readers/visium/_reader.py). ##### Experimental readers @@ -481,14 +481,14 @@ pytest -m xenium pytest -m "xenium and data" pytest -m "xenium and not slow" pytest -m "xenium and cli" -python scripts/test_data_downloader --group xenium +uv run python scripts/test_data_downloader --group xenium SPATIALDATA_IO_TEST_DATA_DIR=/path/to/data pytest -m data ``` To download the same optional datasets used by CI, run: ```bash -python scripts/test_data_downloader +uv run python scripts/test_data_downloader ``` By default, the downloader skips datasets that already exist. Use `--force` to redownload selected datasets, `--dataset` for a single dataset key, and `--list` to show the available keys. @@ -499,6 +499,10 @@ registry. Prefer a stable repository DOI when one is available. For DOI entries, at runtime and verifies every downloaded file. Otherwise, register the archive URL and its SHA-256 hash as `known_hash = "sha256:..."`. +The [optional test-data downloader guide](https://github.com/scverse/spatialdata-io/blob/main/scripts/test_data_downloader/README.md) +extends this section with the downloader's installation model, all selection and output options, the ZIP/DOI/multi-asset +manifest forms, checksum generation, and the complete process for adding a dataset or adding files to an existing dataset. + #### Testing multiple versions When multiple versions of the raw data format are present, we encourage testing the reader on all of them to ensure backward compatibility. This task is greatly simplified if small test datasets are used for the CI tests. If this is not available, we suggest running the tests locally on multiple versions of the data before the PR is ready for review. diff --git a/scripts/test_data_downloader/README.md b/scripts/test_data_downloader/README.md new file mode 100644 index 00000000..52c11372 --- /dev/null +++ b/scripts/test_data_downloader/README.md @@ -0,0 +1,203 @@ +# Optional test-data downloader + +This directory contains the manifest and command-line tool used to download the +external datasets required by some `spatialdata-io` integration tests. These +datasets are intentionally not stored in Git because they are large or are +maintained by another project. + +The downloader provides one reproducible entry point for local development and +CI. It: + +- reads dataset registrations from [`datasets.toml`](datasets.toml); +- verifies every downloaded file against a published or committed checksum; +- assembles multi-file datasets in a temporary staging directory; +- leaves an existing dataset unchanged if a download or extraction fails; and +- makes registered datasets available to tests through the shared + `require_test_dataset` fixture. + +The manifest is project metadata, not a copy of the Pooch registry. It records +stable test keys, reader groups, local directory names, provenance, and one of +the supported download-source configurations. + +## Using the downloader + +Run commands from the repository root with the development dependencies +installed (`uv sync --group dev --group test`). + +List the registered keys, groups, output directories, and sources: + +```bash +uv run python scripts/test_data_downloader --list +``` + +Download one dataset or all datasets in a reader group: + +```bash +uv run python scripts/test_data_downloader --dataset visium_v1_human_lymph_node +uv run python scripts/test_data_downloader --group visium +``` + +`--dataset` and `--group` may each be repeated. When both are present, the +downloader installs the union of the selected keys and groups in manifest +order. With neither option, it downloads every registered dataset. + +Datasets are installed below `data/` by default. Use another parent directory +with `--output` and point pytest at the same location: + +```bash +uv run python scripts/test_data_downloader \ + --dataset visium_v1_human_lymph_node \ + --output /path/to/test-data +SPATIALDATA_IO_TEST_DATA_DIR=/path/to/test-data uv run pytest -m "visium and data" +``` + +An existing destination is skipped. Pass `--force` to download a fresh copy and +replace that destination after the new dataset has been completely staged: + +```bash +uv run python scripts/test_data_downloader --dataset visium_v1_human_lymph_node --force +``` + +The downloader attempts every selected dataset and exits with status 1 after +reporting all failures. Downloads require network access; normal unit tests and +manifest validation do not. + +## Manifest fields + +Every `[[datasets]]` entry defines these fields: + +| Field | Meaning | +| --- | --- | +| `key` | Stable lowercase identifier used by the CLI and pytest fixture. | +| `group` | Lowercase reader-oriented selector used by `--group`. | +| `extracted_dir` | Single directory name created below the output directory. | +| `source` | Human-readable upstream provenance, format version, and license. | +| `test_path` | Optional directory below `extracted_dir` returned to a test. | + +Keys and groups may contain lowercase letters, digits, underscores, and +hyphens. Paths must be relative and may not contain `..`. Keep keys and output +directory names stable: tests, local caches, and CI workflows can refer to them. + +Each dataset uses exactly one of the following source forms. + +### One ZIP archive + +Use a direct, stable URL and pin the complete ZIP by SHA-256. The archive's +contents are extracted into `extracted_dir` without removing a top-level +directory. + +```toml +[[datasets]] +key = "example" +group = "example_reader" +extracted_dir = "example_outs" +source = "Example project 2.0, CC BY 4.0" +url = "https://example.org/releases/2.0/example.zip" +known_hash = "sha256:<64 hexadecimal characters>" +``` + +### DOI repository + +Prefer a stable DOI when its repository publishes a Pooch-compatible file +registry. Pooch obtains the repository's file list and published hashes, then +downloads and verifies every registered file. + +```toml +[[datasets]] +key = "example" +group = "example_reader" +extracted_dir = "example_outs" +source = "Example project 2.0, CC BY 4.0" +doi = "10.5281/zenodo.1234567" +``` + +Store the bare DOI, without a URL or `doi:` prefix. A DOI entry cannot select a +subset of repository files; use independently hashed assets if the test needs +only part of a large publication. + +### Independently hashed assets + +Use `[[datasets.assets]]` when one logical dataset is assembled from multiple +downloads or when retaining a small set of upstream artifacts avoids a much +larger archive. Every asset has its own URL, checksum, and destination. + +```toml +[[datasets]] +key = "example" +group = "example_reader" +extracted_dir = "example_outs" +source = "Example project 2.0, CC BY 4.0" + +[[datasets.assets]] +url = "https://example.org/releases/2.0/counts.h5" +known_hash = "sha256:<64 hexadecimal characters>" +target = "filtered_feature_bc_matrix.h5" + +[[datasets.assets]] +url = "https://example.org/releases/2.0/spatial.tar.gz" +known_hash = "sha256:<64 hexadecimal characters>" +target = "." +extract = true +``` + +For a regular file, `target` includes the destination filename and parent +directories are created automatically. With `extract = true`, the asset must be +a tar archive and `target` is its extraction directory; `target = "."` extracts +at the dataset root. Tar members are filtered to reject path traversal and +unsafe filesystem objects. Asset targets must be unique within an entry. + +## Adding or updating a dataset + +1. Choose the smallest public, stable, and permissively licensed upstream data + that faithfully covers the format behavior under test. Record the data + producer, format/software version, and license in `source` and a nearby TOML + comment with the authoritative dataset or license page. +2. Decide whether a DOI, one ZIP archive, or independently hashed assets best + represents the source. Do not mix source forms in one dataset entry. +3. Download each direct URL once and calculate its SHA-256 digest. Pooch can + produce the exact manifest value: + + ```bash + uv run python -c 'import pooch; print("sha256:" + pooch.file_hash("/path/to/download", alg="sha256"))' + ``` + +4. Append the entry to `datasets.toml`, keeping related reader datasets + together. Never use an unknown hash, a mutable URL without a pinned digest, + credentials, or a private machine path. +5. Validate the registration and perform a clean download into a disposable + output directory: + + ```bash + uv run python scripts/test_data_downloader --list + test_output="$(mktemp -d)" + uv run python scripts/test_data_downloader --dataset example --output "$test_output" + ``` + +6. Inspect the resulting directory and connect the integration test to the + stable key: + + ```python + def test_example(require_test_dataset: Callable[[str], Path]) -> None: + dataset_path = require_test_dataset("example") + result = example_reader(dataset_path) + ``` + + The fixture skips cleanly when optional data is unavailable. Add the reader + marker and let the fixture apply the repository's `data` marker. +7. Run the downloader unit tests and the affected data-backed integration test: + + ```bash + uv run pytest tests/unit/test_download_test_data.py + SPATIALDATA_IO_TEST_DATA_DIR="$test_output" uv run pytest -m "example_reader and data" + ``` + +To add a file to an existing multi-asset dataset, append another +`[[datasets.assets]]` table with its own checksum and unused `target`, then +repeat the clean-download check. A ZIP or DOI entry cannot also contain assets; +either update that upstream source or deliberately convert the complete entry +to the multi-asset form. When changing an existing URL, file, archive contents, +or checksum, verify that the change is expected and rerun every integration test +that uses the dataset key. + +Do not commit downloaded datasets, temporary output, credentials, or private +data. The repository tracks only the manifest and downloader implementation. diff --git a/scripts/test_data_downloader/downloader.py b/scripts/test_data_downloader/downloader.py index 546c28c6..1a2ee450 100644 --- a/scripts/test_data_downloader/downloader.py +++ b/scripts/test_data_downloader/downloader.py @@ -4,6 +4,8 @@ import argparse import shutil +import sys +import tarfile import tempfile import zipfile from pathlib import Path @@ -11,7 +13,7 @@ import pooch import requests -from manifest import DATASETS, TestDataset, validate_datasets +from manifest import DATASETS, DatasetAsset, TestDataset, validate_datasets if TYPE_CHECKING: from collections.abc import Sequence @@ -21,7 +23,15 @@ class DatasetDownloadError(RuntimeError): - """Error raised when a test dataset cannot be downloaded or extracted.""" + """Report that a test dataset could not be downloaded or installed. + + Parameters + ---------- + dataset + Manifest entry whose download failed. + reason + Human-readable failure detail from the underlying operation. + """ def __init__(self, dataset: TestDataset, reason: str) -> None: self.dataset = dataset @@ -30,13 +40,16 @@ def __init__(self, dataset: TestDataset, reason: str) -> None: def download_dataset(dataset: TestDataset, output: Path, force: bool) -> None: - """Download and extract a single dataset. + """Download, verify, and install one dataset. + + All network and extraction work is staged in a temporary directory below + ``output``. An existing destination is changed only after the complete + staged dataset has been produced. Parameters ---------- dataset : TestDataset - Manifest entry describing the dataset archive and expected output - directory. + Validated manifest entry describing the source and destination. output : Path Parent directory where the extracted dataset directory should live. force : bool @@ -47,38 +60,54 @@ def download_dataset(dataset: TestDataset, output: Path, force: bool) -> None: DatasetDownloadError If the dataset files cannot be downloaded, extracted, or validated. """ - target = output / dataset.extracted_dir - if target.exists() and not force: - print(f"Skipping {dataset.key}: {target} already exists") - return - - output.mkdir(parents=True, exist_ok=True) - with tempfile.TemporaryDirectory(prefix=f"{dataset.key}-", dir=output) as tmpdir: - tmp_path = Path(tmpdir) - extracted_path = tmp_path / dataset.extracted_dir - extracted_path.mkdir() - - # Stage work in a temporary directory so interrupted downloads do not leave partial datasets. - try: + try: + validate_datasets((dataset,)) + target = output / dataset.extracted_dir + if (target.exists() or target.is_symlink()) and not force: + print(f"Skipping {dataset.key}: {target} already exists") + return + + output.mkdir(parents=True, exist_ok=True) + with tempfile.TemporaryDirectory(prefix=f"{dataset.key}-", dir=output) as tmpdir: + tmp_path = Path(tmpdir) + extracted_path = tmp_path / dataset.extracted_dir + extracted_path.mkdir() + + # Stage work in a temporary directory so interrupted downloads do not leave partial datasets. _fetch_dataset(dataset, tmp_path, extracted_path) - except (requests.exceptions.RequestException, ValueError, zipfile.BadZipFile, OSError) as exc: - raise DatasetDownloadError(dataset, str(exc)) from exc - if not extracted_path.is_dir() or not any(extracted_path.iterdir()): - raise DatasetDownloadError( - dataset, - f"download did not produce expected directory {dataset.extracted_dir!r}", - ) + if not any(extracted_path.iterdir()): + reason = f"download did not produce expected directory {dataset.extracted_dir!r}" + raise DatasetDownloadError(dataset, reason) - if target.exists(): - shutil.rmtree(target) - # Move the fully validated extraction into place only after all checks pass. - shutil.move(str(extracted_path), target) - print(f"Downloaded {dataset.key} to {target}") + if target.is_symlink() or target.is_file(): + target.unlink() + elif target.is_dir(): + shutil.rmtree(target) + # The temporary directory is below output, so this is a same-filesystem rename in normal operation. + shutil.move(extracted_path, target) + print(f"Downloaded {dataset.key} to {target}") + except DatasetDownloadError: + raise + except (requests.exceptions.RequestException, tarfile.TarError, ValueError, zipfile.BadZipFile, OSError) as exc: + raise DatasetDownloadError(dataset, str(exc)) from exc def main(argv: Sequence[str] | None = None) -> None: - """Run the dataset downloader command-line interface.""" + """Run the optional test-data downloader command-line interface. + + Parameters + ---------- + argv + Arguments without the executable name. When omitted, parse + ``sys.argv``. + + Raises + ------ + SystemExit + With status 1 after all selected datasets have been attempted if one + or more downloads fail. Argument parsing can also raise ``SystemExit``. + """ validate_datasets(DATASETS) args = _parse_args(argv) if args.list: @@ -92,17 +121,17 @@ def main(argv: Sequence[str] | None = None) -> None: download_dataset(dataset, args.output, args.force) except DatasetDownloadError as exc: failures.append(exc) - print(f"ERROR: {exc}") + print(f"ERROR: {exc}", file=sys.stderr) if failures: - print(f"Failed to download {len(failures)} dataset(s):") + print(f"Failed to download {len(failures)} dataset(s):", file=sys.stderr) for failure in failures: - print(f"- {failure}") + print(f"- {failure}", file=sys.stderr) raise SystemExit(1) def _parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: - """Parse command-line arguments for selecting and downloading datasets.""" + """Parse downloader selection, output, replacement, and listing options.""" dataset_keys = sorted(dataset.key for dataset in DATASETS) groups = sorted({dataset.group for dataset in DATASETS}) parser = argparse.ArgumentParser(description="Download optional test datasets used by spatialdata-io CI.") @@ -125,7 +154,7 @@ def _parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: def _selected_datasets(dataset_keys: list[str] | None, groups: list[str] | None) -> list[TestDataset]: - """Return manifest entries selected by explicit keys, groups, or all datasets.""" + """Select the union of explicit keys and groups in manifest order.""" selected_keys = set(dataset_keys or ()) selected_groups = set(groups or ()) if not selected_keys and not selected_groups: @@ -134,7 +163,7 @@ def _selected_datasets(dataset_keys: list[str] | None, groups: list[str] | None) def _fetch_dataset(dataset: TestDataset, temporary_path: Path, extracted_path: Path) -> None: - """Fetch and verify a dataset using its Pooch registry.""" + """Fetch and verify one validated DOI, multi-asset, or ZIP source.""" if dataset.doi: manager = pooch.create( path=extracted_path, @@ -150,6 +179,10 @@ def _fetch_dataset(dataset: TestDataset, temporary_path: Path, extracted_path: P manager.fetch(file_name) return + if dataset.assets: + _fetch_assets(dataset, temporary_path, extracted_path) + return + archive_name = f"{dataset.key}.zip" manager = pooch.create( path=temporary_path, @@ -159,3 +192,38 @@ def _fetch_dataset(dataset: TestDataset, temporary_path: Path, extracted_path: P retry_if_failed=DOWNLOAD_RETRIES, ) manager.fetch(archive_name, processor=pooch.Unzip(extract_dir=dataset.extracted_dir)) + + +def _fetch_assets(dataset: TestDataset, temporary_path: Path, extracted_path: Path) -> None: + """Fetch independently hashed files into one staged dataset directory.""" + registry = {f"asset-{index}": asset.known_hash for index, asset in enumerate(dataset.assets)} + urls = {f"asset-{index}": asset.url for index, asset in enumerate(dataset.assets)} + manager = pooch.create( + path=temporary_path, + base_url="", + registry=registry, + urls=urls, + retry_if_failed=DOWNLOAD_RETRIES, + ) + for index, asset in enumerate(dataset.assets): + fetched = manager.fetch(f"asset-{index}") + if not isinstance(fetched, str): + raise TypeError(f"Pooch returned an unexpected path collection for dataset asset {index}.") + _install_asset(Path(fetched), asset, extracted_path) + + +def _install_asset(downloaded: Path, asset: DatasetAsset, extracted_path: Path) -> None: + """Install one verified file or safely extract one tar archive. + + Tar extraction uses Python's restrictive data filter, which rejects + members that would escape the declared destination or create unsafe + filesystem objects. + """ + target = extracted_path / asset.target + if asset.extract: + target.mkdir(parents=True, exist_ok=True) + with tarfile.open(downloaded, mode="r:*") as archive: + archive.extractall(target, filter="data") + return + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(downloaded, target) diff --git a/scripts/test_data_downloader/manifest.py b/scripts/test_data_downloader/manifest.py index 8a176cf9..98aa166a 100644 --- a/scripts/test_data_downloader/manifest.py +++ b/scripts/test_data_downloader/manifest.py @@ -6,43 +6,77 @@ import tomllib from dataclasses import dataclass from functools import cache -from pathlib import Path, PurePath +from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any DATASETS_TOML = Path(__file__).with_name("datasets.toml") -COMMON_REQUIRED_FIELDS = frozenset({"key", "group", "extracted_dir", "source"}) -OPTIONAL_FIELDS = frozenset({"test_path", "url", "known_hash", "doi"}) -ALLOWED_FIELDS = COMMON_REQUIRED_FIELDS | OPTIONAL_FIELDS +COMMON_REQUIRED_FIELDS = ("key", "group", "extracted_dir", "source") +STRING_SOURCE_FIELDS = ("test_path", "url", "known_hash", "doi") +ALLOWED_FIELDS = frozenset(COMMON_REQUIRED_FIELDS) | frozenset(STRING_SOURCE_FIELDS) | {"assets"} +ASSET_REQUIRED_FIELDS = frozenset({"url", "known_hash", "target"}) +ASSET_OPTIONAL_FIELDS = frozenset({"extract"}) +ASSET_ALLOWED_FIELDS = ASSET_REQUIRED_FIELDS | ASSET_OPTIONAL_FIELDS KNOWN_HASH_PATTERN = re.compile(r"sha256:[0-9a-fA-F]{64}") +IDENTIFIER_PATTERN = re.compile(r"[a-z0-9][a-z0-9_-]*") -@dataclass(frozen=True) +@dataclass(frozen=True, slots=True) +class DatasetAsset: + """Describe one independently verified file in a multi-asset dataset. + + Parameters + ---------- + url + Direct URL from which Pooch downloads the file. + known_hash + Expected digest in ``sha256:`` form. + target + Relative destination below the dataset directory. For a regular file, + this includes the destination filename. For an extracted tar archive, + this names the extraction directory and may be ``"."``. + extract + Whether to treat the downloaded file as a tar archive and safely + extract it below ``target``. + """ + + url: str + known_hash: str + target: str + extract: bool = False + + +@dataclass(frozen=True, slots=True) class TestDataset: - """Test dataset to be downloaded. + """Describe an optional integration-test dataset. + + Exactly one download source must be configured: ``doi``; the ``url`` and + ``known_hash`` archive pair; or one or more ``assets``. Parameters ---------- - key : str - Unique identifier for the dataset, used for referencing in tests and - the CLI. - group : str - Logical grouping of the dataset, for example ``"xenium"``, - ``"visium_hd"``, ``"seqfish"``, or ``"macsima"``. - extracted_dir : str - Name of the final directory containing the downloaded dataset. - source : str - Human-readable description of the dataset source and license. - test_path : str - Optional path inside the extracted directory that should be passed to - integration tests. - url : str - Direct URL to a ZIP archive. Used together with ``known_hash`` when a - DOI is unavailable. - known_hash : str - Pooch-compatible SHA-256 hash of the dataset archive. - doi : str - DOI for a repository whose files and published hashes should be loaded - through Pooch instead of downloading a single archive. + key + Stable command-line and pytest fixture identifier. Keys use lowercase + letters, digits, underscores, and hyphens. + group + Reader-oriented selection group, such as ``"visium"`` or ``"xenium"``. + extracted_dir + Name of the dataset directory created directly below the selected + downloader output directory. + source + Human-readable provenance and licensing description shown by + ``--list``. + test_path + Optional relative directory below ``extracted_dir`` returned by the + shared ``require_test_dataset`` pytest fixture. + url + Direct URL of a ZIP archive for a single-archive source. + known_hash + Expected ZIP archive digest in ``sha256:`` form. + doi + Stable repository DOI without a ``doi:`` prefix. Pooch downloads every + file in the repository's published registry. + assets + Independently downloaded and verified files assembled into one dataset. """ key: str @@ -53,79 +87,78 @@ class TestDataset: url: str = "" known_hash: str = "" doi: str = "" + assets: tuple[DatasetAsset, ...] = () def load_datasets(path: str | Path = DATASETS_TOML) -> tuple[TestDataset, ...]: - """Load dataset entries from a TOML manifest.""" + """Load and validate dataset entries from a TOML manifest. + + Parameters + ---------- + path + TOML manifest to load. + + Returns + ------- + tuple of TestDataset + Validated datasets in manifest order. + + Raises + ------ + OSError + If the manifest cannot be read. + ValueError + If the TOML is malformed or violates the manifest contract. + """ manifest_path = Path(path) try: raw_manifest = tomllib.loads(manifest_path.read_text(encoding="utf-8")) except tomllib.TOMLDecodeError as exc: raise ValueError(f"Invalid dataset manifest TOML in {manifest_path}: {exc}") from exc - unknown_root_fields = set(raw_manifest) - {"datasets"} if unknown_root_fields: unknown = ", ".join(sorted(unknown_root_fields)) raise ValueError(f"Dataset manifest has unknown root field(s): {unknown}.") - raw_datasets = raw_manifest.get("datasets") if not isinstance(raw_datasets, list): raise ValueError("Dataset manifest must define a [[datasets]] array.") - datasets = tuple(_parse_dataset(raw_dataset, index) for index, raw_dataset in enumerate(raw_datasets)) validate_datasets(datasets) return datasets def validate_datasets(datasets: tuple[TestDataset, ...] | None = None) -> None: - """Validate that dataset entries are internally consistent. + """Validate dataset identity, destination, and download-source contracts. Parameters ---------- - datasets : tuple[TestDataset, ...] - Dataset entries to validate. + datasets + Dataset entries to validate. The module-level manifest is validated + when omitted. Raises ------ ValueError - If a required field is empty, a hash or path is invalid, a key or - extracted directory is duplicated, or the download source is - incomplete or ambiguous. + If an entry is incomplete, ambiguous, unsafe, or duplicates another + dataset key or destination. """ if datasets is None: datasets = DATASETS - seen_keys: set[str] = set() seen_extracted_dirs: set[str] = set() - for dataset in datasets: - for field_name in COMMON_REQUIRED_FIELDS: - value = getattr(dataset, field_name) - if not isinstance(value, str): - raise ValueError(f"Dataset {dataset.key!r} field {field_name!r} must be a string.") - if not value.strip(): - raise ValueError(f"Dataset {dataset.key!r} has empty {field_name}.") - - for field_name in OPTIONAL_FIELDS: - if not isinstance(getattr(dataset, field_name), str): - raise ValueError(f"Dataset {dataset.key!r} field {field_name!r} must be a string.") - - has_url = bool(dataset.url.strip()) - has_hash = bool(dataset.known_hash.strip()) - has_doi = bool(dataset.doi.strip()) - if has_doi and (has_url or has_hash): - raise ValueError(f"Dataset {dataset.key!r} cannot define both an archive and a DOI.") - if not has_doi and not (has_url and has_hash): - raise ValueError(f"Dataset {dataset.key!r} must define either doi or both url and known_hash.") - if has_url: - _validate_known_hash(dataset.key, dataset.known_hash) - if has_doi and (not dataset.doi.startswith("10.") or any(character.isspace() for character in dataset.doi)): - raise ValueError(f"Dataset {dataset.key!r} doi must be a DOI without a 'doi:' prefix.") - - test_path = PurePath(dataset.test_path) - # Test paths are appended to extracted_dir by tests, so they must stay inside that directory. - if dataset.test_path and (test_path.is_absolute() or ".." in test_path.parts): - raise ValueError(f"Dataset {dataset.key!r} test_path must be a relative path inside extracted_dir.") + _validate_dataset_strings(dataset) + _validate_identifier(dataset.key, dataset_key=dataset.key, field="key") + _validate_identifier(dataset.group, dataset_key=dataset.key, field="group") + _validate_relative_path( + dataset.key, + "extracted_dir", + dataset.extracted_dir, + allow_current=False, + require_single_component=True, + ) + _validate_dataset_source(dataset) + _validate_relative_path(dataset.key, "test_path", dataset.test_path, allow_current=True) if dataset.key in seen_keys: raise ValueError(f"Duplicate test dataset key: {dataset.key!r}.") if dataset.extracted_dir in seen_extracted_dirs: @@ -135,12 +168,22 @@ def validate_datasets(datasets: tuple[TestDataset, ...] | None = None) -> None: def get_dataset(key: str) -> TestDataset: - """Return the dataset registered for ``key``. + """Return the registered dataset identified by ``key``. + + Parameters + ---------- + key + Dataset key from the manifest. + + Returns + ------- + TestDataset + Matching manifest entry. Raises ------ KeyError - If ``key`` is not a registered dataset key. + If no registered dataset has ``key``. """ try: return _datasets_by_key()[key] @@ -150,43 +193,152 @@ def get_dataset(key: str) -> TestDataset: def datasets_by_group(group: str) -> tuple[TestDataset, ...]: - """Return datasets registered for ``group`` in manifest order.""" + """Return datasets in a selection group. + + Parameters + ---------- + group + Exact manifest group to select. + + Returns + ------- + tuple of TestDataset + Matching datasets in manifest order, or an empty tuple when the group + is unknown. + """ return tuple(dataset for dataset in DATASETS if dataset.group == group) +def _validate_dataset_strings(dataset: TestDataset) -> None: + for field_name in COMMON_REQUIRED_FIELDS: + value = getattr(dataset, field_name) + if not isinstance(value, str): + raise ValueError(f"Dataset {dataset.key!r} field {field_name!r} must be a string.") + if not value.strip(): + raise ValueError(f"Dataset {dataset.key!r} has empty {field_name}.") + for field_name in STRING_SOURCE_FIELDS: + if not isinstance(getattr(dataset, field_name), str): + raise ValueError(f"Dataset {dataset.key!r} field {field_name!r} must be a string.") + + +def _validate_dataset_source(dataset: TestDataset) -> None: + if not isinstance(dataset.assets, tuple): + raise ValueError(f"Dataset {dataset.key!r} assets must be a tuple of DatasetAsset values.") + has_archive = bool(dataset.url.strip() or dataset.known_hash.strip()) + has_doi = bool(dataset.doi.strip()) + has_assets = bool(dataset.assets) + if sum((has_archive, has_doi, has_assets)) != 1: + raise ValueError( + f"Dataset {dataset.key!r} must define exactly one source: an archive, a DOI, or an assets array." + ) + if has_archive: + if not dataset.url.strip() or not dataset.known_hash.strip(): + raise ValueError(f"Dataset {dataset.key!r} archive requires both url and known_hash.") + _validate_known_hash(dataset.key, dataset.known_hash) + if has_doi and (not dataset.doi.startswith("10.") or any(character.isspace() for character in dataset.doi)): + raise ValueError(f"Dataset {dataset.key!r} doi must be a DOI without a 'doi:' prefix.") + if has_assets: + _validate_assets(dataset) + + +def _validate_assets(dataset: TestDataset) -> None: + seen_targets: set[str] = set() + for index, asset in enumerate(dataset.assets): + if not isinstance(asset, DatasetAsset): + raise ValueError(f"Dataset {dataset.key!r} asset {index} must be a DatasetAsset.") + if not isinstance(asset.url, str): + raise ValueError(f"Dataset {dataset.key!r} asset {index} url must be a string.") + if not isinstance(asset.known_hash, str): + raise ValueError(f"Dataset {dataset.key!r} asset {index} known_hash must be a string.") + if not isinstance(asset.target, str): + raise ValueError(f"Dataset {dataset.key!r} asset {index} target must be a string.") + if not isinstance(asset.extract, bool): + raise ValueError(f"Dataset {dataset.key!r} asset {index} extract must be a boolean.") + if not asset.url.strip(): + raise ValueError(f"Dataset {dataset.key!r} asset {index} has empty url.") + _validate_known_hash(f"{dataset.key} asset {index}", asset.known_hash) + _validate_relative_path(dataset.key, f"asset {index} target", asset.target, allow_current=asset.extract) + if asset.target in seen_targets: + raise ValueError(f"Dataset {dataset.key!r} has duplicate asset target {asset.target!r}.") + seen_targets.add(asset.target) + + +def _validate_identifier(value: str, *, dataset_key: str, field: str) -> None: + if not IDENTIFIER_PATTERN.fullmatch(value): + raise ValueError( + f"Dataset {dataset_key!r} {field} must contain only lowercase letters, digits, underscores, and hyphens." + ) + + +def _validate_relative_path( + dataset_key: str, + field: str, + value: str, + *, + allow_current: bool, + require_single_component: bool = False, +) -> None: + posix_path = PurePosixPath(value) + windows_path = PureWindowsPath(value) + paths = (posix_path, windows_path) + if value and (any(path.is_absolute() for path in paths) or any(".." in path.parts for path in paths)): + raise ValueError(f"Dataset {dataset_key!r} {field} must be a relative path inside extracted_dir.") + if value in {"", "."} and not allow_current: + raise ValueError(f"Dataset {dataset_key!r} {field} must name a destination inside extracted_dir.") + if require_single_component and any(len(path.parts) != 1 for path in paths): + raise ValueError(f"Dataset {dataset_key!r} {field} must be a single directory name.") + + def _parse_dataset(raw_dataset: object, index: int) -> TestDataset: if not isinstance(raw_dataset, dict): raise ValueError(f"Dataset manifest entry at index {index} must be a table.") - dataset_fields = set(raw_dataset) unknown_fields = dataset_fields - ALLOWED_FIELDS if unknown_fields: unknown = ", ".join(sorted(unknown_fields)) raise ValueError(f"Dataset manifest entry at index {index} has unknown field(s): {unknown}.") - - missing_fields = COMMON_REQUIRED_FIELDS - dataset_fields + missing_fields = set(COMMON_REQUIRED_FIELDS) - dataset_fields if missing_fields: missing = ", ".join(sorted(missing_fields)) raise ValueError(f"Dataset manifest entry at index {index} is missing required field(s): {missing}.") - values: dict[str, Any] = dict(raw_dataset) - for field_name in OPTIONAL_FIELDS: + raw_assets = values.pop("assets", []) + for field_name in STRING_SOURCE_FIELDS: values.setdefault(field_name, "") + values["assets"] = _parse_assets(raw_assets, dataset_index=index) dataset = TestDataset(**values) validate_datasets((dataset,)) return dataset +def _parse_assets(raw_assets: object, *, dataset_index: int) -> tuple[DatasetAsset, ...]: + if not isinstance(raw_assets, list): + raise ValueError(f"Dataset manifest entry at index {dataset_index} assets must be an array of tables.") + parsed: list[DatasetAsset] = [] + for asset_index, raw_asset in enumerate(raw_assets): + if not isinstance(raw_asset, dict): + raise ValueError(f"Dataset {dataset_index} asset {asset_index} must be a table.") + fields = set(raw_asset) + unknown = fields - ASSET_ALLOWED_FIELDS + missing = ASSET_REQUIRED_FIELDS - fields + if unknown: + raise ValueError(f"Dataset {dataset_index} asset {asset_index} has unknown field(s): {sorted(unknown)}.") + if missing: + raise ValueError(f"Dataset {dataset_index} asset {asset_index} is missing field(s): {sorted(missing)}.") + values = dict(raw_asset) + values.setdefault("extract", False) + parsed.append(DatasetAsset(**values)) + return tuple(parsed) + + def _validate_known_hash(dataset_key: str, known_hash: str) -> None: if not KNOWN_HASH_PATTERN.fullmatch(known_hash): raise ValueError(f"Dataset {dataset_key!r} known_hash must be formatted as 'sha256:<64 hex characters>'.") -# Keep keys and groups stable because CI workflows and pytest markers may refer to them. DATASETS = load_datasets() @cache def _datasets_by_key() -> dict[str, TestDataset]: - """Return cached lookup table for dataset keys.""" return {dataset.key: dataset for dataset in DATASETS} diff --git a/tests/unit/test_download_test_data.py b/tests/unit/test_download_test_data.py index 90daa586..6233a2e9 100644 --- a/tests/unit/test_download_test_data.py +++ b/tests/unit/test_download_test_data.py @@ -2,6 +2,7 @@ import importlib.util import sys +import tarfile from pathlib import Path from typing import TYPE_CHECKING, Any @@ -44,7 +45,7 @@ def _make_dataset( return download_test_data.TestDataset( key=key, group=group, - extracted_dir=extracted_dir or key, + extracted_dir=key if extracted_dir is None else extracted_dir, source=source, test_path=test_path, url="" if doi else url or f"https://example.com/{key}.zip", @@ -145,6 +146,51 @@ def load_registry_from_doi(self) -> None: with pytest.raises(ValueError, match="contains no files"): download_test_data._fetch_dataset(dataset, tmp_path, tmp_path / dataset.extracted_dir) + def test_fetches_and_installs_independently_hashed_assets( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + first = tmp_path / "downloaded-counts.h5" + first.write_bytes(b"counts") + second = tmp_path / "downloaded-spatial.tar.gz" + payload = tmp_path / "tissue_positions.csv" + payload.write_text("positions", encoding="utf-8") + with tarfile.open(second, "w:gz") as archive: + archive.add(payload, arcname="spatial/tissue_positions.csv") + assets = ( + manifest.DatasetAsset("https://example.com/counts.h5", SHA256, "filtered_feature_bc_matrix.h5"), + manifest.DatasetAsset("https://example.com/spatial.tar.gz", SHA256, ".", extract=True), + ) + dataset = manifest.TestDataset( + key="assets", + group="group", + extracted_dir="assets", + source="source", + assets=assets, + ) + extracted = tmp_path / "staged" + extracted.mkdir() + seen_create: dict[str, Any] = {} + + class Manager: + def fetch(self, file_name: str) -> str: + return str(first if file_name == "asset-0" else second) + + def create(**kwargs: Any) -> Manager: + seen_create.update(kwargs) + return Manager() + + monkeypatch.setattr(download_test_data.pooch, "create", create) + + download_test_data._fetch_dataset(dataset, tmp_path, extracted) + + assert seen_create["registry"] == {"asset-0": SHA256, "asset-1": SHA256} + assert seen_create["urls"] == { + "asset-0": "https://example.com/counts.h5", + "asset-1": "https://example.com/spatial.tar.gz", + } + assert (extracted / "filtered_feature_bc_matrix.h5").read_bytes() == b"counts" + assert (extracted / "spatial" / "tissue_positions.csv").read_text(encoding="utf-8") == "positions" + class TestDatasetManifest: def test_manifest_is_valid(self) -> None: @@ -216,6 +262,39 @@ def test_loads_doi_dataset_from_toml(self, tmp_path: Path) -> None: assert dataset.known_hash == "" assert dataset.doi == "10.5281/zenodo.123" + def test_loads_multi_asset_dataset_from_toml(self, tmp_path: Path) -> None: + manifest_path = tmp_path / "datasets.toml" + manifest_path.write_text( + f""" +[[datasets]] +key = "example" +group = "group" +extracted_dir = "example" +source = "example source" + +[[datasets.assets]] +url = "https://example.com/counts.h5" +known_hash = "{SHA256}" +target = "filtered_feature_bc_matrix.h5" + +[[datasets.assets]] +url = "https://example.com/spatial.tar.gz" +known_hash = "{SHA256}" +target = "." +extract = true +""", + encoding="utf-8", + ) + + (dataset,) = manifest.load_datasets(manifest_path) + + assert dataset.url == "" + assert dataset.doi == "" + assert dataset.assets == ( + manifest.DatasetAsset("https://example.com/counts.h5", SHA256, "filtered_feature_bc_matrix.h5"), + manifest.DatasetAsset("https://example.com/spatial.tar.gz", SHA256, ".", extract=True), + ) + def test_rejects_duplicate_dataset_keys(self) -> None: first = _make_dataset("duplicate", extracted_dir="first") second = _make_dataset("duplicate", extracted_dir="second") @@ -242,6 +321,24 @@ def test_rejects_test_path_outside_extracted_dir(self) -> None: with pytest.raises(ValueError, match="test_path must be a relative path"): manifest.validate_datasets((dataset,)) + @pytest.mark.parametrize( + "extracted_dir", + ["", ".", "/absolute", "../outside", "nested/directory", r"C:\outside"], + ) + def test_rejects_unsafe_extracted_directory(self, extracted_dir: str) -> None: + dataset = _make_dataset("unsafe-directory", extracted_dir=extracted_dir) + + with pytest.raises(ValueError, match="extracted_dir"): + manifest.validate_datasets((dataset,)) + + @pytest.mark.parametrize(("field", "value"), [("key", "Uppercase"), ("group", "has spaces")]) + def test_rejects_invalid_identifiers(self, field: str, value: str) -> None: + values = {"key": "example", "group": "group", field: value} + dataset = _make_dataset(**values) + + with pytest.raises(ValueError, match=field): + manifest.validate_datasets((dataset,)) + def test_rejects_invalid_known_hash(self) -> None: dataset = _make_dataset("invalid-hash") dataset = manifest.TestDataset( @@ -276,7 +373,45 @@ def test_rejects_incomplete_or_ambiguous_download_source(self, url: str, known_h doi=doi, ) - with pytest.raises(ValueError, match="either doi|both an archive"): + with pytest.raises(ValueError, match="exactly one source|requires both url and known_hash"): + manifest.validate_datasets((dataset,)) + + @pytest.mark.parametrize("target", ["", "/absolute", "../outside", r"C:\outside", r"nested\..\outside"]) + def test_rejects_unsafe_asset_target(self, target: str) -> None: + dataset = manifest.TestDataset( + key="example", + group="group", + extracted_dir="example", + source="source", + assets=(manifest.DatasetAsset("https://example.com/file", SHA256, target),), + ) + + with pytest.raises(ValueError, match="target"): + manifest.validate_datasets((dataset,)) + + def test_rejects_duplicate_asset_targets(self) -> None: + asset = manifest.DatasetAsset("https://example.com/file", SHA256, "same") + dataset = manifest.TestDataset( + key="example", + group="group", + extracted_dir="example", + source="source", + assets=(asset, asset), + ) + + with pytest.raises(ValueError, match="duplicate asset target"): + manifest.validate_datasets((dataset,)) + + def test_rejects_non_boolean_asset_extract(self) -> None: + dataset = manifest.TestDataset( + key="example", + group="group", + extracted_dir="example", + source="source", + assets=(manifest.DatasetAsset("https://example.com/file", SHA256, ".", extract=1),), # type: ignore[arg-type] + ) + + with pytest.raises(ValueError, match="extract must be a boolean"): manifest.validate_datasets((dataset,)) def test_rejects_manifest_without_datasets_array(self, tmp_path: Path) -> None: @@ -337,6 +472,38 @@ def test_rejects_unknown_manifest_field(self, tmp_path: Path) -> None: manifest.load_datasets(manifest_path) +class TestInstallAsset: + def test_extracts_tar_below_declared_target(self, tmp_path: Path) -> None: + payload = tmp_path / "payload.txt" + payload.write_text("contents", encoding="utf-8") + archive_path = tmp_path / "payload.tar.gz" + with tarfile.open(archive_path, "w:gz") as archive: + archive.add(payload, arcname="payload.txt") + extracted = tmp_path / "extracted" + extracted.mkdir() + asset = manifest.DatasetAsset("https://example.com/payload.tar.gz", SHA256, "nested", extract=True) + + download_test_data._install_asset(archive_path, asset, extracted) + + assert not (extracted / "payload.txt").exists() + assert (extracted / "nested" / "payload.txt").read_text(encoding="utf-8") == "contents" + + def test_rejects_tar_member_outside_staging_directory(self, tmp_path: Path) -> None: + source = tmp_path / "payload.txt" + source.write_text("unsafe", encoding="utf-8") + archive_path = tmp_path / "unsafe.tar.gz" + with tarfile.open(archive_path, "w:gz") as archive: + archive.add(source, arcname="../outside.txt") + extracted = tmp_path / "extracted" + extracted.mkdir() + asset = manifest.DatasetAsset("https://example.com/unsafe.tar.gz", SHA256, ".", extract=True) + + with pytest.raises(tarfile.OutsideDestinationError): + download_test_data._install_asset(archive_path, asset, extracted) + + assert not (tmp_path / "outside.txt").exists() + + class TestDownloadDataset: def test_skips_existing_dataset_without_force( self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] @@ -380,6 +547,40 @@ def fetch(dataset: object, temporary_path: Path, extracted_path: Path) -> None: with pytest.raises(download_test_data.DatasetDownloadError, match="error: 403 Client Error"): download_test_data.download_dataset(dataset, tmp_path, force=False) + def test_failed_forced_download_preserves_existing_dataset( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + dataset = _make_dataset("existing") + target = tmp_path / dataset.extracted_dir + target.mkdir() + existing = target / "existing.txt" + existing.write_text("keep", encoding="utf-8") + + def fetch(*_args: object) -> None: + raise requests.HTTPError("download failed") + + monkeypatch.setattr(download_test_data, "_fetch_dataset", fetch) + + with pytest.raises(download_test_data.DatasetDownloadError, match="download failed"): + download_test_data.download_dataset(dataset, tmp_path, force=True) + + assert existing.read_text(encoding="utf-8") == "keep" + + def test_force_replaces_non_directory_destination(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + dataset = _make_dataset("file-target") + target = tmp_path / dataset.extracted_dir + target.write_text("old", encoding="utf-8") + + def fetch(_dataset: object, _temporary_path: Path, extracted_path: Path) -> None: + (extracted_path / "payload.txt").write_text("new", encoding="utf-8") + + monkeypatch.setattr(download_test_data, "_fetch_dataset", fetch) + + download_test_data.download_dataset(dataset, tmp_path, force=True) + + assert target.is_dir() + assert (target / "payload.txt").read_text(encoding="utf-8") == "new" + def test_reports_download_without_expected_contents(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: dataset = _make_dataset("empty") monkeypatch.setattr(download_test_data, "_fetch_dataset", lambda *args: None) @@ -439,6 +640,7 @@ def download_dataset(dataset: Any, output: Path, force: bool) -> None: assert exc_info.value.code == 1 assert attempted == ["first", "second"] - output = capsys.readouterr().out - assert "Failed to download 1 dataset(s)" in output - assert "first" in output + captured = capsys.readouterr() + assert captured.out == "" + assert "Failed to download 1 dataset(s)" in captured.err + assert "first" in captured.err