-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
Resolve Mypy erorrs in v3
branch
#1692
Conversation
src/zarr/v3/store/core.py
Outdated
@@ -26,7 +26,8 @@ def __init__(self, store: Store, path: Optional[str] = None): | |||
|
|||
@classmethod | |||
def from_path(cls, pth: Path) -> StorePath: | |||
return cls(Store.from_path(pth)) | |||
# NOT SOLVED: This is instantiating an ABC + there is no from_path method |
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.
Not solved here, as per comment. What subclass of Store
should this use?
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.
Good catch. I don't think we're using StorePath.from_path()
anymore. If so, I'd be comfortable removing it here.
src/zarr/v3/store/core.py
Outdated
@@ -76,7 +77,8 @@ def make_store_path(store_like: StoreLike) -> StorePath: | |||
try: | |||
from upath import UPath | |||
|
|||
return StorePath(Store.from_path(UPath(store_like))) | |||
# NOT SOLVED: Similar here, ABC instantiation + no from_path method |
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.
Similar here
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.
See above.
def _sync_iter( | ||
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs | ||
) -> List[T]: | ||
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]: |
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 removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.
Thanks, @DahnJ. I've launched the workflows. |
Thanks @joshmoore I tried to fix a failing test (which I think happened because |
On their way! 🏇🏽 |
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.
Thanks @DahnJ for jumping in here! Really happy to see this contribution.
Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.
src/zarr/v3/group.py
Outdated
@@ -46,11 +46,11 @@ def to_bytes(self) -> Dict[str, bytes]: | |||
return {ZARR_JSON: json.dumps(self.to_dict()).encode()} | |||
else: | |||
return { | |||
ZGROUP_JSON: self.zarr_format, | |||
ZGROUP_JSON: str(self.zarr_format).encode(), |
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.
Flagging that this was actually a bug in hiding. I believe it should have been something like:
ZGROUP_JSON: str(self.zarr_format).encode(), | |
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(), |
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.
Addressed in a30af88
src/zarr/v3/store/core.py
Outdated
@@ -26,7 +26,8 @@ def __init__(self, store: Store, path: Optional[str] = None): | |||
|
|||
@classmethod | |||
def from_path(cls, pth: Path) -> StorePath: | |||
return cls(Store.from_path(pth)) | |||
# NOT SOLVED: This is instantiating an ABC + there is no from_path method |
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.
Good catch. I don't think we're using StorePath.from_path()
anymore. If so, I'd be comfortable removing it here.
src/zarr/v3/store/core.py
Outdated
@@ -76,7 +77,8 @@ def make_store_path(store_like: StoreLike) -> StorePath: | |||
try: | |||
from upath import UPath | |||
|
|||
return StorePath(Store.from_path(UPath(store_like))) | |||
# NOT SOLVED: Similar here, ABC instantiation + no from_path method |
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.
See above.
@jhamman I have addressed the comments in a30af88 I was not sure how to handle the second As for getting mypy to pass, it does pass for me locally and I didn't add any |
thanks @DahnJ ! |
* refactor(v3): Using appropriate types * fix(v3): Typing fixes + minor code fixes * fix(v3): _sync_iter works with coroutines * docs(v3/store/core.py): clearer comment * fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic * fix(zarr/v3): correct zarr format + remove unused method * fix(v3/store/core.py): Potential suggestion on handling str store_like * refactor(zarr/v3): Add more typing * ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* chore: add deprecation warnings to v3 classes / functions * Resolve Mypy erorrs in `v3` branch (#1692) * refactor(v3): Using appropriate types * fix(v3): Typing fixes + minor code fixes * fix(v3): _sync_iter works with coroutines * docs(v3/store/core.py): clearer comment * fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic * fix(zarr/v3): correct zarr format + remove unused method * fix(v3/store/core.py): Potential suggestion on handling str store_like * refactor(zarr/v3): Add more typing * ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit * Specify hatch envs using GitHub actions matrix for v3 tests (#1728) * Specify v3 hatch envs using GitHub actions matrix * Update .github/workflows/test-v3.yml Co-authored-by: Joe Hamman <jhamman1@gmail.com> * Update .github/workflows/test-v3.yml Co-authored-by: Joe Hamman <jhamman1@gmail.com> * test on 3.12 too * no 3.12 --------- Co-authored-by: Joe Hamman <jhamman1@gmail.com> Co-authored-by: Joe Hamman <joe@earthmover.io> * black -> ruff format + cleanup (#1639) * black -> ruff + cleanup * format * Preserve git blame * pre-commit fix * Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643) Co-authored-by: Joe Hamman <joe@earthmover.io> * chore: remove old v3 implementation * chore: remove more version-conditional logic * chore: remove v3_storage_transformers.py again --------- Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com> Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com> Co-authored-by: Joe Hamman <jhamman1@gmail.com> Co-authored-by: Joe Hamman <joe@earthmover.io> Co-authored-by: Saransh Chopra <saransh0701@gmail.com> Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Attempts to address #1593
There are still two unsolved issues around the nonexistant
Store.from_path
incore.py
.TODO: