Remove implicit groups (#1827)

* wip: more group members work

* remove implicit groups

refactor Group.getitem and Group.open to better handle loading members
for v2/v3

* tidy

Author: jhamman

src/zarr/codecs/bytes.py

@@ -82,7 +82,6 @@ async def decode(
             dtype = np.dtype(f"{prefix}{chunk_spec.dtype.str[1:]}")
         else:
             dtype = np.dtype(f"|{chunk_spec.dtype.str[1:]}")
-        print(dtype)
         chunk_array = np.frombuffer(chunk_bytes, dtype)
 
         # ensure correct chunk shape

src/zarr/group.py

@@ -109,37 +109,52 @@ async def open(
         cls,
         store: StoreLike,
         runtime_configuration: RuntimeConfiguration = RuntimeConfiguration(),
-        zarr_format: Literal[2, 3] = 3,
+        zarr_format: Literal[2, 3, None] = 3,
     ) -> AsyncGroup:
         store_path = make_store_path(store)
-        zarr_json_bytes = await (store_path / ZARR_JSON).get()
-        assert zarr_json_bytes is not None
-
-        # 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)
-            zarr_json_bytes = await (store_path / ZARR_JSON).get()
-            zarr_json = (
-                json.loads(zarr_json_bytes) if zarr_json_bytes is not None else {"zarr_format": 3}
-            )
 
-        elif zarr_format == 2:
-            # V2 groups are comprised of a .zgroup and .zattrs objects
-            # (both are optional in the case of implicit groups)
+        if zarr_format == 2:
             zgroup_bytes, zattrs_bytes = await asyncio.gather(
                 (store_path / ZGROUP_JSON).get(), (store_path / ZATTRS_JSON).get()
             )
-            zgroup = (
-                json.loads(json.loads(zgroup_bytes))
-                if zgroup_bytes is not None
-                else {"zarr_format": 2}
+            if zgroup_bytes is None:
+                raise KeyError(store_path)  # filenotfounderror?
+        elif zarr_format == 3:
+            zarr_json_bytes = await (store_path / ZARR_JSON).get()
+            if zarr_json_bytes is None:
+                raise KeyError(store_path)  # filenotfounderror?
+        elif zarr_format is None:
+            zarr_json_bytes, zgroup_bytes, zattrs_bytes = await asyncio.gather(
+                (store_path / ZARR_JSON).get(),
+                (store_path / ZGROUP_JSON).get(),
+                (store_path / ZATTRS_JSON).get(),
             )
-            zattrs = json.loads(json.loads(zattrs_bytes)) if zattrs_bytes is not None else {}
-            zarr_json = {**zgroup, "attributes": zattrs}
+            if zarr_json_bytes is not None and zgroup_bytes is not None:
+                # TODO: revisit this exception type
+                # alternatively, we could warn and favor v3
+                raise ValueError("Both zarr.json and .zgroup objects exist")
+            if zarr_json_bytes is None and zgroup_bytes is None:
+                raise KeyError(store_path)  # filenotfounderror?
+            # set zarr_format based on which keys were found
+            if zarr_json_bytes is not None:
+                zarr_format = 3
+            else:
+                zarr_format = 2
         else:
             raise ValueError(f"unexpected zarr_format: {zarr_format}")
-        return cls.from_dict(store_path, zarr_json, runtime_configuration)
+
+        if zarr_format == 2:
+            # V2 groups are comprised of a .zgroup and .zattrs objects
+            assert zgroup_bytes is not None
+            zgroup = json.loads(zgroup_bytes)
+            zattrs = json.loads(zattrs_bytes) if zattrs_bytes is not None else {}
+            group_metadata = {**zgroup, "attributes": zattrs}
+        else:
+            # V3 groups are comprised of a zarr.json object
+            assert zarr_json_bytes is not None
+            group_metadata = json.loads(zarr_json_bytes)
+
+        return cls.from_dict(store_path, group_metadata, runtime_configuration)
 
     @classmethod
     def from_dict(
@@ -174,13 +189,7 @@ async def getitem(
         if self.metadata.zarr_format == 3:
             zarr_json_bytes = await (store_path / ZARR_JSON).get()
             if zarr_json_bytes is None:
-                # implicit group?
-                logger.warning("group at %s is an implicit group", store_path)
-                zarr_json = {
-                    "zarr_format": self.metadata.zarr_format,
-                    "node_type": "group",
-                    "attributes": {},
-                }
+                raise KeyError(key)
             else:
                 zarr_json = json.loads(zarr_json_bytes)
             if zarr_json["node_type"] == "group":
@@ -200,6 +209,9 @@ async def getitem(
                 (store_path / ZATTRS_JSON).get(),
             )
 
+            if zgroup_bytes is None and zarray_bytes is None:
+                raise KeyError(key)
+
             # unpack the zarray, if this is None then we must be opening a group
             zarray = json.loads(zarray_bytes) if zarray_bytes else None
             # unpack the zattrs, this can be None if no attrs were written
@@ -212,9 +224,6 @@ async def getitem(
                     store_path, zarray, runtime_configuration=self.runtime_configuration
                 )
             else:
-                if zgroup_bytes is None:
-                    # implicit group?
-                    logger.warning("group at %s is an implicit group", store_path)
                 zgroup = (
                     json.loads(zgroup_bytes)
                     if zgroup_bytes is not None
@@ -288,7 +297,12 @@ def __repr__(self):
         return f"<AsyncGroup {self.store_path}>"
 
     async def nmembers(self) -> int:
-        raise NotImplementedError
+        # TODO: consider using aioitertools.builtins.sum for this
+        # return await aioitertools.builtins.sum((1 async for _ in self.members()), start=0)
+        n = 0
+        async for _ in self.members():
+            n += 1
+        return n
 
     async def members(self) -> AsyncGenerator[tuple[str, AsyncArray | AsyncGroup], None]:
         """
@@ -321,10 +335,14 @@ async def members(self) -> AsyncGenerator[tuple[str, AsyncArray | AsyncGroup], N
                 logger.warning(
                     "Object at %s is not recognized as a component of a Zarr hierarchy.", subkey
                 )
-                pass
 
     async def contains(self, member: str) -> bool:
-        raise NotImplementedError
+        # TODO: this can be made more efficient.
+        try:
+            await self.getitem(member)
+            return True
+        except KeyError:
+            return False
 
     # todo: decide if this method should be separate from `groups`
     async def group_keys(self) -> AsyncGenerator[str, None]:
@@ -493,26 +511,18 @@ def members(self) -> tuple[tuple[str, Array | Group], ...]:
     def __contains__(self, member) -> bool:
         return self._sync(self._async_group.contains(member))
 
-    def group_keys(self) -> list[str]:
-        # uncomment with AsyncGroup implements this method
-        # return self._sync_iter(self._async_group.group_keys())
-        raise NotImplementedError
+    def group_keys(self) -> tuple[str, ...]:
+        return tuple(self._sync_iter(self._async_group.group_keys()))
 
-    def groups(self) -> list[Group]:
+    def groups(self) -> tuple[Group, ...]:
         # TODO: in v2 this was a generator that return key: Group
-        # uncomment with AsyncGroup implements this method
-        # return [Group(obj) for obj in self._sync_iter(self._async_group.groups())]
-        raise NotImplementedError
+        return tuple(Group(obj) for obj in self._sync_iter(self._async_group.groups()))
 
-    def array_keys(self) -> list[str]:
-        # uncomment with AsyncGroup implements this method
-        # return self._sync_iter(self._async_group.array_keys)
-        raise NotImplementedError
+    def array_keys(self) -> tuple[str, ...]:
+        return tuple(self._sync_iter(self._async_group.array_keys()))
 
-    def arrays(self) -> list[Array]:
-        # uncomment with AsyncGroup implements this method
-        # return [Array(obj) for obj in self._sync_iter(self._async_group.arrays)]
-        raise NotImplementedError
+    def arrays(self) -> tuple[Array, ...]:
+        return tuple(Array(obj) for obj in self._sync_iter(self._async_group.arrays()))
 
     def tree(self, expand=False, level=None) -> Any:
         return self._sync(self._async_group.tree(expand=expand, level=level))

tests/v3/test_group.py

@@ -41,17 +41,11 @@ def test_group_members(store_type, request):
 
     # add an extra object to the domain of the group.
     # the list of children should ignore this object.
-    sync(store.set(f"{path}/extra_object", b"000000"))
+    sync(store.set(f"{path}/extra_object-1", b"000000"))
     # add an extra object under a directory-like prefix in the domain of the group.
-    # this creates an implicit group called implicit_subgroup
-    sync(store.set(f"{path}/implicit_subgroup/extra_object", b"000000"))
-    # make the implicit subgroup
-    members_expected["implicit_subgroup"] = Group(
-        AsyncGroup(
-            metadata=GroupMetadata(),
-            store_path=StorePath(store=store, path=f"{path}/implicit_subgroup"),
-        )
-    )
+    # this creates a directory with a random key in it
+    # this should not show up as a member
+    sync(store.set(f"{path}/extra_directory/extra_object-2", b"000000"))
     members_observed = group.members
     # members are not guaranteed to be ordered, so sort before comparing
     assert sorted(dict(members_observed)) == sorted(members_expected)