Repository navigation
fix: suggest store class when reading a packed zarr store by path - #2571
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2571 +/- ##
==========================================
- Coverage 87.49% 85.67% -1.82%
==========================================
Files 49 49
Lines 8058 8075 +17
==========================================
- Hits 7050 6918 -132
- Misses 1008 1157 +149
|
`zarr.open` treats a path like `adata.zarr.zip` as a directory store, so `read_zarr` failed with a bare `GroupNotFoundError` that gave no hint that the path needs wrapping in `zarr.storage.ZipStore`. Attach a note pointing at the right store class when the path has a suffix known to hold a whole store in one file. Reading such a path still fails, so no magic-string store inference is introduced.
Writing an AnnData through a ZipStore makes write_zarr set the root attributes after the group is written, so zarr.json is stored twice and zipfile warns about the duplicate entry. That warning is an error under --strict-warnings, which broke the zarr_io CI jobs. Write a directory store and zip its contents instead, which is what a user ends up with anyway. Also covers the non-path store branch, which returns without adding a note.
16e4573 to
1161512
Compare
|
Rebased onto current main. The block this touches was rewritten in the meantime ( Dropped the now-unused |
|
thank you! |
…ding a packed zarr store by path) (#2620) Co-authored-by: Johnson K C <[email protected]>
anndata.io.read_zarrincompatible with a Zarr ZipStorage #2152Problem
zarr.opentreats a path likeadata.zarr.zipas a directory store, finds nogroup in it, and raises a bare
GroupNotFoundError. Nothing in that error hintsthat the path has to be wrapped in a store class first, so the fix is hard to
guess:
Approach
This follows @flying-sheep's suggestion in
#2152 (comment). When
read_zarrgets a path-like whose suffix is known to hold a whole store in asingle file, the raised error gets a note naming the store class to use:
Deliberately not done, per @ilan-gold's comment on the same issue: the
suffix is not used to open a
ZipStoreautomatically. Reading such a path stillraises, so there is no magic-string store inference and no behaviour change
beyond the added note.
add_noteis already the codebase's idiom for enrichingI/O errors (
anndata._io.utils.add_key_note).The mapping from suffix to store class is a module-level constant, so other
single-file stores can be added later without touching the logic.
read_lazyopens user-supplied paths the same way(
anndata/experimental/backed/_io.py) and so has the same rough edge. I left italone to keep this focused on the reported issue. Happy to extend it there in
this PR or a follow-up, whichever you prefer.
Test plan
test_read_zarr_zip_path_suggests_store: asserts the note is attached,and that the incantation it suggests actually round-trips
test_read_zarr_dir_path_suggests_nothing: a directory store that holdsno group gets no misleading suggestion
-m "not dask_distributed"): 8051 passed, 1441 skipped,210 xfailed, 154 subtests passed in 7:45. The 3 failures are pre-existing
and unrelated (all reproduce on
main, and none touch this code path):test_write_large_categorical[zarr2/zarr3]is an OOM from running 16xdist workers locally and passes when run serially;
test_concatenate_disk.py::test_output_dir_existspasses a Windows pathto
pytest.raises(match=...), which is an invalid regex (incomplete escape \U) on Windows only.dask_distributedshard: 59 passed, 6 skippedruff check,ruff format --check,ruff check --preview --select=PLR0917,and
codespellclean on the changed files