Fix partial Zarr download by URL: per-URL filters, missing subpaths, trailing slashes - #1929
Open
yarikoptic wants to merge 6 commits into
Open
yarikoptic wants to merge 6 commits into
yarikoptic wants to merge 6 commits into
Conversation
Two bugs in the partial-zarr download support added in #1816, both affecting the use case of #1461 (an asset path that points within a zarr): - The filter derived from a URL's zarr subpath was accumulated into a single global list and handed to every `Downloader`, so in a multi-URL invocation one URL's subpath silently restricted the download of all the others: `dandi download <zarrA>/sub/path <zarrB>` fetched only the `sub/path`-matching subset of zarrB. URL-derived filters are now resolved per-URL through a new `ParsedDandiURL.get_zarr_filter()` hook, overridden by `AssetZarrEntryURL`. Filters from `--zarr` remain global, as before. - A subpath that matched no entries (e.g. a typo) reported "done" and exited 0 without downloading anything, where before #1816 the user got a clear "No asset at path" error. Entries named by the URL are now required to exist: matching none is reported as an error. Filters from `--zarr` keep their previous semantics of being selectors that may legitimately match nothing. Since the two filter sources are now distinguished, `--sync` reports which of them it conflicts with instead of always naming `--zarr`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG
- A `--zarr` filter that matched entries masked a URL subpath that matched none: the emptiness check looked at the combined (OR'ed) result, so `dandi download --zarr metadata <zarr>/typo` downloaded the metadata files and exited 0. Track matches of the URL-derived filters separately from the combined predicate, and check them on both the "nothing downloaded" and the "download completed" paths. - A URL with a subpath whose asset turns out not to be a Zarr (a blob named `foo.zarr`) downloaded the whole blob and discarded the subpath silently; report it as an error instead. - `AssetZarrEntryURL.get_zarr_filter()` now returns no filters for an empty `zarr_subpath`. `parse_dandi_url()` cannot produce one, but the class is public and such a filter would reject every entry. `_download_zarr()` now takes the required filters themselves rather than a prepared message, which drops the duplicate naming between it and `Downloader` and lets the message be built where the error is reported. Tests: the new `_download_zarr()` unit tests exercise the required-filter logic without a local archive instance, so the fixes stay covered in the jobs that always run; the integration test now asserts the error message rather than just that some error occurred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG
A trailing slash routed the location to `AssetFolderURL` before the zarr
boundary was considered, so `dandi://.../x.zarr/a/` failed with
No assets found under folder 'x.zarr/a/'
rather than downloading the entries under `a`. A path within a zarr can
never name a folder of assets -- entries inside a zarr are not assets --
so the boundary is now checked first.
`.../x.zarr/` (a trailing slash at the boundary, with no subpath) now
names the zarr asset itself. `AssetFolderURL` could never resolve it:
the asset's own path has no trailing slash, so nothing had that prefix
unless a Dandiset held blobs nested under a `.zarr/` path, which
uploading a zarr directory does not produce. A trailing slash is thus
ignored at and below a zarr boundary, and folders that are not zarrs are
untouched.
Docs: `--zarr` was not documented at all. `download.rst` now covers the
filter syntax, the `metadata` alias, OR composition, the URL-implied
filter and why a URL subpath that matches nothing is an error, plus the
`--sync` restriction; `urls.rst` gains a section on paths within zarr
assets, including the trailing-slash rule, and the two `location` bullets
now mention `AssetZarrEntryURL`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG
`at_zarr_boundary()` split the path only to look at its last component, which `str.endswith` already does given a tuple of suffixes. Apply the same idiom to the `any(... endswith ...)` loop in `split_zarr_location()`. Verified equivalent over the boundary cases, including repeated and bare slashes and a leading-dot `.zarr` component. Group the zarr tests into tables rather than a function per case: - `split_zarr_location()` and `at_zarr_boundary()` share one table of locations, so a location's classification is stated in one place - the `get_zarr_filter()` cases become one table of URL -> filters - the trailing-slash parsing cases move into the existing `test_parse_api_url`/`test_parse_api_url_glob` tables - the two `Downloader` filter tests become one table - the four `_download_zarr()` required-filter tests become one table - the two `--sync` conflict tests become one table - the URL-subpath download test covers all four spellings of the path, with and without a trailing slash, at and below the boundary - `--zarr`'s glob-filter and `metadata`-alias tests differed only in the filter, so they become one table too Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG
yarikoptic
added
patch
Claude
yarikoptic
changed the title
Ported verbatim from #1928 so this PR can go green before that one merges; it is a no-op once master carries the same change. The dev-deps job installs hdmf-zarr from git, which now requires Python >= 3.12, so the install step fails before any test runs. The same job is the only failure on master at this PR's base commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG
Member
Author
|
#1928 already fixes it, so I ported that change verbatim in bf3a7a4 rather than wait — the resulting file is byte-identical to #1928's, so it merges cleanly and becomes a no-op once #1928 lands. Happy to drop the commit if you'd rather keep the two PRs separate. Also added the Generated by Claude Code |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1929 +/- ##
==========================================
+ Coverage 78.14% 78.28% +0.14%
==========================================
Files 91 91
Lines 13944 14016 +72
==========================================
+ Hits 10896 10973 +77
+ Misses 3048 3043 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`Deleter.register_url()` passes any non-Dandiset URL to `register_assets_url()`, which deletes whatever `get_assets()` yields. For a URL pointing inside a Zarr that is the *whole Zarr asset*, so `dandi delete dandi://.../x.zarr/0/0` would take every entry, not the named ones. That much came with #1816, but routing the trailing-slash spellings to `AssetZarrEntryURL` widened it: `.../x.zarr/a/` used to fail safely with `NotFoundError`. Refuse such URLs outright, reusing the `get_zarr_filter()` hook, and verify no request is issued. Docs corrections: - `urls.rst` claimed a location with a trailing slash always yields an `AssetFolderURL`. That is now false when the location ends at a Zarr asset, which is exactly what a "browse to this Zarr" GUI URL produces, since the parser appends the slash itself. Both bullet lists now say so, and the `?path=` form's prefix semantics are noted as not Zarr-aware. - `download.rst` said `--zarr` downloads "only" matching entries and that a URL subpath "restricts" the download. Both are false together: the two are unioned, so passing both widens the download. Documented with the filter-side spelling that does narrow it. - The `metadata` alias selects any entry beginning with `.z`, not just the four names listed; `regex:` is `re.search`, so unanchored. Tests: cover the GUI `?location=` route to a Zarr, the "asset is not a Zarr" error path, and restore the `b/data.bin` content assertion that the parametrized merge of the URL-subpath tests had weakened to an `exists()` check. Also: hoist `tuple(ZARR_EXTENSIONS)` to a module constant instead of rebuilding it per path component, drop the dead `| None` from `_download_zarr(required_filters=...)`, and give `AssetZarrEntryURL`'s fields the `#:` documentation DEVELOPMENT.md asks for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three ways a URL pointing inside a Zarr asset (#1816) misbehaved, and documents the feature.
doneand exited 0; it is now an error.--zarrfilters keep their old semantics — they are selectors that may legitimately match nothing.dandi download <zarrA>/sub/path <zarrB>silently downloaded only a matching subset of zarrB. EachDownloadernow resolves its own URL's filters.AssetFolderURLbefore the Zarr boundary was considered, so.../x.zarr/a/failed withNo assets found under folder. A trailing slash is now ignored at and below a Zarr boundary.--zarrwas undocumented;docs/source/cmdline/download.rstnow covers it anddocs/source/ref/urls.rstgains a section on paths within Zarr assets.Towards #1461 — the remaining part of that issue is generic "path upscending"; the boundary is still found by sniffing
ZARR_EXTENSIONS, so a Zarr asset not named*.zarr/*.ngffis still not recognised.Details
Why a URL subpath is not just another
--zarrfilterEntries named by a URL are ones the user asserts exist, so matching none is an error;
--zarrselects among whatever is there. The two are tracked separately (Downloader.url_zarr_filtersvszarr_filters) because they are combined with OR into one predicate — checkingentriesalone would let a matching--zarrfilter mask a missing subpath.ParsedDandiURL.get_zarr_filter()(non-abstract, returns[]) is the hook; onlyAssetZarrEntryURLoverrides it. This is the shape sketched indoc/design/partial-zarr.mdbut not implemented in #1816.Trailing slashes
.../x.zarr/a/can only mean the directoryainside the Zarr — entries in a Zarr are not assets, so the folder reading could never resolve..../x.zarr/likewise now names the Zarr asset itself:AssetFolderURLcould never have returned it, since the asset's own path carries no trailing slash. This takes a case away fromAssetFolderURLand is the one judgement call here: it only regresses a Dandiset holding blob assets nested under a.zarr/path, which uploading a Zarr directory does not produce.Also
foo.zarr) used to download the whole blob and drop the subpath silently; now an error.--syncreports which of the two filter sources it conflicts with, rather than always naming--zarr.at_zarr_boundary()/split_zarr_location()usestr.endswith(tuple(...)); the Zarr tests are grouped into parametrized tables rather than a function per case.Verification
sphinx-build -E -Wclean; flake8, isort, black (pinned 22.3.0) and mypy clean on the changed files; 731 passing in the non-Docker suite. The integration tests here need thelocal_dandi_apifixture and first run in CI. Behaviour was also checked live against 000719: subpath with and without a trailing slash, a trailing slash at the boundary, a non-Zarr folder URL (unaffected), a missing subpath alone and combined with--zarr metadata.🤖 Generated with Claude Code
https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG