dandi upload: STATUS/MESSAGE for Zarr assets is uninformative
- Dominant language
- Python
- Stars
- 28
- Forks
- 37
- Avg merge
- 1d 17h
- Merged PRs (30d)
- 9
Description
claude produced out of experience in
- https://github.com/dandi/dandi-cli/pull/1816
## Summary
`dandi upload` reports uninformative and slightly misleading `STATUS` /
`MESSAGE` values for Zarr assets. Raised by @kabilar during review of
#1816 (see [comment][k-comment]).
[k-comment]: https://github.com/dandi/dandi-cli/pull/1816#issuecomment-4919027625
## Observed behavior
### 1. Unchanged Zarr is reported as `done / exists - reuploading`
A dandiset whose local Zarr is bit-identical to the remote Zarr renders
as:
```
PATH SIZE ... STATUS MESSAGE
...atives/dandi-cli-1816-test/HG9_Z1_Y49.nii.zarr 5.9 GB ... done exists - reuploading
```
...where the same-hash short-circuit for a regular blob would render
`skipped / file exists`.
### 2. Modified Zarr uses the same non-descriptive message
After a local edit, the same asset renders `done / exists - reuploading`
with no indication of *what* changed — no distinction between additions,
deletions, and modifications, and no idea of the affected size (which
matters because the `SIZE` column reflects the whole-Zarr size, not the
delta).
## Root cause
`check_replace_asset()` in `dandi/upload.py:547-548` unconditionally
short-circuits Zarr assets:
```python
if isinstance(local_asset, ZarrAsset):
return (True, {"message": "exists - reuploading"})
```
It never compares local vs remote, and never differentiates additions
from modifications from deletions. In contrast, the non-Zarr path at
`dandi/upload.py:579-583` does perform an etag/mtime comparison and
returns `skip_file("file exists")` (i.e. `STATUS=skipped`) when the
local and remote copies match.
## Proposed changes
### Change 1 -- Detect unchanged Zarr and skip
For a `ZarrAsset` where the local tree matches the remote Zarr (e.g. by
comparing per-entry digests, or by comparing the aggregated Zarr
checksum), return `skip_file("file exists")` so that `STATUS=skipped /
MESSAGE=file exists`, matching the non-Zarr case.
Note that the diff needed to decide "unchanged" is currently only
computed inside `ZarrAsset.iter_upload()` (`dandi/files/zarr.py`) after
Zarr registration. Two implementation choices:
(a) Do a cheap upfront comparison in `check_replace_asset` (e.g. via
per-entry digest listing) at the cost of an extra API round-trip
per asset.
(b) Let `iter_upload` yield an early `skipped` status once it has
computed the diff, and have the caller replace the initial
`exists - reuploading` marker. This avoids the upfront cost
but leaks knowledge about Zarr internals into the reporter.
### Change 2 -- Describe *what* is changing
When a Zarr *is* changing, replace `exists - reuploading` with the
kind and size of the modification. @kabilar's proposed vocabulary:
- `file exists - uploading additional objects (N GB)`
- `file exists - deleting objects (N GB)`
- `file exists - modifying objects (N GB)`
The relevant sizes are already tracked inside
`ZarrAsset.iter_upload()` via `to_upload.total_size` and the
per-entry sizes of `to_delete`
(`dandi/files/zarr.py:655,728`); they just aren't surfaced as
`MESSAGE`. This overlaps with implementation choice (b) above.
### Interaction with `--zarr-mode patch` (from #1816)
Once #1816 lands, patch mode does *not* delete remote-only entries, so
"deleting objects" should not appear for `--zarr-mode patch`. The
choice of message per mode:
| Mode | Local == remote | Local adds only | Local modifies | Local also drops entries |
| ------------- | --------------- | ---------------------- | -------------------- | ------------------------ |
| `full` (dflt) | `skipped` | `- uploading … (N GB)` | `- modifying … (…)` | `- deleting … (…)` |
| `patch` | `skipped` | `- uploading … (N GB)` | `- modifying … (…)` | *not applicable* |
## Scope
Orthogonal to #1816 -- misreport exists on `master` today. Filing
separately so #1816 can land on its current scope and this can be
picked up as a UX follow-up.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with check_replace_asset() in dandi/upload.py and ZarrAsset.iter_upload() in dandi/files/zarr.py, comparing the existing non-Zarr skip behavior with the Zarr diff data. Done means unchanged Zarr assets report skipped/file exists, while changed assets expose the appropriate upload, modification, or deletion message and size, including the --zarr-mode patch behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- cli
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100