basic support for v2 and v3 groups by jhamman · Pull Request #1590 · zarr-developers/zarr-python

jhamman · 2023-12-05T20:06:48Z

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

pep8speaks · 2023-12-05T20:07:00Z

Hello @jhamman! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file zarr/v3/__init__.py:

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

jhamman

A few thoughts below 👇

jhamman · 2023-12-05T20:08:47Z

zarr/v3/group.py

    attributes: Dict[str, Any] = field(factory=dict)
-    zarr_format: Literal[3] = 3
-    node_type: Literal["group"] = "group"
+    zarr_format: Literal[2, 3] = 3  # field(default=3, validator=validators.in_([2, 3]))


@normanrz - I tried this and failed! Happy to return to it later.

jhamman · 2023-12-05T20:21:24Z

zarr/v3/abc/group.py

+
+    @abstractmethod
+    def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
+        """get child"""


Suggested change

"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

jhamman · 2023-12-05T20:22:03Z

zarr/v3/abc/group.py

+    @abstractmethod
+    def group_keys(self) -> AsyncIterator[str]:
+        """iterate over child group keys"""
+        ...


all of these iterators need listable stores before they can be built.

jhamman · 2023-12-05T20:31:28Z

zarr/v3/config.py

+@frozen
+class SyncConfiguration:
+    concurrency: Optional[int] = None
+    asyncio_loop: Optional[AbstractEventLoop] = None


this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

jhamman · 2023-12-05T20:32:54Z

zarr/v3/group.py

+    def __setitem__(self, key, value):
+        raise NotImplementedError


Suggested change

def __setitem__(self, key, value):

raise NotImplementedError

jhamman · 2023-12-05T20:35:16Z

zarr/v3/group.py

+        return self._sync(self._async_group.nchildren)
+
+    @property
+    def children(self) -> List[Array, "Group"]:


some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

normanrz · 2023-12-06T09:59:24Z

zarr/v3/abc/group.py

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

normanrz · 2023-12-06T10:07:38Z

zarr/v3/group.py

+        # TODO: consider trying to autodiscover the zarr-format here
+        if zarr_format == 3:
+            # V3 groups are comprised of a zarr.json object
+            # (it is optional in the case of implicit groups)


Are implicit groups really a thing? I don't think they show up in the v3 spec.

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

I didn't know that!

normanrz · 2023-12-06T10:08:28Z

zarr/v3/group.py

+        if self.zarr_format == 3:
+            zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
+            if zarr_json_bytes is None:
+                # implicit group?


This is a bit dangerous because we have no good way of telling whether this path even exists.

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

normanrz · 2023-12-06T10:09:42Z

zarr/v3/group.py

+                    store_path, zarr_json, runtime_configuration=self.runtime_configuration
+                )
+        elif self.zarr_format == 2:
+            # Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?


I think the overfetching is fine because it is happening in parallel.

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

zarr/v3/group.py

- removed abcs for groups/arrays - improved return types in group.py - warn (temporarily) when an implicit group is found - add attributes.py with Attributes class

d-v-b · 2023-12-06T17:00:12Z

zarr/v3/group.py

+        elif self.zarr_format == 2:
+            return {
+                ZGROUP_JSON: self.zarr_format,
+                ZATTRS_JSON: json.dumps(self.attributes).encode(),


how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

d-v-b · 2023-12-06T17:08:39Z

zarr/v3/group.py

+    def create_array(self, name: str, **kwargs) -> Array:
+        return Array(self._sync(self._async_group.create_array(name, **kwargs)))
+
+    def empty(self, **kwargs) -> Array:


thoughts on ultimately putting these routines in a dedicated namespace?

d-v-b · 2023-12-06T17:09:14Z

zarr/v3/store.py

+        try:
+            if self.store == other.store and self.path == other.path:
+                return True
+        except Exception:


we can probably be a bit more specific about the exception type here

jhamman commented Dec 5, 2023

View reviewed changes

jhamman added the V3-compat label Dec 5, 2023

normanrz reviewed Dec 6, 2023

View reviewed changes

jhamman added 3 commits December 6, 2023 16:22

basic support for v2 and v3 groups

fae9028

second rev on group apis

7f6a715

- removed abcs for groups/arrays - improved return types in group.py - warn (temporarily) when an implicit group is found - add attributes.py with Attributes class

add test file

84b497f

jhamman force-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497f Compare December 6, 2023 15:24

jhamman marked this pull request as ready for review December 6, 2023 16:52

d-v-b reviewed Dec 6, 2023

View reviewed changes

This was referenced Jan 16, 2024

Listable V3 Stores #1634

Merged

doc(v3): add v3 roadmap and design document #1583

Merged

jhamman closed this Feb 7, 2024

jhamman mentioned this pull request Mar 30, 2024

implement group.members #1726

Merged

6 tasks

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

Uh oh!

Conversation

jhamman commented Dec 5, 2023

Uh oh!

pep8speaks commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2023-12-06 15:13:06 UTC

Uh oh!

jhamman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pep8speaks commented Dec 5, 2023 •

edited

Loading