-
-
Notifications
You must be signed in to change notification settings - Fork 274
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
Basic Zarr-python 2.x compatibility changes #2098
Changes from 10 commits
0c8e7f7
fa9e51d
8fdb605
0ac17cc
be2b3cd
94933b3
44ad6c7
08e7f3c
f937468
784cb28
b01e61c
c519fbe
46c2c11
1933f57
36a2ceb
1ae1cfd
9f5429f
a5ad0ca
3d04845
b710c64
0ab29d1
aa1e3bc
fb4bb85
1cc84ab
559399e
16bad38
f7f8457
56431fe
b13dee3
348aed8
2327141
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,7 +66,8 @@ test = [ | |
"flask", | ||
"requests", | ||
"mypy", | ||
"hypothesis" | ||
"hypothesis", | ||
"universal-pathlib", | ||
] | ||
|
||
jupyter = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import warnings | ||
from collections.abc import Callable | ||
from functools import wraps | ||
from inspect import Parameter, signature | ||
from typing import Any, TypeVar | ||
|
||
T = TypeVar("T") | ||
|
||
# Based off https://github.com/scikit-learn/scikit-learn/blob/e87b32a81c70abed8f2e97483758eb64df8255e9/sklearn/utils/validation.py#L63 | ||
|
||
|
||
def _deprecate_positional_args( | ||
func: Callable[..., T] | None = None, *, version: str = "1.3" | ||
) -> Callable[..., T]: | ||
"""Decorator for methods that issues warnings for positional arguments. | ||
|
||
Using the keyword-only argument syntax in pep 3102, arguments after the | ||
* will issue a warning when passed as a positional argument. | ||
|
||
Parameters | ||
---------- | ||
func : callable, default=None | ||
Function to check arguments on. | ||
version : callable, default="1.3" | ||
The version when positional arguments will result in error. | ||
""" | ||
|
||
def _inner_deprecate_positional_args(f: Callable[..., T]) -> Callable[..., T]: | ||
sig = signature(f) | ||
kwonly_args = [] | ||
all_args = [] | ||
|
||
for name, param in sig.parameters.items(): | ||
if param.kind == Parameter.POSITIONAL_OR_KEYWORD: | ||
all_args.append(name) | ||
elif param.kind == Parameter.KEYWORD_ONLY: | ||
kwonly_args.append(name) | ||
|
||
@wraps(f) | ||
def inner_f(*args: Any, **kwargs: Any) -> T: | ||
extra_args = len(args) - len(all_args) | ||
if extra_args <= 0: | ||
return f(*args, **kwargs) | ||
|
||
# extra_args > 0 | ||
args_msg = [ | ||
f"{name}={arg}" | ||
for name, arg in zip(kwonly_args[:extra_args], args[-extra_args:], strict=False) | ||
] | ||
formatted_args_msg = ", ".join(args_msg) | ||
warnings.warn( | ||
( | ||
f"Pass {formatted_args_msg} as keyword args. From version " | ||
f"{version} passing these as positional arguments " | ||
"will result in an error" | ||
), | ||
FutureWarning, | ||
stacklevel=2, | ||
) | ||
kwargs.update(zip(sig.parameters, args, strict=False)) | ||
return f(**kwargs) | ||
|
||
return inner_f | ||
|
||
if func is not None: | ||
return _inner_deprecate_positional_args(func) | ||
|
||
return _inner_deprecate_positional_args # type: ignore[return-value] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ | |
|
||
from zarr.abc.codec import Codec | ||
from zarr.abc.metadata import Metadata | ||
from zarr.abc.store import set_or_delete | ||
from zarr.abc.store import Store, set_or_delete | ||
from zarr.core.array import Array, AsyncArray | ||
from zarr.core.attributes import Attributes | ||
from zarr.core.buffer import default_buffer_prototype | ||
|
@@ -123,7 +123,7 @@ class AsyncGroup: | |
store_path: StorePath | ||
|
||
@classmethod | ||
async def create( | ||
async def from_store( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 2.x, I slightly prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this needs to change in order to avoid confusion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm also a fan of |
||
cls, | ||
store: StoreLike, | ||
*, | ||
|
@@ -309,6 +309,21 @@ def attrs(self) -> dict[str, Any]: | |
def info(self) -> None: | ||
raise NotImplementedError | ||
|
||
@property | ||
def store(self) -> Store: | ||
return self.store_path.store | ||
|
||
@property | ||
def read_only(self) -> bool: | ||
# Backwards compatibility for 2.x | ||
return self.store_path.store.mode.readonly | ||
|
||
@property | ||
def synchronizer(self) -> None: | ||
# Backwards compatibility for 2.x | ||
# Not implemented in 3.x yet. | ||
return None | ||
|
||
async def create_group( | ||
self, | ||
name: str, | ||
|
@@ -317,7 +332,7 @@ async def create_group( | |
attributes: dict[str, Any] | None = None, | ||
) -> AsyncGroup: | ||
attributes = attributes or {} | ||
return await type(self).create( | ||
return await type(self).from_store( | ||
self.store_path / name, | ||
attributes=attributes, | ||
exists_ok=exists_ok, | ||
|
@@ -588,7 +603,7 @@ class Group(SyncMixin): | |
_async_group: AsyncGroup | ||
|
||
@classmethod | ||
def create( | ||
def from_store( | ||
cls, | ||
store: StoreLike, | ||
*, | ||
|
@@ -598,7 +613,7 @@ def create( | |
) -> Group: | ||
attributes = attributes or {} | ||
obj = sync( | ||
AsyncGroup.create( | ||
AsyncGroup.from_store( | ||
store, | ||
attributes=attributes, | ||
exists_ok=exists_ok, | ||
|
@@ -678,6 +693,22 @@ def attrs(self) -> Attributes: | |
def info(self) -> None: | ||
raise NotImplementedError | ||
|
||
@property | ||
def store(self) -> Store: | ||
# Backwards compatibility for 2.x | ||
return self._async_group.store | ||
|
||
@property | ||
def read_only(self) -> bool: | ||
# Backwards compatibility for 2.x | ||
return self._async_group.read_only | ||
|
||
@property | ||
def synchronizer(self) -> None: | ||
# Backwards compatibility for 2.x | ||
# Not implemented in 3.x yet. | ||
return self._async_group.synchronizer | ||
|
||
def update_attributes(self, new_attributes: dict[str, Any]) -> Group: | ||
self._sync(self._async_group.update_attributes(new_attributes)) | ||
return self | ||
|
@@ -717,6 +748,10 @@ def tree(self, expand: bool = False, level: int | None = None) -> Any: | |
def create_group(self, name: str, **kwargs: Any) -> Group: | ||
return Group(self._sync(self._async_group.create_group(name, **kwargs))) | ||
|
||
def create(self, *args: Any, **kwargs: Any) -> Array: | ||
# Backwards compatibility for 2.x | ||
return self.create_array(*args, **kwargs) | ||
|
||
def create_array( | ||
self, | ||
name: str, | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this suggestion. Should we do it in 2.18.3 or 3.0 though?
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 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