Uh oh!
There was an error while loading. Please reload this page.
Implement Zarr V3 protocol - #898
Conversation
Define the StoreV3 class and create v3 versions of most existing stores Add a test_storage_v3.py with test classes inheriting from their v2 counterparts. Only a subset of methods involving differences in v3 behavior were overridden.
Codecov Report
@@ Coverage Diff @@## master #898 +/- ##
=========================================
Coverage 99.94% 99.95% =========================================
Files 32 33 +1 Lines 11256 13622 +2366 =========================================
+ Hits 11250 13616 +2366
Misses 6 6
|
de243b2 to
266ab58Comparezarr_version should not be in the array metadata, only the base store metadata compressor should be absent when there is no compression
classmethods adapted from zarrita code
avoid pytest error about missing fixture fix flake8 error related to zarr_version fixture
update hexdigests
No need for this class as DirectoryStoreV3 with / chunk separator can be used instead
grlee77
commented
Mar 4, 2022
|
joshmoore
left a comment
There was a problem hiding this comment.
Looking nice, @grlee77! A number of mostly aesthetic points below after an initial read. Don't think any of them are MUSTs before merging this into a dev branch. (See zarr-developers/governance#15)
The only other high-level comment, also aesthetic, is whether zarr._storage.v3 (or similar) would be appropriate earlier rather than later.
| self.rmdir() | ||
| def rmdir_abs(store: ABSStore, path=None): |
There was a problem hiding this comment.
Why is this not a method on the store?
There was a problem hiding this comment.
Not sure why I did it that way. I can make this ABSStore.rmdir and just call it from there in ABSStoreV3.rmdir
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| del store[key] | ||
| def _rmdir_from_keys_v3(store: StoreV3, path: str = "") -> None: |
There was a problem hiding this comment.
Knowing how this module is growing, is there any place relatively convenient that we could "hide" v3-specific methods?
| key == 'zarr.json') | ||
| # cannot create a group without a path in v3 | ||
| # so create /meta/root/consolidated group to store the metadata |
There was a problem hiding this comment.
Use of "/meta/root/consolidated" likely needs review at the zarr-specs level.
There was a problem hiding this comment.
I opened zarr-developers/zarr-specs#136 so we don't forget
| store = _normalize_store_arg(store) | ||
| store = _normalize_store_arg(store, zarr_version=zarr_version) | ||
| if zarr_version is None: | ||
| zarr_version = getattr(store, '_store_version', 2) |
There was a problem hiding this comment.
Worth planning now for when the default swaps from 2 to 3? Either by using a constant or by documenting the locations while you have it in mind?
There was a problem hiding this comment.
Okay, I added DEFAULT_ZARR_VERSION (currently set to 2) in place of 2 in these checks.
It is intended as a constant used by developers, not something a user would set in their code to select the default version.
These define the root path for metadata and data, respectively
Still use a default set of metadata in __init__ method of Group or Array classes. Add a _get_metadata_suffix helper that defaults to '.json' if metadata is not present.
default metadata already gets added by Metadata3.encode_hierarchy_metadata when meta=None
joshmoore
commented
Mar 15, 2022
Anything else planned from your side, @grlee77 ? |
Before release we will need to decide on where we want the import path for v3 Stores to be (currently in the same file as the V2 ones). We can think about that and a possible environment variable to enable/disable v3 visibility in a followup, though. |
joshmoore
commented
Mar 23, 2022
👍 for handling the refactoring and v3-switch in follow ups. As discussed during the past two community calls, getting this in to the mainline. 🎉 (And as promised for @grlee77: 🍪) Clear warning for @zarr-developers/core-devs : the mainline is now in an unreleasable See zarr-developers/governance#15 for some background. |
This PR builds on #897, adding zarr v3 support
to most of the high-level convenience functions (I haven't looked at the consolidated metadata yet).EDIT: I closed the prior v3 PRs leading up to this one as their commits are also included here along with additional bug fixes and tests.
TODO: