Add support for partial Zarr download and upload - #1816
Conversation
yarikoptic
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1816 +/- ##
==========================================
+ Coverage 77.45% 77.94% +0.48%
==========================================
Files 89 91 +2
Lines 13397 13792 +395
==========================================
+ Hits 10377 10750 +373
- Misses 3020 3042 +22
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:
|
yarikoptic
added
enhancement
Covers five areas:
- --zarr TYPE:PATTERN filtering for download (glob, path, regex)
- URL parsing with zarr boundary detection (AssetZarrEntryURL)
- --zarr-mode {full, patch} for upload
- Checksums and manifests (per-directory checksums are NOT
persisted on the archive; legacy .checksum files exist on S3
under zarr-checksums/ for ~72% of older zarrs but are orphaned)
- dandi ls for zarr contents
Related: #1462, #1474
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add --zarr CLI option for download to filter entries within zarr assets (glob/path/regex patterns with predefined 'metadata' alias), and --zarr-mode option for upload to support 'patch' mode (upload/update without deleting remote-only files). Key changes: - New dandi/zarr_filter.py: filter parsing, matching, and aliases - URL parsing: AssetZarrEntryURL for URLs pointing into zarr assets - Download pipeline: thread zarr_entry_filter through Downloader and _download_zarr, skip deletion and checksum when filter active - Upload pipeline: zarr_mode='patch' skips remote file deletion and client-side checksum verification - dandi ls: list zarr entries when URL points into a zarr Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Pre-compile regex patterns at ZarrFilter construction time, catching
invalid patterns early instead of on every matches() call (B1)
- Remove redundant get_asset_download_path override in AssetZarrEntryURL
that was identical to the inherited SingleAssetURL method (H1)
- Use Literal["full", "patch"] for zarr_mode parameter instead of bare
str to prevent silent misbehavior on invalid values (H2)
- Collapse consecutive ** glob segments to avoid exponential
backtracking in _glob_match_parts (H3)
- Simplify split_zarr_location to use str.split instead of
PurePosixPath (M1)
- Add explanatory comment for type: ignore in parse_zarr_filter (M2)
- Yield {"status": "done"} when zarr filter matches zero entries (M4)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
yarikoptic
changed the title
click.Choice returns str at runtime, but upload() expects Literal["full", "patch"]. Add typing cast at the call site. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Follow existing codebase pattern (UploadExisting, DownloadExisting, etc.) to define ZarrMode as a str Enum in upload.py. Eliminates the duplicated Literal["full", "patch"] across three files and the cast() workaround in cmd_upload.py. Uses TYPE_CHECKING guard in files/zarr.py to avoid circular import (files/zarr.py -> upload.py -> .files). Co-Authored-By: Claude Code 2.1.63 / Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Code 2.1.63 / Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Kabilar Gunalan <kabilar.gunalan@gmail.com>
|
Hi @yarikoptic, thanks for this pull request. I was able to add objects to an existing zarr, delete objects, and modify metadata (i.e. I just have some minor feedback on the terminal output. When I run And then when I modify the zarr locally and run So I would propose the following changes to the terminal output:
|
|
Just explicitly noting that For For |
Six files had conflicts, all driven by three orthogonal upstream changes overlapping with the partial-zarr work: 1. --sync redesign (upstream PR "Redesign --sync as optional-value option", cb394e8): --sync switched from is_flag=True (bool) to an optional-value option resolving to None / "ask" / "do", with a SyncMode enum in dandi.consts. Function signatures for download() and upload() went from `sync: bool` to `sync: bool | SyncMode | None`, and the CLI now converts the string via `SyncMode(sync) if sync is not None else None`. 2. Zarr cleanup consolidation (upstream, commits db6bf4b..976122c): the "deleting extra files" phase in download.py was reworked into a single-pass tree scan with lazy "deleting extra files" status announcement (only yielded when something is actually deleted) and debug logging. 3. Validation companion file (upstream PR series around 3873319): upload() gained a `validation_log_path` parameter, cmd_upload.py imports SyncMode from ..consts, and computes a companion path from the CLI logfile. Partial-zarr's own additions that overlapped: - --zarr FILTER option on `dandi download` and a `zarr_filters` param threaded through to download(). - ZarrMode(str, Enum) in dandi.upload and --zarr-mode on `dandi upload`. - Skip extra-file deletion and whole-zarr checksum verification when a zarr filter is active (partial download semantics). Conflict resolutions: * dandi/cli/cmd_download.py Kept upstream's `sync: str | None` signature and `sync=SyncMode(sync) if sync is not None else None` conversion at the download() call site. Preserved partial-zarr's `zarr_filters` param and `zarr_filters=zarr_filters` kwarg. * dandi/cli/cmd_upload.py Merged imports: kept `SyncMode` from ..consts (upstream) alongside `ZarrMode` from ..upload (partial-zarr). In the upload_() call, combined upstream's `sync=SyncMode(sync) if sync is not None else None` and `validation_log_path=companion` with partial-zarr's `zarr_mode=zarr_mode`. * dandi/cli/tests/test_download.py Updated seven mock assertions to expect `sync=None` (upstream's new default) plus `zarr_filters=()` (partial-zarr's new kwarg). * dandi/download.py (signature) Signature is `sync: bool | SyncMode | None = False, zarr_filters: Sequence[str] = ()`. Bool default kept for backward compatibility with programmatic callers; upstream already handles `sync is True` by normalizing to SyncMode.ASK. * dandi/download.py (zarr cleanup block) Kept partial-zarr's outer guard `if zarr_entry_filter is not None: ... else:` so partial downloads skip both extra-file deletion and whole-zarr checksum verification. Inside the `else` branch, adopted upstream's single-pass scan with the `announced` flag, typed `dirs: deque[Path]`, and debug logging for both file deletion and empty-directory removal. The "skipped in final_out['message']" checksum block remains nested inside the same else, consistent with the pre-existing partial-zarr layout. * dandi/files/zarr.py Combined imports at the top of the file: `TYPE_CHECKING, Any, Optional` (partial-zarr, needed for the `ZarrMode` forward reference) plus `import urllib.parse` (upstream), and a TYPE_CHECKING guarded `from ..upload import ZarrMode` block to avoid a circular import. * dandi/upload.py Signature is `sync: bool | SyncMode | None = False, zarr_mode: ZarrMode = ZarrMode.FULL, validation_log_path: str | Path | None = None`. All three params are used later in the function (sync normalization at ~line 525, zarr_mode threaded into upload kwargs at ~line 415, validation_log_path at ~line 315). No conflicts in dandi/dandiarchive.py, dandi/tests/test_dandiarchive.py, dandi/tests/test_download.py, dandi/tests/test_upload.py — auto-merged. Co-Authored-By: Claude Code 2.1.138 / Claude Opus 4.7 <noreply@anthropic.com>
kabilar
changed the title
Restructure the design doc's "Primary Use Case" section into a "Use Cases" section covering: 1. Update metadata (the original scenario) 2. Incrementally add shards to an existing Zarr per @kabilar's review suggestion on PR #1816 (#1816 (comment)). The motivation for Use Case 2 is producing large datasets in shards when the full Zarr cannot fit locally. No source changes are needed -- the existing --zarr-mode patch code path already supports Use Case 2 by construction: new local files are uploaded, remote-only files are preserved, and whole-Zarr checksum verification is skipped so a pruned local checkout does not spuriously mismatch. Add a regression test test_upload_zarr_patch_mode_incremental_shards that explicitly exercises Use Case 2: prunes the fixture Zarr's local tree down to metadata files, drops in a synthetic shard, uploads with zarr_mode="patch", and asserts both that the new shard reached the remote and that every previously-pruned remote entry is still present. Co-Authored-By: Kabilar Gunalan <871137+kabilar@users.noreply.github.com> Co-Authored-By: Claude Code 2.1.138 / Claude Opus 4.7 <noreply@anthropic.com>
Awesome, thank you |
|
Thanks @kabilar for the detailed report and the concrete before/after mockups -- both are real bugs in what
Both are orthogonal to the partial-zarr download/upload plumbing this PR introduces (they would apply just as much on `master` today), so I'd like to keep the scope of #1816 focused on landing partial-zarr and track your terminal-output feedback in a follow-up issue. Would that work for you? I can open the follow-up issue and reference this comment. |
|
as agreed at weekly meeting, here is a dedicated issue
|
|
@kabilar if overall satisfied etc, approve to be merged ;-) |
* origin/master: docs: note validation caveats in the `get_metadata` methods Update CHANGELOG.md [skip ci] Clarify the placeholder `schemaKey` in the `metadata2asset*.json` files Concentrate asset `schemaKey` setting in `set_asset_schema_key()` Make `test_prepare_metadata` independent of the `BareAsset.schemaKey` value Drop redundant `schemaKey="Asset"` from `nwb2asset` assertions Set uploaded asset metadata schemaKey to "Asset"
`dandi upload` used to report every Zarr asset the same way: the initial message was "exists - reuploading" regardless of whether the local tree was bit-identical to the remote, changing, or being added to. Kabilar's report in #1893 asked for two behaviors: * If the local Zarr matches the remote, STATUS should be "skipped" (matching the non-Zarr blob case) rather than "done". * If the local Zarr is being modified, MESSAGE should describe what is actually happening -- how many files are being added, modified, or deleted (with sizes for adds/modifies; count only for deletes, since byte counts are less informative than file counts for Zarr fan-out). The diff needed to make either decision is only known after `ZarrAsset.iter_upload` walks the local tree against the remote, so `check_replace_asset` cannot decide upfront. Instead: * `check_replace_asset` for Zarr now yields the interim message "exists - checking" and lets iter_upload refine it once the diff is computed. * `iter_upload` computes n_add / n_modify / n_delete after the compare-with-remote loop. If all three are zero, it short-circuits to a terminal yield of {status="skipped", message="identical", asset=a}. Otherwise it yields an intermediate {status="planning upload", message="adding N files (S), modifying M files (S), deleting K files"} (only non-zero clauses appear), then proceeds through the normal upload / finalize / done flow. * The consumer in `upload.py` tracks iter_upload's last-yielded status and suppresses its trailing {status="done"} yield when the inner generator finished with "skipped", so pyout's final row shows STATUS=skipped rather than being clobbered to STATUS=done. n_delete only counts remote-only leftovers in `full` mode, matching the actual delete behavior; in `patch` mode remote-only files are preserved and are (correctly) not reported as pending deletions. Added regression tests (marked `@pytest.mark.ai_generated`) that spy on `ZarrAsset.iter_upload` and assert the new yield shape for both unchanged and modified Zarrs. Refs: #1893 Co-Authored-By: Claude Code 2.1.245 / Claude Opus 4.7 <noreply@anthropic.com>
| "deleting. With 'do', delete without prompting.", | ||
| ) | ||
| @click.option( | ||
| "--zarr", |
There was a problem hiding this comment.
should this be an explicit option or inferred from metadata like neuroglancer does? or a hint when the user/agent knows specifically to do so.
There was a problem hiding this comment.
we infer already based on extensions IIRC; this is just an a new option for filter... I guess we might better call it that --zarr-filter (with --z-f shortcut)
| show_default=True, | ||
| ) | ||
| @click.option( | ||
| "--zarr-mode", |
There was a problem hiding this comment.
same comment here. we are increasing the CLI surface and should ask if these should be there or inferred.
There was a problem hiding this comment.
and the same reply here: it is not about the fact that we are dealing with zarr, but rather an option specific if working with zarr(s). Let's also add shortcut --z-m
| Parameters | ||
| ---------- | ||
| location : str | ||
| A POSIX-style path, e.g. ``"sub-1/file.ome.zarr/0/0/0"``. |
There was a problem hiding this comment.
zarr doesn't have to have a / separator. a local zarr could have been generated with a different separator (e.g. zarr/0.0.0) or sharded.
There was a problem hiding this comment.
if sharded -- don't we upload it "as is" with all its shardiness?
it is a new info for me and I guess refers to dimension_separator in v2 and now chunk_key_encoding in v3? (docs: https://zarr.readthedocs.io/en/v3.1.0/api/zarr/)
good to know but I wonder if that is an issue bigger than this "docstring fix" e.g.
- are we here to map here from
.to/and it is client's job - is our dandi-archive code capable of handling those correctly?
- does https://github.com/dandi/zarr_checksum ?
likely worth a concerted effort to support such ones (unless I am wrong and we already do). Pragmatically, does any of the actively working with DANDI groups use alternative separators @satra @kabilar ? I think worth a separate issue, and if you see what to change here specifically -- please leave a suggestion @satra
| from dataclasses import dataclass, field | ||
| from fnmatch import fnmatchcase | ||
| import re | ||
| from typing import Callable, Literal |
There was a problem hiding this comment.
this file seems to do a lot of things without using a zarr or ome-zarr library, which could help deal with versioning, chunking, and sharding, as appropriate. and for ome-zarr elements related to metadata changes.
There was a problem hiding this comment.
we could potentially later add some more "semantic driven" filtering on what to download or upload, but here it is not the goal, we just implement based on basic glob and metadata matching (agnostic whether it is ome or not) and we do not track any heavy dependencies in. Since we have those matching namespaces (e.g. glob:) we will be able to extend later with some ome-zarr: if needed . But I would not blow it up even more for this one
|
Hi @yarikoptic, just following up from our discussion last week. Is this ready again for testing? Thanks. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
Replies to @satra's replies.
Also remember "perfect is an enemy of the good"!
| "deleting. With 'do', delete without prompting.", | ||
| ) | ||
| @click.option( | ||
| "--zarr", |
There was a problem hiding this comment.
we infer already based on extensions IIRC; this is just an a new option for filter... I guess we might better call it that --zarr-filter (with --z-f shortcut)
| show_default=True, | ||
| ) | ||
| @click.option( | ||
| "--zarr-mode", |
There was a problem hiding this comment.
and the same reply here: it is not about the fact that we are dealing with zarr, but rather an option specific if working with zarr(s). Let's also add shortcut --z-m
| Parameters | ||
| ---------- | ||
| location : str | ||
| A POSIX-style path, e.g. ``"sub-1/file.ome.zarr/0/0/0"``. |
There was a problem hiding this comment.
if sharded -- don't we upload it "as is" with all its shardiness?
it is a new info for me and I guess refers to dimension_separator in v2 and now chunk_key_encoding in v3? (docs: https://zarr.readthedocs.io/en/v3.1.0/api/zarr/)
good to know but I wonder if that is an issue bigger than this "docstring fix" e.g.
- are we here to map here from
.to/and it is client's job - is our dandi-archive code capable of handling those correctly?
- does https://github.com/dandi/zarr_checksum ?
likely worth a concerted effort to support such ones (unless I am wrong and we already do). Pragmatically, does any of the actively working with DANDI groups use alternative separators @satra @kabilar ? I think worth a separate issue, and if you see what to change here specifically -- please leave a suggestion @satra
| from dataclasses import dataclass, field | ||
| from fnmatch import fnmatchcase | ||
| import re | ||
| from typing import Callable, Literal |
There was a problem hiding this comment.
we could potentially later add some more "semantic driven" filtering on what to download or upload, but here it is not the goal, we just implement based on basic glob and metadata matching (agnostic whether it is ome or not) and we do not track any heavy dependencies in. Since we have those matching namespaces (e.g. glob:) we will be able to extend later with some ome-zarr: if needed . But I would not blow it up even more for this one
The `--zarr-mode` option was declared as `click.Choice(list(ZarrMode))`
with `default="full"`. Since click 8.2, `Choice.normalize_choice()`
matches `Enum` members on `.name` rather than `.value`, so the accepted
tokens were `FULL`/`PATCH` while the declared default stayed `"full"`.
The default therefore failed its own validation and *every* invocation
of `dandi upload` -- with or without `--zarr-mode` -- aborted with a
usage error (exit code 2):
Error: Invalid value for '--zarr-mode': 'full' is not one of 'FULL', 'PATCH'
This is what failed `test_upload_validation_error_has_no_traceback`
across all test jobs, and it also meant the documented
`--zarr-mode patch` spelling never worked under click >= 8.2.
Use `EnumChoice`, the repo's `click.Choice` subclass that exists for
exactly this reason (it overrides `normalize_choice()` to match on
`.value`), consistent with `--existing`, `--sync` and `--validation` in
this same command. Add a regression test covering the default and both
explicit spellings.
Also reformat the `try:`/`upload_(...)` block left non-black-formatted by
the merge resolution in 741730b, which was failing the `lint` job with
E303 (too many blank lines) and E111 (indentation not a multiple of 4).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ghxpr3yRN5gbnbGAEvBsuL
|
ok, if no objections voiced, I will merge/release it tomorrow so it could be attempted to be used "in production", and contributions will be welcome to enhance with additional ways. |
yarikoptic-gitmate
left a comment
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Since @kabilar confirmed this PR helping out in his use-cases, and no further objections were voiced, let's proceed!
|
🚀 PR was released in |
7 participants
Summary
Design and implementation for partial zarr download and upload support, addressing #1462, #1474, and related archive issues.
The PR covers five areas:
--zarr TYPE:PATTERNfiltering fordandi download— glob, path, and regex filters for selecting entries within zarr assets, with ametadataalias for common zarr metadata filesAssetZarrEntryURLto handle URLs likedandi://dandi/000108/.../file.ome.zarr/0/0/0--zarr-mode {full, patch}fordandi upload— patch mode uploads changed files without deleting remote files absent locallyzarr_checksumlibrary but are NOT persisted (only the root digest is stored in the DB); legacy.checksumfiles exist on S3 atzarr-checksums/for ~72% of older zarrs but are orphaned since Dec 2022dandi lsfor zarr contents — listing files within a zarr asset when URL points into a zarrKey findings from investigation
zarr_checksumalgorithm IS hierarchical (Merkle tree, bottom-up viaZarrChecksumTree)ingest_zarr_archivetask computes checksums entirely in memory and stores only the root digest.checksumfiles on S3 (zarr-checksums/prefix) were written byZarrChecksumFileUpdater, removed in dandi-archive PRs Testing started to fail due to an error in parsing (?) + new deprecationwarning #1390/[FEAT] dandi fsspec filesystem #1395 (Dec 2022). Legacy files remain for older zarrs but no API exposes themImplementation
New files
dandi/zarr_filter.py—ZarrFilterdataclass with glob/path/regex matching,parse_zarr_filter(),make_zarr_entry_filter(),ZARR_FILTER_ALIASES(includesmetadataalias)dandi/tests/test_zarr_filter.py— 52 unit tests covering all filter types, parsing, aliases, edge cases, invalid regex validationModified files
dandi/dandiarchive.py—split_zarr_location(),AssetZarrEntryURLclass, updatedparse_dandi_url()to detect zarr boundariesdandi/download.py—zarr_filtersparameter ondownload(), filter threading throughDownloaderand_download_zarr(), skip deletion/checksum when filter activedandi/files/zarr.py—zarr_mode: Literal["full", "patch"]oniter_upload(), patch mode skips remote file deletion and client-side checksum verificationdandi/upload.py—zarr_modeparameter onupload(), conditional pass-through forZarrAssetdandi/cli/cmd_download.py—--zarrclick option (multiple, OR logic)dandi/cli/cmd_upload.py—--zarr-modeclick optiondandi/cli/cmd_ls.py— list zarr entries when URL isAssetZarrEntryURLdandi/cli/tests/test_download.py— updated mock expectations for newzarr_filtersparameterIntegration tests
dandi/tests/test_download.py— 6 tests (glob filter, metadata alias, no-delete, path filter, nonexistent filter, sync conflict)dandi/tests/test_upload.py— 3 tests (patch no-delete, full delete, patch updates)dandi/tests/test_dandiarchive.py— 3 URL parsing cases + 8split_zarr_locationcasesReview checklist
Please review the design at
doc/design/partial-zarr.mdand comment on:--zarr TYPE:PATTERNsyntax — is the filter approach right? Are glob/path/regex the right types?metadataalias expansion — doesglob:**/.z*+glob:**/zarr.json+glob:**/.zmetadatacover all cases?--zarr-mode patchsemantics — is "upload without deleting" the right default for patch?Should subtree cleanup happen?AssetZarrEntryURLwith zarr boundary detection the right approach?--syncconflict raises error, server-side glob deferred)zarr-checksums/files on S3 be cleaned up as part of this or separately?TODO
dandi/zarr_filter.py— filter parsing and matchingAssetZarrEntryURLandsplit_zarr_location()indandi/dandiarchive.py--zarroption todandi downloadCLI_download_zarr()for partial download support--zarr-modeoption todandi uploadCLIiter_upload()(dandi/files/zarr.py)zarr_modethroughdandi/upload.pydandi lszarr contents supportzarr_filter.py(52 tests)split_zarr_location(11 test cases)Literaltyping,**collapse, redundant override removal)🤖 Generated with Claude Code