add v3 store classes (zarr v3 support part 1) #874

grlee77 · 2021-11-17T19:01:09Z

This PR contains changes to storage.py and meta.py for v3 support. I split this out from the full v3 branch and will have follow up PRs adding v3 support to the Array and Group classes as well as the higher level routines.

This PR defines StoreV3 versions of most existing Store types (no N5Store or ABSStore, currently). This will need some rounds of revision/feedback and there are a number of "TODO" comments in code where I was unsure about desired behavior of some aspect.

For the test cases, I tried to avoid a lot of the duplication by subclassing the v2 test classes, just overriding methods where it was necessary to make changes. These v3 classes are currently in a separate file, but could instead just be appended to the existing file if that is preferred. Another possible approach is to try keeping just the existing classes, but add parameterization based on the Zarr version.

For test coverage, there are a handful of functions like rmdir that support StoreLike inputs that only have test coverage via higher-level functions which are not yet included in this PR, so there will currently be some reduction in test coverage unless that code is removed or we add test cases.

[Description of PR]

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

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 · 2021-11-17T19:13:04Z

Codecov Report

Merging #874 (ba09ed5) into master (5f7e2b6) will decrease coverage by 1.92%.
The diff coverage is 80.66%.

@@            Coverage Diff             @@
##           master     #874      +/-   ##
==========================================
- Coverage   99.94%   98.02%   -1.93%     
==========================================
  Files          32       33       +1     
  Lines       11216    12359    +1143     
==========================================
+ Hits        11210    12115     +905     
- Misses          6      244     +238

Impacted Files	Coverage Δ
zarr/_storage/store.py	`69.58% <52.51%> (-30.42%)`	⬇️
zarr/tests/test_storage.py	`99.92% <66.66%> (-0.08%)`	⬇️
zarr/meta.py	`84.93% <72.02%> (-15.07%)`	⬇️
zarr/storage.py	`92.90% <74.10%> (-7.10%)`	⬇️
zarr/tests/test_storage_v3.py	`96.80% <96.80%> (ø)`
zarr/tests/util.py	`100.00% <100.00%> (ø)`

joshmoore · 2021-11-23T13:35:21Z

@grlee77 : while you hammer out the actions fun, is there anything in particular you could use feedback on?

grlee77 · 2021-11-30T19:33:10Z

while you hammer out the actions fun, is there anything in particular you could use feedback on?

I will try to summarize a bit here later today

grlee77 · 2021-12-02T20:27:25Z

Probably my biggest question regarding this PR is how to handle extended data types. I have not done this in a way consistent with the spec yet. Specifically there is a description of how data_type metadata should handle this description here. The issue is that we do not have a URI ("value must be a URI that identifies the protocol extension") yet that we can point to?

The _v3_core_type dict in meta.yml contains all the dtypes in the core v3 specification. The other types we need for various tests are:

complex (NumPy dtype.kind == 'c')
DateTime (NumPy dtype.kind in 'mM')
NumPy object arrays (NumPy dtype.kind == 'O')
NumPy structured arrays (NumPy dtype.kind == 'V')
unicode arrays (NumPy dtype.kind == 'U')
bytes arrays (NumPy dtype.kind == 'S')

The unicode and byte array cases are currently not covered in our current Zarr test suite, but they are used in the Xarray test suite downstream.

Does each of these need to have its own separate extension URI that we can point to in the data_type metadata or can we just pace a nonexistent placeholder URI for these for now? Otherwise, I can see this taking quite a while to resolve...

I don't think we should be introducing all of the ZARR_V3_ALLOW_... environment variables near the top of meta.yml in this PR and think we should probably remove those. I was using that initially in testing, but doubt we want all of those as public.

grlee77 · 2021-12-02T20:42:29Z

Actually, I see now that there are indeed already (draft) extensions for two of the above here:
https://zarr-specs.readthedocs.io/en/core-protocol-v3.0-dev/protocol/extensions.html

Let me update this PR to use data_type extensions properly

joshmoore · 2021-12-09T16:15:35Z

Let me update this PR to use data_type extensions properly

Sounds good. I do think those URIs are fairly key to the extension mechanism, so before we get too far we should have the most core ones well-defined as good examples.

cc: @alimanfoo

pep8speaks · 2021-12-15T00:42:49Z

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-12-21 11:49:10 UTC

grlee77 · 2021-12-15T14:58:11Z

Sounds good. I do think those URIs are fairly key to the extension mechanism, so before we get too far we should have the most core ones well-defined as good examples.

I have not worked on any new URI text yet, but I did update meta.yml here to read and write data_type metadata with the format described in the v3 spec for extensions. Specifically, for the core dtypes, the JSON just has a string such as >f8, but for any extended type the entry is a dict with 3 keys: extension, type and fallback as specified in this section of the spec. The spec just states that it must be "an object containing the names extension, type and fallback" so a dict seemed an easy JSON-compatible way to store that.

The newly introduced get_extended_dtype_info function converts a numpy dtype's string representation to a corresponding dict of extension info. For complex and datetime64/timedelta64 types these point to the existing URI stubs. There are 4 other types that don't have a URL to point to at the moment 1.) object arrays, 2.) structured arrays, 3.) bytestring arrays, 4.) unicode arrays. Do you think those should be 4 separate extensions or should bytestring and unicode be combined into one?

grlee77 · 2021-12-15T15:04:49Z

Also, are any of these cases where we should be specifying a fallback?. An array of complex128 can be read as a double-length array of type float64 for instance with real and imaginary values interleaved?

zarr_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

joshmoore

so a dict seemed an easy JSON-compatible way to store that.

nods

should bytestring and unicode be combined into one?

I don't know the use case(s) for bytestring. Have you seen anything mentioned?

are any of these cases where we should be specifying a fallback?. An array of complex128 can be read as a double-length array of type float64 for instance with real and imaginary values interleaved?

Is the fallback the extent of "extension subclassing"? If so, it would be good to have examples. So far it's always been time->long or similar. The complex -> 2 floats would certainly test the concept further.

cc: @DennisHeimbigner @alimanfoo

joshmoore · 2021-12-17T16:51:39Z

zarr/meta.py

+from typing import cast, Union, Any, List, Mapping as MappingType, Optional

 ZARR_FORMAT = 2
+ZARR_FORMAT_v3 = 3


Not sure if this is realistic, but I could see also adding ZARR_FORMAT_V2 such that ZARR_FORMAT=ZARR_FORMAT_V2 defines the current default with the hope of swapping that at some (well-defined?) point in the future.

could be fine to add that. not sure how this is used

currently ZARR_FORMAT_v3 is only used once to set the ZARR_FORMAT attribute in the Metadata3 class so we could just remove it if there is not another use for it.

looks like the other ZARR_FORMAT is used mostly in test cases, but also when setting metadata in n5.py

joshmoore · 2021-12-17T16:51:51Z

zarr/_storage/store.py

+
+    @staticmethod
+    def _valid_key(key: str) -> bool:
+        """


❤️ docs!

joshmoore · 2021-12-17T16:53:54Z

zarr/meta.py

+
+# FLOAT_FILLS = {"NaN": np.nan, "Infinity": np.PINF, "-Infinity": np.NINF}
+
+_default_entry_point_metadata_v3 = {


Do you have a feeling how much this is going to grow? i.e. if we should plan for this to have its own module?

grlee77 · 2021-12-17T23:07:05Z

I don't know the use case(s) for bytestring. Have you seen anything mentioned?

IIRC there is some test case in Xarray that currently tests zarr v2 with both unicode and bytestring arrays, but we don't currently have tests for either of those in this repository!

DennisHeimbigner · 2021-12-17T23:10:11Z

The usual case for bytestring is to hold something like an image.

avoids these tests running a second time when this file is called

grlee77 · 2022-03-04T18:32:53Z

closing, these commits along with additional fixes and tests are incorporated in #898

grlee77 added 2 commits November 17, 2021 13:30

add v3 store classes

64bbbd8

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.

add TODO comment to meta.py

85b8e23

Merge remote-tracking branch 'upstream/master' into v3-store-part1

5fee6fb

grlee77 added 2 commits November 30, 2021 14:37

fix flake8 errors

983d190

Merge remote-tracking branch 'upstream/master' into v3-store-part1

4057088

grlee77 mentioned this pull request Nov 30, 2021

Add StoreV3 support to Attributes (zarr v3 support part 2) #894

Closed

6 tasks

grlee77 added 2 commits December 14, 2021 19:15

follow zarr v3 spec when dealing with extension data types

9ed6181

fixes to v3 dtype handling

662e310

Merge remote-tracking branch 'upstream/master' into v3-store-part1

9950a4d

grlee77 force-pushed the v3-store-part1 branch 2 times, most recently from c24305c to 9950a4d Compare December 15, 2021 00:49

flake8 cleanup

450c575

grlee77 added 6 commits December 15, 2021 19:47

remove duplicate lines in Metadata2.encode_array_metadata

63a9e3c

Fix fields in array metadata

1a2a1ef

zarr_version should not be in the array metadata, only the base store metadata compressor should be absent when there is no compression

Fix encode/decode of codec metadata

bcb2d5f

classmethods adapted from zarrita code

add missing level to Zlib in _decode_codec_metadata

9bfeacb

add extensions entry to v3 array metadata

23aca42

dimension_separator should not be in the array metadata for v3

e47035d

joshmoore reviewed Dec 17, 2021

View reviewed changes

adjust test have dimension_separator key was removed from v3 metadata

2c21761

grlee77 and others added 2 commits December 17, 2021 18:21

add underscores to imported test classes in test_storage_v3.py

8783f44

avoids these tests running a second time when this file is called

Merge branch 'master' into v3-store-part1

ba09ed5

grlee77 closed this Mar 4, 2022


		# FLOAT_FILLS = {"NaN": np.nan, "Infinity": np.PINF, "-Infinity": np.NINF}

		_default_entry_point_metadata_v3 = {

Uh oh!

add v3 store classes (zarr v3 support part 1) #874

add v3 store classes (zarr v3 support part 1) #874

Uh oh!

Conversation

grlee77 commented Nov 17, 2021

Uh oh!

codecov bot commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joshmoore commented Nov 23, 2021

Uh oh!

grlee77 commented Nov 30, 2021

Uh oh!

grlee77 commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grlee77 commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshmoore commented Dec 9, 2021

Uh oh!

pep8speaks commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-12-21 11:49:10 UTC

Uh oh!

grlee77 commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grlee77 commented Dec 15, 2021

Uh oh!

joshmoore left a comment

Choose a reason for hiding this comment

Uh oh!

joshmoore Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

grlee77 Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

grlee77 Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

joshmoore Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

joshmoore Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

grlee77 commented Dec 17, 2021

Uh oh!

DennisHeimbigner commented Dec 17, 2021

Uh oh!

grlee77 commented Mar 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Nov 17, 2021 •

edited

Loading

grlee77 commented Dec 2, 2021 •

edited

Loading

grlee77 commented Dec 2, 2021 •

edited

Loading

pep8speaks commented Dec 15, 2021 •

edited

Loading

grlee77 commented Dec 15, 2021 •

edited

Loading