Resolve Mypy erorrs in `v3` branch #1692

DahnJ · 2024-03-04T18:54:45Z

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

DahnJ · 2024-03-04T18:57:26Z

src/zarr/v3/store/core.py

@@ -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


Not solved here, as per comment. What subclass of Store should this use?

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

DahnJ · 2024-03-04T18:57:49Z

src/zarr/v3/store/core.py

@@ -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


Similar here

DahnJ · 2024-03-04T18:58:33Z

src/zarr/v3/sync.py

-    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]:


I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

joshmoore · 2024-03-05T09:42:40Z

Thanks, @DahnJ. I've launched the workflows.

DahnJ · 2024-03-05T10:13:24Z

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

joshmoore · 2024-03-05T10:29:04Z

On their way! 🏇🏽

jhamman

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.

jhamman · 2024-03-10T21:05:43Z

src/zarr/v3/group.py

@@ -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(),


Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change

ZGROUP_JSON: str(self.zarr_format).encode(),

ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Addressed in a30af88

jhamman · 2024-03-10T21:14:30Z

src/zarr/v3/store/core.py

@@ -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


Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

jhamman · 2024-03-10T21:14:38Z

src/zarr/v3/store/core.py

@@ -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


DahnJ · 2024-03-19T22:05:26Z

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the second Store.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

jhamman · 2024-04-06T04:50:06Z

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

d-v-b · 2024-04-06T08:48:08Z

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

d-v-b · 2024-04-06T08:48:22Z

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>

DahnJ added 4 commits March 4, 2024 18:39

refactor(v3): Using appropriate types

c0452be

fix(v3): Typing fixes + minor code fixes

d541a2b

fix(v3): _sync_iter works with coroutines

94972d6

docs(v3/store/core.py): clearer comment

d3379b4

DahnJ commented Mar 4, 2024

View reviewed changes

DahnJ mentioned this pull request Mar 4, 2024

Resolve Mypy errors in v3 branch #1593

Open

2 tasks

DahnJ commented Mar 4, 2024

View reviewed changes

fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic

f2635e3

jhamman reviewed Mar 10, 2024

View reviewed changes

DahnJ added 4 commits March 19, 2024 22:43

fix(zarr/v3): correct zarr format + remove unused method

a30af88

fix(v3/store/core.py): Potential suggestion on handling str store_like

f57527a

refactor(zarr/v3): Add more typing

e1fdd1e

ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit

db6da34

MSanKeys963 added the V3 Related to compatibility with V3 spec label Mar 28, 2024

jhamman requested a review from d-v-b April 6, 2024 04:41

jhamman added this to the 3.0.0.alpha milestone Apr 6, 2024

d-v-b approved these changes Apr 6, 2024

View reviewed changes

d-v-b merged commit 15a9747 into zarr-developers:v3 Apr 6, 2024
7 checks passed

d-v-b mentioned this pull request Apr 12, 2024

implement group.members #1726

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve Mypy erorrs in `v3` branch #1692

Resolve Mypy erorrs in `v3` branch #1692

DahnJ commented Mar 4, 2024 •

edited

DahnJ Mar 4, 2024

jhamman Mar 10, 2024

DahnJ Mar 4, 2024

jhamman Mar 10, 2024

DahnJ Mar 4, 2024

joshmoore commented Mar 5, 2024

DahnJ commented Mar 5, 2024

joshmoore commented Mar 5, 2024

jhamman left a comment

jhamman Mar 10, 2024

DahnJ Mar 19, 2024

jhamman Mar 10, 2024

jhamman Mar 10, 2024

DahnJ commented Mar 19, 2024

jhamman commented Apr 6, 2024

d-v-b commented Apr 6, 2024

d-v-b commented Apr 6, 2024

	ZGROUP_JSON: str(self.zarr_format).encode(),
	ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Resolve Mypy erorrs in v3 branch #1692

Resolve Mypy erorrs in v3 branch #1692

Conversation

DahnJ commented Mar 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore commented Mar 5, 2024

DahnJ commented Mar 5, 2024

joshmoore commented Mar 5, 2024

jhamman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DahnJ commented Mar 19, 2024

jhamman commented Apr 6, 2024

d-v-b commented Apr 6, 2024

d-v-b commented Apr 6, 2024

Resolve Mypy erorrs in `v3` branch #1692

Resolve Mypy erorrs in `v3` branch #1692

DahnJ commented Mar 4, 2024 •

edited