refactor metadata into metadata.v2 and metadata.v3 modules
#2059
Conversation
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
refactor metadata into v2 and v3 modules remove version signifiers from function names fix tests
d-v-b
requested review from
normanrz and
jhamman
and removed request for
normanrz
normanrz
reviewed
Jul 31, 2024
src/zarr/metadata/v2.py
Outdated
There was a problem hiding this comment.
Should this file also get an __all__?
There was a problem hiding this comment.
I'm open to this, but I think it would only affect cases where people did from zarr.metadata.v2 import *; from zarr.metadata.v2 import foo would still work whether or not foo is in __all__.
jhamman
approved these changes
Aug 9, 2024
src/zarr/metadata/v3.py
Outdated
|
|
||
| @property | ||
| def byte_count(self) -> int: | ||
| data_type_byte_counts = { |
There was a problem hiding this comment.
I know this wasn't introduced here but we should pull all of these dicts into module level variables. No need to recreate them every time we use the datatype.
src/zarr/array.py
Outdated
| @@ -92,14 +92,14 @@ def create_codec_pipeline(metadata: ArrayV2Metadata | ArrayV3Metadata) -> CodecP | |||
|
|
|||
| @dataclass(frozen=True) | |||
| class AsyncArray: | |||
| metadata: ArrayMetadata | |||
| metadata: ArrayV3Metadata | ArrayV2Metadata | |||
There was a problem hiding this comment.
I slightly favor reverting this change as a way to future proof this a bit.
6 tasks
|
this was addressed by #2163 |
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.
In
main, we handle v2 and v3 metadata differences by having functions with names likefunction_purpose_v2andfunction_purpose_v3. I think it would be more clear to collect all the v2- and v3-specific routines into separate modules.This PR creates new
metadata.v2andmetadata.v3modules for the metadata classes, as well as ametadata.commonmodule, for some routines that have identical v2 and v3 semantics. Functions that previously signified v2-ness or v3-ness in their name now signify it from their location. In this PR The only exports fromzarr.metadataareArrayV2MetadataandArrayV3Metadata.I made a few other related changes:
in
main, we annotate themetadataattribute ofAsyncArraywith the abstract array metadata base class. I changed these annotations toArrayV2Metadata | ArrayV3Metadata, which should be much more useful. In a later effort we could consider makingAsyncArraygeneric over the type of array metadata to give even more information to the type system.In
mainv2 metadata creation relies on an import fromzarr.v2. This will not be valid after v2 is gone, so I ported thev2.normalize_fill_valueroutine intozarr.metadata.v2.parse_fill_value, and made some minor internal changes to the exceptions in that routine.TODO: