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.
add storage_transformers and get/set_partial_values #1096
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add storage_transformers and get/set_partial_values #1096
Changes from all commits
605620b
566e4b0
5f85439
3c38d57
dd7fedb
e33b365
81ebf68
ca28471
85f3309
03de894
41eaafb
5d7be76
46229ad
3a9f7cc
efa4e07
b4668a8
e4a4853
e454046
a3c7f74
c041dd8
696d5ca
c099440
be98c01
7c2767a
146c30a
59cca8b
c2dc0d6
7402262
b9d8177
91f0c2c
e68c97f
a7e4d89
fcb9ba0
b6588e7
eba9006
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight surprise to find this in store.py rather than under v3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that the base-classes go into
store.py
, similar toStoreV3
, but also happy to move this into_storage/v3.py
if that's a better fit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of the unused
array
argument here? Is it a intended that subclasses would use this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, I've seen a need for this when implementing the Sharding storage transformer in #1111:
zarr-python/zarr/_storage/v3_storage_transformers.py
Lines 99 to 102 in 6e2790c
Related to this: I'm not super happy about the double-initialization using both
__init__
and_copy_for_array
, but it seems necessary to supply the information in two steps, first from the config, then from the array. Please let me know if you have an idea how to solve this more elegantly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to wonder if this wouldn't ever need to be the transformed_chunk_store. In general, this chain of replacement stores feels slightly like an anti-pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm also not particularly happy with this pattern, but also couldn't come up with a better solution for now. Happy about any ideas 👍