diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 452a1bda4..8f03fae7e 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -60,7 +60,9 @@ jobs: python: 3.14 mode: obolibrary-only - os: ubuntu-latest - python: '3.11' + # cannot use dev-deps on 3.11 now, see + # https://github.com/hdmf-dev/hdmf-zarr/issues/406 + python: '3.12' mode: dev-deps - os: ubuntu-latest python: 3.14 diff --git a/dandi/dandiarchive.py b/dandi/dandiarchive.py index 262d162d8..686c2040c 100644 --- a/dandi/dandiarchive.py +++ b/dandi/dandiarchive.py @@ -54,9 +54,13 @@ from .dandiapi import BaseRemoteAsset, DandiAPIClient, RemoteDandiset from .exceptions import FailedToConnectError, NotFoundError, UnknownURLError from .utils import get_instance, get_retry_after +from .zarr_filter import ZarrFilter lgr = get_logger() +#: `ZARR_EXTENSIONS` in the form `str.endswith()` takes +_ZARR_SUFFIXES = tuple(ZARR_EXTENSIONS) + @dataclass class ParsedDandiURL(ABC): @@ -201,6 +205,20 @@ def get_asset_download_path( """ ... + def get_zarr_filter(self) -> list[ZarrFilter]: + """ + Returns the filters restricting which entries within the Zarr assets + returned by `get_assets()` should be downloaded. An empty list means + that no restriction is implied by the URL and all entries are to be + downloaded. + + Only `AssetZarrEntryURL` — a URL pointing inside a Zarr asset — returns + a non-empty list. + + :meta private: + """ + return [] + @abstractmethod def is_under_download_path(self, path: str) -> bool: """ @@ -474,13 +492,38 @@ def split_zarr_location(location: str) -> tuple[str, str] | None: """ parts = [p for p in location.split("/") if p] for i, part in enumerate(parts): - if any(part.endswith(ext) for ext in ZARR_EXTENSIONS): + if part.endswith(_ZARR_SUFFIXES): asset_path = "/".join(parts[: i + 1]) zarr_subpath = "/".join(parts[i + 1 :]) return (asset_path, zarr_subpath) if zarr_subpath else None return None +def at_zarr_boundary(location: str) -> bool: + """Whether ``location``'s last component ends with a Zarr extension. + + Parameters + ---------- + location : str + A POSIX-style path, e.g. ``"sub-1/file.ome.zarr/"``. + + Returns + ------- + bool + True if the path ends at a Zarr asset (ignoring a trailing slash). + + Examples + -------- + >>> at_zarr_boundary("sub-1/file.ome.zarr/") + True + >>> at_zarr_boundary("sub-1/file.ome.zarr/0/0") # below the boundary + False + >>> at_zarr_boundary("sub-1/") + False + """ + return location.rstrip("/").endswith(_ZARR_SUFFIXES) + + @dataclass class AssetZarrEntryURL(SingleAssetURL): """Parsed from a URL that points into entries within a Zarr asset. @@ -489,8 +532,10 @@ class AssetZarrEntryURL(SingleAssetURL): produce ``asset_path="sub-1/file.ome.zarr"`` and ``zarr_subpath="0/0/0"``. """ - asset_path: str # e.g., "sub-1/file.ome.zarr" - zarr_subpath: str # e.g., "0/0/0" + #: The path of the Zarr asset, e.g. ``"sub-1/file.ome.zarr"`` + asset_path: str + #: The path within the Zarr asset, e.g. ``"0/0/0"`` + zarr_subpath: str def get_assets( self, client: DandiAPIClient, order: str | None = None, strict: bool = False @@ -512,6 +557,17 @@ def get_assets( with _maybe_strict(strict): yield dandiset.get_asset_by_path(self.asset_path) + def get_zarr_filter(self) -> list[ZarrFilter]: + """Restrict the download to the entries at or under `zarr_subpath`. + + :meta private: + """ + if not self.zarr_subpath: + # `parse_dandi_url()` never produces this, but the class is public + # and an empty subpath would otherwise reject every entry. + return [] + return [ZarrFilter("path", self.zarr_subpath)] + @dataclass class AssetFolderURL(MultiAssetURL): @@ -905,6 +961,28 @@ def parse( version_id=version_id, path=location, ) + elif (zarr_split := split_zarr_location(location)) is not None: + # The location crosses a zarr boundary. This is checked + # before the folder case, as a path within a zarr never names + # a folder of assets: entries within a zarr are not assets. + asset_path, zarr_subpath = zarr_split + parsed_url = AssetZarrEntryURL( + instance=instance, + dandiset_id=dandiset_id, + version_id=version_id, + asset_path=asset_path, + zarr_subpath=zarr_subpath, + ) + elif location.endswith("/") and at_zarr_boundary(location): + # `.../x.zarr/` names the zarr asset itself; a folder of + # assets by that name could never hold it, as the asset's own + # path does not end in a slash. + parsed_url = AssetItemURL( + instance=instance, + dandiset_id=dandiset_id, + version_id=version_id, + path=location.rstrip("/"), + ) elif location.endswith("/"): parsed_url = AssetFolderURL( instance=instance, @@ -913,24 +991,12 @@ def parse( path=location, ) else: - # Check if location crosses a zarr boundary - zarr_split = split_zarr_location(location) - if zarr_split is not None: - asset_path, zarr_subpath = zarr_split - parsed_url = AssetZarrEntryURL( - instance=instance, - dandiset_id=dandiset_id, - version_id=version_id, - asset_path=asset_path, - zarr_subpath=zarr_subpath, - ) - else: - parsed_url = AssetItemURL( - instance=instance, - dandiset_id=dandiset_id, - version_id=version_id, - path=location, - ) + parsed_url = AssetItemURL( + instance=instance, + dandiset_id=dandiset_id, + version_id=version_id, + path=location, + ) elif asset_id: if dandiset_id is None: parsed_url = BaseAssetIDURL(instance=instance, asset_id=asset_id) diff --git a/dandi/delete.py b/dandi/delete.py index fefd68221..2bcc171cf 100644 --- a/dandi/delete.py +++ b/dandi/delete.py @@ -142,6 +142,13 @@ def register_url(self, url: str) -> None: assert parsed_url.dandiset_id is not None self.register_dandiset(parsed_url.instance, parsed_url.dandiset_id) else: + if parsed_url.get_zarr_filter(): + # The URL points inside a Zarr asset, but `get_assets()` yields + # the whole asset, so deleting it would take the entire Zarr. + raise NotImplementedError( + "Cannot delete individual entries within a Zarr asset;" + f" {url} points inside one" + ) if parsed_url.version_id is None: parsed_url.version_id = DRAFT self.register_assets_url(url, parsed_url) diff --git a/dandi/download.py b/dandi/download.py index a87d5380e..366e26c39 100644 --- a/dandi/download.py +++ b/dandi/download.py @@ -48,7 +48,6 @@ from .dandiapi import AssetType, BaseRemoteZarrAsset, RemoteDandiset from .dandiarchive import ( AssetItemURL, - AssetZarrEntryURL, DandisetURL, ParsedDandiURL, SingleAssetURL, @@ -122,23 +121,20 @@ def download( parsed_urls = [parse_dandi_url(u, glob=path_type is PathType.GLOB) for u in urls] - # Parse zarr entry filters - zarr_entry_filter: Callable[[str], bool] | None = None - all_zf: list[ZarrFilter] = [] - if zarr_filters: - for spec in zarr_filters: - all_zf.extend(parse_zarr_filter(spec)) + # Parse the explicit ``--zarr`` filters. Filters implied by a URL that + # points inside a Zarr asset are resolved per-URL by each `Downloader`, so + # that the subpath of one URL does not restrict the download of another. + explicit_zarr_filters: list[ZarrFilter] = [] + for spec in zarr_filters: + explicit_zarr_filters.extend(parse_zarr_filter(spec)) - # Merge URL-derived path filters from AssetZarrEntryURL - for purl in parsed_urls: - if isinstance(purl, AssetZarrEntryURL): - all_zf.append(ZarrFilter("path", purl.zarr_subpath)) - - if all_zf: - zarr_entry_filter = make_zarr_entry_filter(all_zf) - - if sync and zarr_entry_filter is not None: - raise ValueError("--sync and --zarr cannot be used together") + if sync: + if explicit_zarr_filters: + raise ValueError("--sync and --zarr cannot be used together") + if any(purl.get_zarr_filter() for purl in parsed_urls): + raise ValueError( + "--sync cannot be used with a URL pointing inside a Zarr asset" + ) # dandi.cli.formatters are used in cmd_ls to provide switchable pyout_style = pyouts.get_style(hide_if_missing=False) @@ -176,7 +172,7 @@ def download( preserve_tree=preserve_tree, jobs_per_zarr=jobs_per_zarr, on_error="yield" if format is DownloadFormat.PYOUT else "raise", - zarr_entry_filter=zarr_entry_filter, + zarr_filters=explicit_zarr_filters, **kw, ) for purl in parsed_urls @@ -272,7 +268,17 @@ class Downloader: preserve_tree: bool jobs_per_zarr: int | None on_error: Literal["raise", "yield"] - zarr_entry_filter: Callable[[str], bool] | None = None + #: Filters from the ``--zarr`` option; they apply to every Zarr asset, and + #: matching no entries in a given asset is not an error + zarr_filters: list[ZarrFilter] = field(default_factory=list) + #: Filters implied by `url` pointing inside a Zarr asset. Unlike + #: `zarr_filters`, these name entries the user explicitly asked for, so + #: matching none of them is an error + url_zarr_filters: list[ZarrFilter] = field(init=False, default_factory=list) + #: Predicate for selecting entries within a Zarr asset, combining + #: `zarr_filters` with `url_zarr_filters`; `None` means that every entry is + #: to be downloaded + zarr_entry_filter: Callable[[str], bool] | None = field(init=False, default=None) #: which will be set .gen to assets. Purpose is to make it possible to get #: summary statistics while already downloading. TODO: reimplement #: properly! @@ -292,6 +298,11 @@ def __post_init__(self, output_dir: str | Path) -> None: else: self.output_prefix = Path() self.output_path = Path(output_dir, self.output_prefix) + self.url_zarr_filters = self.url.get_zarr_filter() + all_filters = self.url_zarr_filters + list(self.zarr_filters) + self.zarr_entry_filter = ( + make_zarr_entry_filter(all_filters) if all_filters else None + ) def is_dandiset_yaml(self) -> bool: return isinstance(self.url, AssetItemURL) and self.url.path == "dandiset.yaml" @@ -345,6 +356,17 @@ def download_generator(self) -> Iterator[dict]: download_path = Path(self.output_path, path) path = str(self.output_prefix / path) + if self.url_zarr_filters and asset.asset_type is not AssetType.ZARR: + # The URL named a path inside the asset, but the asset is + # not a Zarr, so there is nothing to descend into. + yield { + "path": path, + "status": "error", + "message": f"Asset {asset.path!r} is not a Zarr asset," + " so it has no entries to download", + } + continue + try: metadata = asset.get_raw_metadata() except NotFoundError as e: @@ -398,6 +420,7 @@ def download_generator(self) -> Iterator[dict]: jobs=self.jobs_per_zarr, lock=lock, zarr_entry_filter=self.zarr_entry_filter, + required_filters=self.url_zarr_filters, ) def _progress_filter(gen): @@ -1041,6 +1064,7 @@ def _download_zarr( lock: Lock, jobs: int | None = None, zarr_entry_filter: Callable[[str], bool] | None = None, + required_filters: Sequence[ZarrFilter] = (), ) -> Iterator[dict]: # Avoid heavy import by importing within function: from .support.digests import get_zarr_checksum @@ -1050,16 +1074,33 @@ def _download_zarr( entries: list = [] digests: dict[str, str] = {} pc = ProgressCombiner(zarr_size=asset.size) + # `zarr_entry_filter` is the OR of the required filters and the ``--zarr`` + # ones, so a non-empty `entries` does not mean the required filters + # matched; track them separately. + required_match = ( + make_zarr_entry_filter(list(required_filters)) if required_filters else None + ) + matched_required = False + + def unmatched_required_error() -> dict: + patterns = ", ".join(repr(f.pattern) for f in required_filters) + return { + "status": "error", + "message": f"No entries in the Zarr asset match {patterns}", + } def digest_callback(path: str, algoname: str, d: str) -> None: if algoname == "md5": digests[path] = d def downloads_gen(): + nonlocal matched_required for entry in asset.iterfiles(): entry_path = str(entry) if zarr_entry_filter is not None and not zarr_entry_filter(entry_path): continue + if required_match is not None and required_match(entry_path): + matched_required = True entries.append(entry) etag = entry.digest assert etag.algorithm is DigestType.md5 @@ -1094,11 +1135,19 @@ def downloads_gen(): if final_out is not None: break else: - if zarr_entry_filter is not None: + if required_filters and not matched_required: + yield unmatched_required_error() + elif zarr_entry_filter is not None: # Filter matched no entries; still report completion yield {"status": "done"} return + if required_filters and not matched_required: + # Entries downloaded for the ``--zarr`` filters do not make up for the + # ones the URL named but the asset does not have. + yield unmatched_required_error() + return + if zarr_entry_filter is not None: # Partial download: skip deleting extra local files and skip # whole-zarr checksum verification (individual file checksums diff --git a/dandi/tests/test_dandiarchive.py b/dandi/tests/test_dandiarchive.py index 1154b025b..4f00707ba 100644 --- a/dandi/tests/test_dandiarchive.py +++ b/dandi/tests/test_dandiarchive.py @@ -16,6 +16,7 @@ BaseAssetIDURL, DandisetURL, ParsedDandiURL, + at_zarr_boundary, follow_redirect, multiasset_target, parse_dandi_url, @@ -23,6 +24,7 @@ ) from dandi.exceptions import FailedToConnectError, NotFoundError, UnknownURLError from dandi.tests.skip import mark +from dandi.zarr_filter import ZarrFilter from .fixtures import DandiAPI, SampleDandiset @@ -290,6 +292,38 @@ zarr_subpath="scale0/0/0", ), ), + ( # the GUI's own "browse to this zarr" URL: the parser appends the + # trailing slash, which is not meaningful at a zarr boundary + "https://dandiarchive.org/dandiset/000108/draft/files" + "?location=sub-1/file.ome.zarr", + AssetItemURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id="draft", + path="sub-1/file.ome.zarr", + ), + ), + ( # ... and one browsing into it names entries + "https://dandiarchive.org/dandiset/000108/draft/files" + "?location=sub-1/file.ome.zarr/0/0", + AssetZarrEntryURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id="draft", + asset_path="sub-1/file.ome.zarr", + zarr_subpath="0/0", + ), + ), + ( # a trailing slash below the boundary still names entries + "dandi://dandi/000108/sub-1/file.ome.zarr/0/0/", + AssetZarrEntryURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + asset_path="sub-1/file.ome.zarr", + zarr_subpath="0/0", + ), + ), ( # plain .zarr URL without subpath should stay AssetItemURL "dandi://dandi/000108/sub-1/file.ome.zarr", AssetItemURL( @@ -299,6 +333,15 @@ path="sub-1/file.ome.zarr", ), ), + ( # ... as should one with a trailing slash at the boundary + "dandi://dandi/000108/sub-1/file.ome.zarr/", + AssetItemURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + path="sub-1/file.ome.zarr", + ), + ), ( "https://api.dandiarchive.org/api/dandisets/000003/versions/draft" "/assets/0a748f90-d497-4a9c-822e-9c63811db412/download/", @@ -389,24 +432,53 @@ def test_parse_api_url(url: str, parsed_url: ParsedDandiURL) -> None: @pytest.mark.ai_generated @pytest.mark.parametrize( - "location,expected", + "location,split,boundary", + [ + # Crosses a zarr boundary + ("sub-1/file.ome.zarr/0/0/0", ("sub-1/file.ome.zarr", "0/0/0"), False), + ("file.zarr/scale0/data", ("file.zarr", "scale0/data"), False), + ("sub-1/file.ngff/0/0", ("sub-1/file.ngff", "0/0"), False), + ("a/b.zarr/c/d/e/f", ("a/b.zarr", "c/d/e/f"), False), + # At a zarr boundary: no subpath to split off + ("sub-1/file.ome.zarr", None, True), + ("sub-1/file.ome.zarr/", None, True), + ("sub-1/file.ngff/", None, True), + ("file.zarr", None, True), + # Not a zarr at all + ("sub-1/file.nwb", None, False), + ("some/path/file.txt", None, False), + ("sub-1/", None, False), + ("", None, False), + ], +) +def test_zarr_location_helpers( + location: str, split: tuple[str, str] | None, boundary: bool +) -> None: + """`split_zarr_location()` finds a subpath; `at_zarr_boundary()` finds the asset.""" + assert split_zarr_location(location) == split + assert at_zarr_boundary(location) is boundary + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "url,expected", [ - # Crosses zarr boundary - ("sub-1/file.ome.zarr/0/0/0", ("sub-1/file.ome.zarr", "0/0/0")), - ("file.zarr/scale0/data", ("file.zarr", "scale0/data")), - ("sub-1/file.ngff/0/0", ("sub-1/file.ngff", "0/0")), - # No zarr extension - ("sub-1/file.nwb", None), - ("some/path/file.txt", None), - # Zarr without subpath — no split - ("sub-1/file.ome.zarr", None), - ("file.zarr", None), - # Deeply nested subpath - ("a/b.zarr/c/d/e/f", ("a/b.zarr", "c/d/e/f")), + # Only a URL pointing *inside* a zarr restricts the download + ( + "dandi://dandi/000108/sub-1/file.ome.zarr/0/0/0", + [ZarrFilter("path", "0/0/0")], + ), + ("dandi://dandi/000108/sub-1/file.ome.zarr/0/0/", [ZarrFilter("path", "0/0")]), + # Everything else leaves zarr assets unfiltered + ("dandi://dandi/000108", []), + ("dandi://dandi/000108/sub-1/file.ome.zarr", []), + ("dandi://dandi/000108/sub-1/file.ome.zarr/", []), + ("dandi://dandi/000108/sub-1/file.nwb", []), + ("dandi://dandi/000108/sub-1/", []), ], ) -def test_split_zarr_location(location: str, expected: tuple[str, str] | None) -> None: - assert split_zarr_location(location) == expected +def test_get_zarr_filter(url: str, expected: list[ZarrFilter]) -> None: + assert parse_dandi_url(url).get_zarr_filter() == expected @pytest.mark.parametrize( @@ -431,6 +503,16 @@ def test_split_zarr_location(location: str, expected: tuple[str, str] | None) -> path="f*/bar.nwb", ), ), + ( + # a zarr-looking path is still a glob, trailing slash and all: + "dandi://dandi/000108/sub-1/*.zarr/a/", + AssetGlobURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + path="sub-1/*.zarr/a/", + ), + ), ( # `path=` does not produce a glob: "https://api.dandiarchive.org/api/dandisets/000003/versions/draft" diff --git a/dandi/tests/test_delete.py b/dandi/tests/test_delete.py index 9a6ff8dd1..250f86a90 100644 --- a/dandi/tests/test_delete.py +++ b/dandi/tests/test_delete.py @@ -407,6 +407,30 @@ def test_delete_version( delete_spy.assert_not_called() +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "suffix", ["/acquisition/data_00000_AD0", "/acquisition/data_00000_AD0/"] +) +def test_delete_within_zarr_refused( + mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch, suffix: str +) -> None: + """A URL inside a Zarr asset must not delete the whole asset. + + `AssetZarrEntryURL.get_assets()` yields the Zarr asset itself, so without + a guard the deletion would take every entry, not the named ones. + """ + delete_spy = mocker.spy(RESTFullAPIClient, "delete") + with pytest.raises(NotImplementedError) as excinfo: + delete( + [f"dandi://dandi/000108/sub-1/file.ome.zarr{suffix}"], + dandi_instance="dandi", + devel_debug=True, + force=True, + ) + assert "Cannot delete individual entries within a Zarr asset" in str(excinfo.value) + delete_spy.assert_not_called() + + def test_delete_no_dandiset( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 43bb9579f..7df8e74cb 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Callable, Iterator +from collections.abc import Callable, Iterator, Sequence from contextlib import nullcontext from datetime import datetime, timedelta, timezone from email.utils import parsedate_to_datetime @@ -19,7 +19,7 @@ from typing import Any from unittest import mock -from dandischema.models import ID_PATTERN +from dandischema.models import ID_PATTERN, DigestType import numpy as np import pytest from pytest_mock import MockerFixture @@ -28,11 +28,13 @@ import responses import zarr +import dandi.download + from .fixtures import SampleDandiset, SampleDandisetFactory from .skip import mark from .test_helpers import TWO_ARRAY_ZARR_LAYOUT, assert_dirtrees_eq, zarr_format_of from ..consts import DRAFT, MTIME_TOLERANCE, SyncMode, dandiset_metadata_file -from ..dandiarchive import DandisetURL +from ..dandiarchive import DandisetURL, parse_dandi_url from ..download import ( DownloadDirectory, Downloader, @@ -46,8 +48,10 @@ download, ) from ..exceptions import NotFoundError +from ..misctypes import Digest from ..support.digests import Digester from ..utils import list_paths, yaml_load +from ..zarr_filter import ZarrFilter, make_zarr_entry_filter # both urls point to 000027 (lean test dataset), and both draft and "released" @@ -1533,49 +1537,30 @@ def test__check_attempts_and_sleep_retries(status_code: int) -> None: @pytest.mark.ai_generated -def test_download_zarr_with_glob_filter( - tmp_path: Path, zarr_dandiset: SampleDandiset +@pytest.mark.parametrize( + "zarr_filters", + [("glob:**/.z*", "glob:**/zarr.json"), ("metadata",)], + ids=["glob", "alias"], +) +def test_download_zarr_metadata_only( + tmp_path: Path, zarr_dandiset: SampleDandiset, zarr_filters: tuple[str, ...] ) -> None: - """Download only metadata files from a zarr asset via a glob filter. + """Download only the metadata files of a zarr asset. Zarr v2 stores metadata in dot-files (``.zarray``/``.zgroup``/``.zattrs``), while Zarr v3 uses ``zarr.json``. Match both so the test is agnostic to the on-disk format produced by the installed zarr-python. """ download( - zarr_dandiset.dandiset.version_api_url, - tmp_path, - zarr_filters=("glob:**/.z*", "glob:**/zarr.json"), + zarr_dandiset.dandiset.version_api_url, tmp_path, zarr_filters=zarr_filters ) zarr_dir = tmp_path / zarr_dandiset.dandiset_id / "sample.zarr" assert zarr_dir.exists() - # All downloaded files should be metadata files. all_files = list_paths(zarr_dir) assert len(all_files) > 0 for f in all_files: - assert f.name.startswith(".z") or f.name == "zarr.json", ( - f"Non-metadata file downloaded: {f}" - ) - - -@pytest.mark.ai_generated -def test_download_zarr_metadata_alias( - tmp_path: Path, zarr_dandiset: SampleDandiset -) -> None: - """Test the 'metadata' alias for --zarr.""" - download( - zarr_dandiset.dandiset.version_api_url, - tmp_path, - zarr_filters=("metadata",), - ) - zarr_dir = tmp_path / zarr_dandiset.dandiset_id / "sample.zarr" - assert zarr_dir.exists() - all_files = list_paths(zarr_dir) - assert len(all_files) > 0 - for f in all_files: - assert f.name.startswith(".z") or f.name in ( - "zarr.json", - ".zmetadata", + assert ( + f.name.startswith(".z") or f.name == "zarr.json" ), f"Non-metadata file downloaded: {f}" @@ -1643,12 +1628,284 @@ def test_download_zarr_filter_nonexistent( @pytest.mark.ai_generated -def test_download_zarr_sync_conflict() -> None: - """--sync and --zarr cannot be used together.""" - with pytest.raises(ValueError, match="--sync and --zarr cannot be used together"): - download( +@pytest.mark.parametrize( + "url,zarr_filters,error", + [ + ( "https://dandiarchive.org/dandiset/000027", - "/tmp/unused", - sync=True, - zarr_filters=("metadata",), + ("metadata",), + "--sync and --zarr cannot be used together", + ), + ( + "dandi://dandi/000027/sample.zarr/0/0", + (), + "--sync cannot be used with a URL pointing inside a Zarr asset", + ), + ], +) +def test_download_zarr_sync_conflict( + url: str, zarr_filters: tuple[str, ...], error: str +) -> None: + """A partial zarr download and ``--sync`` are mutually exclusive.""" + with pytest.raises(ValueError, match=re.escape(error)): + download(url, "/tmp/unused", sync=True, zarr_filters=zarr_filters) + + +def _make_downloader( + url: str, tmp_path: Path, zarr_filters: Sequence[ZarrFilter] = () +) -> Downloader: + return Downloader( + url=parse_dandi_url(url), + output_dir=tmp_path, + existing=DownloadExisting.ERROR, + get_metadata=True, + get_assets=True, + preserve_tree=False, + jobs_per_zarr=None, + on_error="raise", + zarr_filters=list(zarr_filters), + ) + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "url,zarr_filters,included,excluded,url_filters", + [ + # A URL inside a zarr restricts that download to its subpath ... + ( + "dandi://dandi/000108/sub-1/file.ome.zarr/0/0", + (), + ["0/0/.zarray"], + ["1/1/.zarray"], + [ZarrFilter("path", "0/0")], + ), + # ... and no other URL in the same invocation is affected + ("dandi://dandi/000108/sub-2/other.ome.zarr", (), None, None, []), + # ``--zarr`` filters apply to every asset, whatever the URL + ( + "dandi://dandi/000108/sub-2/other.ome.zarr", + (ZarrFilter("path", "a"),), + ["a/data.bin"], + ["b/data.bin"], + [], + ), + ], +) +def test_downloader_zarr_filters( + tmp_path: Path, + url: str, + zarr_filters: Sequence[ZarrFilter], + included: list[str] | None, + excluded: list[str] | None, + url_filters: list[ZarrFilter], +) -> None: + """Only the URL's own subpath filters its download; ``--zarr`` filters all. + + ``included``/``excluded`` of `None` means no filtering at all, i.e. the + whole zarr is downloaded. A non-empty ``url_filters`` marks the entries as + named by the user, so matching none of them is an error. + """ + dl = _make_downloader(url, tmp_path, zarr_filters) + assert dl.url_zarr_filters == url_filters + if included is None: + assert dl.zarr_entry_filter is None + else: + assert dl.zarr_entry_filter is not None + assert all(dl.zarr_entry_filter(p) for p in included) + assert excluded is not None + assert not any(dl.zarr_entry_filter(p) for p in excluded) + + +def _upload_two_zarrs(ds: SampleDandiset) -> None: + """Upload ``sample.zarr`` (subdirs a, b) and ``other.zarr`` (subdirs c, d).""" + for name, subdirs in [("sample.zarr", "ab"), ("other.zarr", "cd")]: + zf = ds.dspath / name + zf.mkdir() + for sub in subdirs: + (zf / sub).mkdir() + (zf / sub / "data.bin").write_text(f"data-{sub}") + ds.upload(validation="skip") + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "suffix,whole_zarr", + [ + # A subpath fetches only that subtree ... + ("/a", False), + # ... and a trailing slash below the boundary means the same + ("/a/", False), + # At the boundary, with or without a slash, the whole zarr is fetched + ("", True), + ("/", True), + ], +) +def test_download_zarr_url_subpath( + tmp_path: Path, new_dandiset: SampleDandiset, suffix: str, whole_zarr: bool +) -> None: + """A URL inside a zarr fetches that subtree; a trailing slash is ignored.""" + _upload_two_zarrs(new_dandiset) + download( + f"dandi://{new_dandiset.api.instance_id}" + f"/{new_dandiset.dandiset_id}/sample.zarr{suffix}", + tmp_path, + ) + zarr_dir = tmp_path / "sample.zarr" + assert (zarr_dir / "a" / "data.bin").read_text() == "data-a" + if whole_zarr: + assert (zarr_dir / "b" / "data.bin").read_text() == "data-b" + else: + assert not (zarr_dir / "b").exists() + + +@pytest.mark.ai_generated +def test_download_url_subpath_of_non_zarr_asset( + tmp_path: Path, new_dandiset: SampleDandiset +) -> None: + """A subpath under a blob that merely *looks* like a Zarr is an error. + + The URL's subpath cannot be honoured, and downloading the whole blob + instead would silently give the user something they did not ask for. + """ + (new_dandiset.dspath / "sample.zarr").write_text("This is not a Zarr.\n") + new_dandiset.upload(allow_any_path=True) + with pytest.raises(RuntimeError, match="1 error while downloading"): + download( + f"dandi://{new_dandiset.api.instance_id}" + f"/{new_dandiset.dandiset_id}/sample.zarr/0/0", + tmp_path, ) + + +@pytest.mark.ai_generated +def test_download_zarr_url_subpath_nonexistent( + tmp_path: Path, new_dandiset: SampleDandiset, caplog: pytest.LogCaptureFixture +) -> None: + """A URL naming a nonexistent path inside a Zarr asset errors out.""" + _upload_two_zarrs(new_dandiset) + with pytest.raises(RuntimeError, match="1 error while downloading"): + download( + f"dandi://{new_dandiset.api.instance_id}" + f"/{new_dandiset.dandiset_id}/sample.zarr/nonexistent", + tmp_path, + ) + assert "No entries in the Zarr asset match 'nonexistent'" in caplog.text + + +@pytest.mark.ai_generated +def test_download_zarr_url_subpath_does_not_filter_other_urls( + tmp_path: Path, new_dandiset: SampleDandiset +) -> None: + """A Zarr subpath in one URL must not restrict a second URL's download.""" + _upload_two_zarrs(new_dandiset) + prefix = f"dandi://{new_dandiset.api.instance_id}/{new_dandiset.dandiset_id}" + download([f"{prefix}/sample.zarr/a", f"{prefix}/other.zarr"], tmp_path) + # First URL: only the requested subtree + assert (tmp_path / "sample.zarr" / "a" / "data.bin").exists() + assert not (tmp_path / "sample.zarr" / "b").exists() + # Second URL: the whole Zarr, unaffected by the first URL's subpath + assert (tmp_path / "other.zarr" / "c" / "data.bin").read_text() == "data-c" + assert (tmp_path / "other.zarr" / "d" / "data.bin").read_text() == "data-d" + + +class _FakeZarrEntry: + """Minimal stand-in for a `RemoteZarrEntry` for `_download_zarr` tests.""" + + def __init__(self, path: str, size: int = 10) -> None: + self.path = path + self.size = size + self.digest = Digest(algorithm=DigestType.md5, value="0" * 32) + self.modified = datetime(2024, 1, 1, tzinfo=timezone.utc) + + def __str__(self) -> str: + return self.path + + def get_download_file_iter(self) -> Callable[[], Iterator[bytes]]: + return lambda: iter([]) + + +class _FakeZarrAsset: + """Minimal stand-in for a `BaseRemoteZarrAsset` for `_download_zarr` tests.""" + + def __init__(self, paths: Sequence[str]) -> None: + self.paths = list(paths) + self.size = 10 * len(self.paths) + + def iterfiles(self, prefix: str | None = None) -> Iterator[_FakeZarrEntry]: + return iter([_FakeZarrEntry(p) for p in self.paths]) + + +def _run_download_zarr( + tmp_path: Path, + paths: Sequence[str], + filters: Sequence[ZarrFilter], + required_filters: Sequence[ZarrFilter] = (), +) -> list[dict]: + """Drive `_download_zarr` over a fake asset, stubbing out the file transfer.""" + with mock.patch.object( + dandi.download, + "_download_file", + lambda *args, **kwargs: iter( + [{"size": 10}, {"status": "downloading"}, {"status": "done"}] + ), + ): + return list( + dandi.download._download_zarr( + _FakeZarrAsset(paths), # type: ignore[arg-type] + tmp_path / "sample.zarr", + toplevel_path=tmp_path, + existing=DownloadExisting.OVERWRITE, + lock=Lock(), + jobs=1, + zarr_entry_filter=make_zarr_entry_filter(list(filters)), + required_filters=list(required_filters), + ) + ) + + +NO_MATCH = "No entries in the Zarr asset match 'nonexistent'" + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "filters,required,error,downloads", + [ + # A URL subpath matching nothing is an error ... + ([ZarrFilter("path", "nonexistent")], True, NO_MATCH, False), + # ... even when ``--zarr`` filters match and entries are downloaded + ( + [ZarrFilter("path", "nonexistent"), ZarrFilter("glob", "**/.z*")], + True, + NO_MATCH, + True, + ), + # Without a URL subpath, matching nothing is a silent no-op + ([ZarrFilter("path", "nonexistent")], False, None, False), + # A URL subpath that does match downloads without error + ([ZarrFilter("path", "a")], True, None, True), + ], +) +def test_download_zarr_required_filters( + tmp_path: Path, + filters: list[ZarrFilter], + required: bool, + error: str | None, + downloads: bool, +) -> None: + """Entries named by a URL must exist; ``--zarr`` filters need not match. + + ``required`` marks the first filter as URL-derived. ``downloads`` says + whether any entry was expected to be fetched, which is what distinguishes + "matched nothing at all" from "matched only the ``--zarr`` filters". + """ + out = _run_download_zarr( + tmp_path, + [".zgroup", "a/data.bin"], + filters, + required_filters=filters[:1] if required else (), + ) + errors = [r for r in out if r.get("status") == "error"] + assert [r["message"] for r in errors] == ([error] if error else []) + assert any("size" in r or "done" in r for r in out) is downloads + if error is None: + assert out[-1] == {"status": "done"} diff --git a/docs/source/cmdline/download.rst b/docs/source/cmdline/download.rst index 32407ac76..51f93e4fc 100644 --- a/docs/source/cmdline/download.rst +++ b/docs/source/cmdline/download.rst @@ -65,3 +65,61 @@ Options .. option:: --sync Delete local assets that do not exist on the server after downloading + + Cannot be combined with ``--zarr`` or with a URL that points inside a Zarr + asset: a partial download of a Zarr leaves out entries that are on the + server, which ``--sync`` would then delete locally. + +.. option:: --zarr + + Download only the entries within Zarr assets that match ``filter``, given + as :samp:`{type}:{pattern}` where ``type`` is one of: + + ``glob`` + Match the entry path against a glob pattern. ``*`` matches within a + single path component and ``**`` matches across components, e.g. + ``glob:**/.zarray``. + + ``path`` + Match the entry at ``pattern`` and everything under it, e.g. + ``path:0/0``. + + ``regex`` + Search the entry path for a Python regular expression. The match is + unanchored, so anchor it yourself to match from the start, e.g. + ``regex:^0/[0-9]+/``. + + In place of :samp:`{type}:{pattern}`, the predefined filter ``metadata`` + may be given; it selects the Zarr metadata files, i.e. any entry named + ``zarr.json`` or whose name begins with ``.z`` (``.zarray``, ``.zgroup``, + ``.zattrs``, ``.zmetadata``), at any depth. + + The option may be given more than once, in which case an entry is + downloaded if it matches **any** of the filters. + + A URL that points inside a Zarr asset (see :ref:`resource_ids`) selects + entries in the same way, as though ``path:`` had been given for the + portion of the URL below the Zarr asset:: + + dandi download dandi://dandi/000108/sub-1/file.ome.zarr/0/0 + + Unlike ``--zarr``, such a URL names entries that the Dandiset is expected + to have: if no entry matches, the download fails rather than quietly + downloading nothing. + + .. warning:: + + A URL subpath and ``--zarr`` are **unioned**, not intersected, so + passing both *widens* the download rather than narrowing it. In:: + + dandi download --zarr metadata \ + dandi://dandi/000108/sub-1/file.ome.zarr/0/0 + + the entries under ``0/0`` are downloaded *and* so is every metadata + file anywhere in the Zarr. To restrict metadata to a subtree, give + the subtree in the filter itself (``--zarr 'glob:0/0/**/.z*'``) rather + than in the URL. + + Because only part of a Zarr is fetched, extra local files are not deleted + and the Zarr checksum of the result is not verified; the checksum of each + individual downloaded entry still is. diff --git a/docs/source/ref/urls.rst b/docs/source/ref/urls.rst index d314d6d6a..895332257 100644 --- a/docs/source/ref/urls.rst +++ b/docs/source/ref/urls.rst @@ -37,9 +37,20 @@ has one, and its draft version will be used otherwise. a collection of assets whose paths match the glob pattern ``path``, and `parse_dandi_url()` will convert the URL to an `AssetGlobURL`. - - If the ``glob``/``--path-type glob`` option is not in effect, the URL - refers to an asset folder by path, and `parse_dandi_url()` will convert the - URL to an `AssetFolderURL`. + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` + descends into a Zarr asset, the URL refers to the entries at or under that + location within the Zarr, and `parse_dandi_url()` will convert the URL to + an `AssetZarrEntryURL`. See :ref:`zarr_entry_urls` below. + + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` + ends *at* a Zarr asset, the URL refers to that asset, and + `parse_dandi_url()` will convert the URL to an `AssetItemURL`. Note that + this applies to a plain "browse to this Zarr" GUI URL, as the trailing + slash such a URL carries is not meaningful at a Zarr boundary. + + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` + does not involve a Zarr asset, the URL refers to an asset folder by path, + and `parse_dandi_url()` will convert the URL to an `AssetFolderURL`. - :samp:`https://{server}[/api]/dandisets/{dandiset-id}[/versions[/{version}]]` — Refers to a Dandiset. `parse_dandi_url()` converts this format to a @@ -58,6 +69,10 @@ has one, and its draft version will be used otherwise. prefix ``path``. `parse_dandi_url()` converts this format to an `AssetPathPrefixURL`. + Note that, unlike the forms above, ``path`` here is a plain prefix and is + not interpreted against Zarr boundaries, so a prefix reaching inside a Zarr + asset matches no assets. + - :samp:`https://{server}[/api]/dandisets/{dandiset-id}/versions/{version}/assets/?glob={path}` — Refers to all assets in the given Dandiset whose paths match the glob pattern ``path``. `parse_dandi_url()` converts this format to an @@ -76,11 +91,48 @@ has one, and its draft version will be used otherwise. `parse_dandi_url()` will convert the URL to an `AssetGlobURL`. - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` - ends with a trailing slash, the URL refers to an asset folder by path, and - `parse_dandi_url()` will convert the URL to an `AssetFolderURL`. + descends into a Zarr asset, the URL refers to the entries at or under that + location within the Zarr, and `parse_dandi_url()` will convert the URL to + an `AssetZarrEntryURL`. See :ref:`zarr_entry_urls` below. + + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` + ends with a trailing slash but does not end at a Zarr asset, the URL + refers to an asset folder by path, and `parse_dandi_url()` will convert + the URL to an `AssetFolderURL`. - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` - does not end with a trailing slash, the URL refers to a single asset by - path, and `parse_dandi_url()` will convert the URL to an `AssetItemURL`. + either does not end with a trailing slash or ends at a Zarr asset, the URL + refers to a single asset by path, and `parse_dandi_url()` will convert the + URL to an `AssetItemURL`. - Any other HTTPS URL that redirects to one of the above + + +.. _zarr_entry_urls: + +Paths Within Zarr Assets +------------------------ + +A Zarr asset is a directory, and the paths inside it are entries of that asset +rather than assets of their own. A URL whose path continues past a Zarr asset +therefore refers to entries within it. The boundary is recognised by the +extensions in ``dandi.consts.ZARR_EXTENSIONS`` (:file:`.zarr` and +:file:`.ngff`), so in:: + + dandi://dandi/000108/sub-1/file.ome.zarr/0/0 + +the asset is :file:`sub-1/file.ome.zarr` and ``0/0`` names the entries at or +under :file:`0/0` within it. `parse_dandi_url()` converts this to an +`AssetZarrEntryURL`. + +:program:`dandi download` recreates the Zarr's leading directories locally and +fetches only the matching entries; see the ``--zarr`` option of +:doc:`dandi download `. :program:`dandi ls` +lists the matching entries. Because such a URL names entries the Dandiset is +expected to have, a download whose path matches no entry fails rather than +quietly downloading nothing. + +A trailing slash is not meaningful at or below a Zarr boundary, since entries +within a Zarr are not assets: :samp:`{...}/file.ome.zarr/0/0/` is equivalent to +:samp:`{...}/file.ome.zarr/0/0`, and :samp:`{...}/file.ome.zarr/` refers to the +Zarr asset as a whole, just as :samp:`{...}/file.ome.zarr` does.