fix: suggest store class when reading a packed zarr store by path - #2571
Open
JOhnsonKC201 wants to merge 2 commits into
Open
fix: suggest store class when reading a packed zarr store by path#2571JOhnsonKC201 wants to merge 2 commits into
JOhnsonKC201 wants to merge 2 commits into
Conversation
`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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2571 +/- ##
==========================================
- Coverage 87.78% 85.90% -1.88%
==========================================
Files 49 49
Lines 7799 7816 +17
==========================================
- Hits 6846 6714 -132
- Misses 953 1102 +149
|
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.
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
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.
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