implement `group.members` #1726

d-v-b · 2024-03-27T11:50:09Z

Implements Group.children for v3.

Tests are passing with the LocalStore. I still need to mock up a remote store for testing RemoteStore, which will be easy to do with the pre-existing v2 tests fixtures (when this is done I will remove the in progress label).

I would appreciate a critical eye on how I implemented children, because I don't have a lot of experience with async iteration in python and maybe I did something that could be easily improved on.

A few questions, that don't necessarily need answers in this PR:

Do we have to call this property children? For a variety of reasons I prefer members, and that's what I settled on over in the zom zep.
Should we guarantee a return order for children? I think natural sort order on the keys of the subarrays / subgroups is good. This requires that we get all the sub-array / subgroup keys at once, but I think that's how the storage APIs work anyway.

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)

closes #1753

…ring issues

pep8speaks · 2024-03-27T13:39:49Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-04-24 13:19:26 UTC

d-v-b · 2024-03-27T13:54:37Z

Seems like RemoteStore isn't finished yet, so it is excluded from the set of fixtures used for tests in this PR (currently, LocalStore and MemoryStore).

   def remote_store():
>       return RemoteStore()
E       TypeError: Can't instantiate abstract class RemoteStore with abstract methods get_partial_values, list, list_dir, list_prefix, set_partial_values

Implementing RemoteStore completely is out of scope for this PR, so I'm removing the in progress label.

jhamman

Thanks for taking this on @d-v-b. Left you some open-ended questions but this seems like its off to a good start.

jhamman · 2024-03-30T16:38:33Z

src/zarr/v3/group.py

+        # and scoped to specific zarr versions
+        subkeys_filtered = filter(lambda v: v not in ("zarr.json", ".zgroup", ".zattrs"), subkeys)


Suggested change

# and scoped to specific zarr versions

subkeys_filtered = filter(lambda v: v not in ("zarr.json", ".zgroup", ".zattrs"), subkeys)

# and scoped to specific zarr versions

subkeys_filtered = filter(lambda v: v not in ("zarr.json", ".zgroup", ".zarray"), subkeys)

when will .zarray be an immediate subkey of a group? By contrast, we do expect to find .zattrs under the prefix of any v2 group with attributes.

jhamman · 2024-03-30T16:46:51Z

src/zarr/v3/group.py

+            )
+
+            raise ValueError(msg)
+        subkeys = await self.store_path.store.list_dir(self.store_path.path)


Following #1686 (comment), this method should also return an async generator. Just calling that out as a change that will come here soon, what you've done is fine for now.

What I would love to see is something like:

async for subkey in self.store_path.store.list_regex(self.store_path.path, pattern=pattern): yield await self.getitem(subkey)

jhamman · 2024-03-30T16:49:03Z

src/zarr/v3/group.py

+            except KeyError:
+                # keyerror is raised when `subkey``names an object in the store
+                # in which case `subkey` cannot be the name of a sub-array or sub-group.
+                pass


I might be missing something here but if subkey was just found in the store, why would we be getting a KeyError? This pass seems like it could silently ignore something that we care about.

If "foo" is the name of a random non-zarr-related file in the store, group.getitem("foo") raises KeyError, because "foo" is not the name of a sub-array or sub-group. So in this case, a KeyError just means there's a random file in there, that we want to ignore. I'm not sure that there are sources of KeyError (or any other exception) that we would want to handle here?

tests/conftest.py

src/zarr/v3/group.py

d-v-b · 2024-04-04T10:46:42Z

Should we guarantee a return order for children? I think natural sort order on the keys of the subarrays / subgroups is good. This requires that we get all the sub-array / subgroup keys at once, but I think that's how the storage APIs work anyway.

I think this is answered (in the negative) by the need to support asynchronous iteration over keys, and the fact that cloud backends (e.g., s3) will reasonably paginate requests for lists of keys.

…(name, AsyncArray / AsyncGroup) pairs; Group.members repackages these into a dict.

d-v-b · 2024-04-11T09:54:13Z

the name of this method was discussed in an offline discussion in the biweekly refactoring meeting, and the conclusion was that we will switch the name from children to members. The v2 codebase uses both terms internally, but only the term members is used in the Group API. The terms "member" or "members" appears 13 times there, while "children" or "child" are not used.

…p_children_fix

…ed to deal with

d-v-b · 2024-04-12T11:31:28Z

src/zarr/v3/sync.py

-            async_iterator = await coroutine
-            return [item async for item in async_iterator]
+            # async_iterator = await coroutine
+            return [item async for item in coroutine()]


@DahnJ I had to revert some of your changes from #1692 here in order to get the code to run. The type signatures are currently incorrect, but the code works, so we should either adjust the annotations to match the actual types, or adjust the implementation (and the actual types) to match the annotation.

@jhamman, I know you have some ideas about the direction for this

d-v-b · 2024-04-12T11:46:23Z

a design question: in the latest changes to this PR, Group.members returns a tuple of (str, Group | Array) pairs. This is a departure from returning dict[str, Group | Array], but it facilitates streaming individual members instead of prematurely collecting them. I am wondering if "members returns a dict" vs "members returns a tuple of tuples" is a false dichotomy -- can we just do both, by making Group.members a MutableMapping? (this relates to the discussion over in #1787)

…nto group_children_fix

…p_children_fix

jhamman

Let's get this in! It unlocks a bunch of work on the hierarchy. I left a few comments around future optimizations / directions but this is a good first step.

jhamman · 2024-04-24T15:24:30Z

src/zarr/group.py

+    async def group_keys(self) -> AsyncGenerator[str, None]:
+        async for key, value in self.members():
+            if isinstance(value, AsyncGroup):
+                yield key


A future optimization here would be to avoid constructing the AsyncGroup and instead just generate the key based on filtering the metadata doc. As it is currently written, we construct all the members as AsyncGroup | AsyncArray, only to return their name.

Additionally, we could allow Group.members to filter to only='groups' so that we never need construct arrays in these cases.

yes, i think checking the metadata should be much cheaper than instantiating AsyncArray / AsyncGroup. I will have to think about what those changes would mean for the AsyncGroup.members API, since that's what group_keys relies on.

jhamman · 2024-04-24T15:27:40Z

src/zarr/group.py

-        #     self._async_group.children()
-        # )
-        # return [Array(obj) if isinstance(obj, AsyncArray) else Group(obj) for obj in _children]
+    def members(self) -> tuple[tuple[str, Array | Group], ...]:


Let's continue to think about the return type here. I could see a (frozen) dict being a more ergonomic return type.

I'm also a fan of something immutable and dict-like here. I think we can use #1787 to continue brainstorming

Xarray has a FrozenDict type which we could borrow: https://github.com/pydata/xarray/blob/b0036749542145794244dee4c4869f3750ff2dee/xarray/core/utils.py#L415-L443

d-v-b added 4 commits March 26, 2024 21:04

feat: functional .children method for groups

e492be2

changes necessary for correctly generating list of children

b7f66c7

add stand-alone test for group.children

c7b333a

give type hints a glow-up

a64b342

d-v-b requested a review from jhamman March 27, 2024 11:50

d-v-b added in progress Someone is currently working on this V3 Related to compatibility with V3 spec labels Mar 27, 2024

d-v-b added 2 commits March 27, 2024 14:03

test: use separate assert statements to avoid platform-dependent orde…

3d11fc0

…ring issues

test: put fixtures in conftest, add MemoryStore fixture

cf34afc

docs: release notes

16cb226

d-v-b removed the in progress Someone is currently working on this label Mar 27, 2024

jhamman reviewed Mar 30, 2024

View reviewed changes

test: remove prematurely-added mock s3 fixture

b28eaee

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

groundwork for V3 group tests #1743

Open

6 tasks

jhamman mentioned this pull request Apr 5, 2024

[v3] Implement group members / children #1753

Open

fix: Rename children to members; AsyncGroup.members yields tuples of …

b762fa4

…(name, AsyncArray / AsyncGroup) pairs; Group.members repackages these into a dict.

d-v-b changed the title ~~implement group.children~~ implement ---group.children--- group.members Apr 11, 2024

d-v-b changed the title ~~implement ---group.children--- group.members~~ implement group.members Apr 11, 2024

d-v-b added 3 commits April 11, 2024 13:20

fix: make Group.members return a tuple of str, Array | Group pairs

5574226

Merge branch 'v3' of github.com:zarr-developers/zarr-python into grou…

bcd5c7d

…p_children_fix

fix: revert changes to synchronization code; this is churn that we ne…

d634cbf

…ed to deal with

d-v-b commented Apr 12, 2024

View reviewed changes

d-v-b added 2 commits April 12, 2024 13:46

Merge branch 'v3' into group_children_fix

9b9d146

Merge branch 'v3' into group_children_fix

cc70b26

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

jhamman and others added 6 commits April 22, 2024 05:41

Merge branch 'v3' of https://github.com/zarr-developers/zarr-python i…

d5f546e

…nto group_children_fix

make mypy happy

66f71cc

feat: implement member-specific iteration methods in asyncgroup

ad55c3f

Merge branch 'v3' of github.com:zarr-developers/zarr-python into grou…

a029e58

…p_children_fix

chore: clean up some post-merge issues

7637624

chore: remove extra directory added by test code

0827648

d-v-b requested a review from jhamman April 24, 2024 15:15

jhamman approved these changes Apr 24, 2024

View reviewed changes

d-v-b merged commit 57d6ace into zarr-developers:v3 Apr 24, 2024
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `group.members` #1726

implement `group.members` #1726

d-v-b commented Mar 27, 2024 •

edited by jhamman

pep8speaks commented Mar 27, 2024 •

edited

d-v-b commented Mar 27, 2024

jhamman left a comment

jhamman Mar 30, 2024

d-v-b Apr 4, 2024

jhamman Mar 30, 2024

jhamman Mar 30, 2024

d-v-b Mar 30, 2024

d-v-b commented Apr 4, 2024

d-v-b commented Apr 11, 2024

d-v-b Apr 12, 2024

d-v-b commented Apr 12, 2024

jhamman left a comment

jhamman Apr 24, 2024

jhamman Apr 24, 2024

d-v-b Apr 24, 2024

jhamman Apr 24, 2024

d-v-b Apr 24, 2024

jhamman Apr 24, 2024

		# and scoped to specific zarr versions
		subkeys_filtered = filter(lambda v: v not in ("zarr.json", ".zgroup", ".zattrs"), subkeys)

implement group.members #1726

implement group.members #1726

Conversation

d-v-b commented Mar 27, 2024 • edited by jhamman

pep8speaks commented Mar 27, 2024 • edited

Comment last updated at 2024-04-24 13:19:26 UTC

d-v-b commented Mar 27, 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

Choose a reason for hiding this comment

d-v-b commented Apr 4, 2024

d-v-b commented Apr 11, 2024

Choose a reason for hiding this comment

d-v-b commented Apr 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

implement `group.members` #1726

implement `group.members` #1726

d-v-b commented Mar 27, 2024 •

edited by jhamman

pep8speaks commented Mar 27, 2024 •

edited