groundwork for V3 group tests #1743

d-v-b · 2024-04-04T20:06:26Z

Adds tests for the Group API in v3. In progress. I will remove the in progress label when it's ready for a final review, but I encourage people with an interest in the testing infrastructure to look at the early changes and weigh in if you see anything you don't like. It will be easier to course-correct before a lot of code is written.

To outline the strategy I'm using:

no classes (or at least, no inheritance)
parameterize / fixturize as much as possible
every method of the Group / AsyncGroup classes gets a test, that should parametrize over the arguments of that method

This is a deliberate departure from the strategy used in the v2 tests, where test classes are defined that inherit from a base test class, potentially overriding methods. I'm pretty sure we can use pytest fixtures to express the same functionality without needing the cognitive overhead of inheritance, but this is my personal judgment, and I'm open to other ideas here. The goal is just to have a test suite that's easy for developers to understand, which (in my experience) is not a criterion the v2 tests always meet.

~~I don't think I will fix failing tests here. That should probably done in separate PRs, once we agree on the shape of the testing infrastructure.~~ edit: amendment to this: since test failures do slow down the creation of new tests, I'm going to apply some fixes to obvious bugs revealed in the process of adding tests. These fixes should be separable in the commit history, and on the basis of the files they affect.

depends on #1726

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)

…ring issues

…and add some docstrings, and remove redundant def

…ocalStore.get_partial_values to properly handle the byte_range parameter of LocalStore.get.

d-v-b · 2024-04-05T11:48:27Z

Some of my group tests are running into bugs from the storage side. So I'm broadening the scope of this effort to include tests for the stores.

pep8speaks · 2024-04-05T13:50:09Z

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

In the file tests/v3/test_storage.py:

Line 62:29: E203 whitespace before ':'

Comment last updated at 2024-05-10 08:58:14 UTC

src/zarr/v3/group.py

jhamman

Didn't get to reviewing the tests yet but this is looking good.

jhamman · 2024-04-08T16:44:47Z

src/zarr/v3/group.py

+        # if `key` names an object in storage, it cannot be an array or group
+        if await store_path.exists():
+            raise KeyError(key)


I don't think we should include this check. This adds a roundtrip to the store to protect against a situation I have a hard time imaging being a real issue.

For one, most storage backends forbid such a situation but I don't think the spec really cares.

/store/a/b/c /store/a/b/c/zarr.json

If we don't have this check and we are on v3, then zarr_json_bytes = await (store_path / ZARR_JSON).get() will be None for any object, which will result in an implicit group. I don't think we want this. I'm actually thinking that implicit groups might be more trouble than they are worth.

In fact, getitem will never raise a KeyError for zarr v3, since any prefix is Group | Array, thanks to implicit groups.

I opened zarr-developers/zarr-specs#291 to discuss removing implicit groups from the spec. the PR is at zarr-developers/zarr-specs#292

jhamman · 2024-04-08T16:52:09Z

src/zarr/v3/group.py

+        # would be nice to make these special keys accessible programmatically,
+        # and scoped to specific zarr versions


please rework this comment to explain what the subkeys_filtered variable is.

You suggestion to move the tuple below is a good one though.

jhamman · 2024-04-08T16:52:38Z

src/zarr/v3/group.py

+        subkeys = await self.store_path.store.list_dir(self.store_path.path)
+        # would be nice to make these special keys accessible programmatically,
+        # and scoped to specific zarr versions
+        subkeys_filtered = filter(lambda v: v not in ("zarr.json", ".zgroup", ".zattrs"), subkeys)


I'm still confused here. Don't we want to be looking for .zarray to find v2 arrays?

if foo/bar/ is the prefix for a group, then we might expect to find either v2 group stuff (foo/bar/.zattrs, foo/bar/.zgroup) or v3 group stuff (foo/bar/zarr.json), but foo/bar/.zarray would be unexpected, since we know foo/bar is a group, and the same path can't correspond to both a group and an array.

jhamman · 2024-04-08T16:55:55Z

src/zarr/v3/group.py

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


I think the ideal async way to do this would be for list_dir to return an AsyncGenerator. Then your for loop below would look something like:

async for key in subkeys: # filter if key in ("zarr.json", ".zgroup", ".zattrs"): ... yield await self.getitem(subkey) ...

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

…roup_tests

…or group, largely to appease mypy

d-v-b · 2024-05-10T08:45:17Z

src/zarr/group.py

+def _parse_async_node(node: AsyncGroup) -> Group: ...
+
+
+def _parse_async_node(node: AsyncArray | AsyncGroup) -> Array | Group:


this exists as something that can be used with map over an iterable of AsyncArray / AsyncGroup to transform each one to its synchronous counterpart. Previously we used a tuple around a generator expression, but mypy couldn't type check this correctly.

d-v-b · 2024-05-10T08:46:49Z

src/zarr/group.py

-        return tuple(ret)
+        _members = self._sync_iter(self._async_group.members())
+
+        result = tuple(map(lambda kv: (kv[0], _parse_async_node(kv[1])), _members))


see https://github.com/zarr-developers/zarr-python/pull/1743/files#r1596469236

d-v-b · 2024-05-10T08:52:31Z

src/zarr/store/memory.py

@@ -88,4 +88,4 @@ async def list_dir(self, prefix: str) -> AsyncGenerator[str, None]:
        else:
            for key in self._store_dict:
                if key.startswith(prefix + "/") and key != prefix:
-                    yield key.strip(prefix + "/").split("/")[0]
+                    yield key.removeprefix(prefix + "/").split("/")[0]


this fixed a subtle bug in the list_dir method of MemoryStore. str.strip treats its argument as a set of characters to remove, not a substring / prefix. e.g.,

>>> 'a/b/a/b/b/x'.strip('a/b') 'x'

vs

>>> 'a/b/a/b/b/x'.removeprefix('a/b') '/a/b/b/x'

d-v-b · 2024-05-10T09:05:38Z

I think this is ready to go

src/zarr/array.py

tests/v2/conftest.py

…roup_tests

d-v-b · 2024-05-14T19:26:04Z

src/zarr/group.py

        )

-    async def create_array(self, path: str, **kwargs) -> AsyncArray:
+    async def create_array(


group.create_group and group.create_array should preserve the zarr format of the group, which means we can't accept **kwargs in these methods without checking the contents of **kwargs for the absence of zarr_format. Since I'm not a huge fan of **kwargs anyway, I just elevated the underlying function signatures to this method, sans zarr_format, which is set automatically based on the zarr format of the group.

We need to redo that in #1857

jhamman

Looking great @d-v-b. A few small comments for you.

jhamman · 2024-05-14T20:06:35Z

src/zarr/fixture/.zgroup

@@ -0,0 +1,3 @@
+{


I believe this was added in error

ah right, this is the zombie fixture directory that gets created by tests, see #1813

jhamman · 2024-05-14T20:14:09Z

tests/v3/test_storage.py

@@ -1,10 +1,802 @@
+# import array


maybe a bad merge conflict resolution here?

that's a good guess, honestly I have no idea what happened there. it should be fixed now.

src/zarr/group.py

d-v-b · 2024-05-15T15:26:31Z

one large, but minor (or petty) change in the latest version of this effort: I renamed test_storage.py to test_store.py, in order to be consistent with the naming: we have zarr.store, not zarr.storage.

d-v-b added 12 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

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

test: remove prematurely-added mock s3 fixture

b28eaee

chore: move v3 tests into v3 folder

0215b97

chore: type hints

e33ca6e

test: add schema for group method tests

f237172

chore: add type for zarr_formats

4605446

d-v-b added the in progress Someone is currently working on this label Apr 4, 2024

d-v-b added 9 commits April 4, 2024 22:19

chore: remove localstore for now

6e5ded9

test: add __init__.py to support imports from top-level conftest.py, …

f81b6f3

…and add some docstrings, and remove redundant def

fix: return valid JSON from GroupMetadata.to_bytes for v2 metadata

c768ca8

fix: don't use a type as a value

c405506

test: add getitem test

5b96554

fix: replace reference to nonexistent method in with , which does exist

d77d55f

test: declare v3ness via directory structure, not test file name

05f89c6

add a docstring to _get, and pass auto_mkdir to _put

b2f9bee

fix: add docstring to LocalStore.get_partial_values; adjust body of L…

88f4c46

…ocalStore.get_partial_values to properly handle the byte_range parameter of LocalStore.get.

test: add tests for localstore init, set, get, get_partial

624ff77

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

Resolve Mypy erorrs in v3 branch #1692

Merged

6 tasks

jhamman reviewed Apr 8, 2024

View reviewed changes

src/zarr/v3/group.py Outdated Show resolved Hide resolved

jhamman reviewed Apr 8, 2024

View reviewed changes

jhamman mentioned this pull request Apr 10, 2024

Feature/store list methods async generators #1782

Closed

6 tasks

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

b762fa4

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

jhamman mentioned this pull request May 7, 2024

feature(store): list_* -> AsyncGenerators #1844

Merged

d-v-b added 5 commits May 8, 2024 18:41

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

c62e686

…roup_tests

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

b1767ce

…roup_tests

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

8e61ff9

…roup_tests

fix: define utility for converting asyncarray to array, and similar f…

b996aff

…or group, largely to appease mypy

chore: remove checks that only existed because of implicit groups

923fc18

d-v-b commented May 10, 2024

View reviewed changes

d-v-b added 2 commits May 10, 2024 10:54

chore: clean up docstring and modernize some type hints

7c6cfb4

chore: move imports to top-level

17997f8

d-v-b changed the title ~~[WIP] V3 group tests~~ groundwork for V3 group tests May 10, 2024

d-v-b requested a review from jhamman May 10, 2024 09:05

d-v-b requested a review from normanrz May 10, 2024 09:05

normanrz approved these changes May 10, 2024

View reviewed changes

src/zarr/array.py Outdated Show resolved Hide resolved

tests/v2/conftest.py Show resolved Hide resolved

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

d-v-b mentioned this pull request May 10, 2024

[v3] isort or equivalent #1860

Closed

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

1140b5c

…roup_tests

d-v-b commented May 14, 2024

View reviewed changes

jhamman approved these changes May 14, 2024

View reviewed changes

d-v-b added 5 commits May 15, 2024 17:01

remove fixture files

22c0889

remove commented imports

57f564f

remove explicit asyncio marks; use __eq__ method of LocalStore for test

a821808

rename test_storage to test_store

6803f26

modern type hints

d236b24

jhamman approved these changes May 15, 2024

View reviewed changes

d-v-b merged commit f3305d9 into zarr-developers:v3 May 15, 2024
18 checks passed

d-v-b deleted the v3_group_tests branch May 15, 2024 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

groundwork for V3 group tests #1743

groundwork for V3 group tests #1743

d-v-b commented Apr 4, 2024 •

edited

Loading

d-v-b commented Apr 5, 2024

pep8speaks commented Apr 5, 2024 •

edited

Loading

jhamman left a comment

jhamman Apr 8, 2024

d-v-b Apr 12, 2024

d-v-b Apr 12, 2024

jhamman Apr 8, 2024

jhamman Apr 8, 2024

jhamman Apr 8, 2024

d-v-b Apr 12, 2024

jhamman Apr 8, 2024

d-v-b May 10, 2024

d-v-b May 10, 2024

d-v-b May 10, 2024 •

edited

Loading

d-v-b commented May 10, 2024

d-v-b May 14, 2024

normanrz May 14, 2024

jhamman left a comment

jhamman May 14, 2024

d-v-b May 14, 2024

jhamman May 14, 2024

d-v-b May 15, 2024

d-v-b commented May 15, 2024 •

edited

Loading

		# would be nice to make these special keys accessible programmatically,
		# and scoped to specific zarr versions

		def _parse_async_node(node: AsyncGroup) -> Group: ...


		def _parse_async_node(node: AsyncArray \| AsyncGroup) -> Array \| Group:

groundwork for V3 group tests #1743

groundwork for V3 group tests #1743

Conversation

d-v-b commented Apr 4, 2024 • edited Loading

d-v-b commented Apr 5, 2024

pep8speaks commented Apr 5, 2024 • edited Loading

Comment last updated at 2024-05-10 08:58:14 UTC

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

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 May 10, 2024 • edited Loading

Choose a reason for hiding this comment

d-v-b commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

d-v-b commented May 15, 2024 • edited Loading

d-v-b commented Apr 4, 2024 •

edited

Loading

pep8speaks commented Apr 5, 2024 •

edited

Loading

d-v-b May 10, 2024 •

edited

Loading

d-v-b commented May 15, 2024 •

edited

Loading