Basic Zarr-python 2.x compatibility changes #2098
Conversation
| @classmethod | ||
| async def create( | ||
| async def from_store( |
There was a problem hiding this comment.
In 2.x, Group.create was used to create an array in the Group.
I slightly prefer from_* for alternative constructors anyway, so maybe this is a strict improvement and not just a compatibility shim?
There was a problem hiding this comment.
I agree that this needs to change in order to avoid confusion.
There was a problem hiding this comment.
i'm also a fan of from_X, and from_store seems good
| store: StoreLike | None = None, | ||
| *, |
There was a problem hiding this comment.
We can vendor and use https://github.com/scikit-learn/scikit-learn/blob/e87b32a81c70abed8f2e97483758eb64df8255e9/sklearn/utils/validation.py#L32 to deprecate positional arguments. At least of xarray, it passed just store as a positional argument, but deprecating these doesn't feel like too much work.
There was a problem hiding this comment.
I like this suggestion. Should we do it in 2.18.3 or 3.0 though?
There was a problem hiding this comment.
I don't have a strong preference here.
IMO, calendar time is more meaningful than version numbers for giving projects time to adapt to changes. So I'd say deprecate it in 3.0, as long as we don't have a hard stance on requiring keyword only for these APIs in 3.0
src/zarr/core/group.py
Outdated
| @@ -122,8 +122,12 @@ class AsyncGroup: | |||
| metadata: GroupMetadata | |||
| store_path: StorePath | |||
|
|
|||
| @property | |||
| def store(self) -> Store: | |||
There was a problem hiding this comment.
Should we deprecate this, and encourage users to do group.store_path.store?
There was a problem hiding this comment.
github is flagging this conversation as outdated, but regardless: I agree that we should just have either group.store or group.store_path.store. But I'm not a huge fan of store_path actually, so I'd actually favor deprecating that, and going back to v2 behavior where arrays / groups have a store attribute, and the store has a path attribute. But this is just my personal opinion, built from finding group.store_path.store a little weird.
src/zarr/core/group.py
Outdated
| return self._async_group.store | ||
|
|
||
| @property | ||
| def read_only(self) -> bool: |
There was a problem hiding this comment.
Same question as store.
Should probably add this to AsyncGroup too and update this implementation.
|
@TomAugspurger - this generally looks like a solid set of fixes. Do you have more here or should try to wrap these up? |
TomAugspurger
changed the title
TomAugspurger
force-pushed
the
user/tom/fix/v2-compat
branch
from
f4c82bc
to
784cb28
Compare
tests/v3/test_attributes.py
Outdated
| import zarr.store | ||
|
|
||
|
|
||
| def test_put(): |
There was a problem hiding this comment.
in principle we could test this functionality for other stores. but I also think testing the full product of [array / group features * stores] is excessive. I guess as long as we are confident that we are densely testing the core store APIs across store types, then it's fine to just test something derived from the store APIs on a single store.
There was a problem hiding this comment.
I think that in this case it's probably OK, since the test here is focused on the behavior of Attributes. This happens to require a reference to a Group (which requires a Store) but hopefully that interface is sufficiently covered by other tests.
|
I've confirmed that #2157 fixes the CI failure here. |
There was a problem hiding this comment.
Thanks @TomAugspurger for pulling these together. Much improved compatibility with v2.
Demonstration of some changes for backwards compatibility being discussed at #2097
TODO: