Merge branch 'main' into fix-zarr-load

Author: brokkoli71

changes/3127.bugfix.rst

@@ -0,0 +1,2 @@
+When `zarr.save` has an argument `path=some/path/` and multiple arrays in `args`, the path resulted in `some/path/some/path` due to using the `path`
+argument twice while building the array path. This is now fixed.

docs/conf.py

@@ -106,7 +106,6 @@ def skip_submodules(
      "installation": "user-guide/installation.html",
      "api": "api/zarr/index",
      "release": "release-notes.html",
-     "release-notes": "release-notes.html"
 }
 
 # The language for content autogenerated by Sphinx. Refer to documentation

docs/user-guide/consolidated_metadata.rst

@@ -114,3 +114,23 @@ removed, or modified, consolidated metadata may not be desirable.
    metadata.
 
 .. _Consolidated Metadata: https://github.com/zarr-developers/zarr-specs/pull/309
+
+Stores Without Support for Consolidated Metadata
+------------------------------------------------
+
+Some stores may want to opt out of the consolidated metadata mechanism. This
+may be for several reasons like:
+
+* They want to maintain read-write consistency, which is challenging with
+  consolidated metadata.
+* They have their own consolidated metadata mechanism.
+* They offer good enough performance without need for consolidation.
+
+This type of store can declare it doesn't want consolidation by implementing
+`Store.supports_consolidated_metadata` and returning `False`. For stores that don't support
+consolidation, Zarr will:
+
+* Raise an error on `consolidate_metadata` calls, maintaining the store in
+  its unconsolidated state.
+* Raise an error in `AsyncGroup.open(..., use_consolidated=True)`
+* Not use consolidated metadata in `AsyncGroup.open(..., use_consolidated=None)`

src/zarr/abc/store.py

@@ -264,6 +264,18 @@ async def _set_many(self, values: Iterable[tuple[str, Buffer]]) -> None:
         """
         await gather(*starmap(self.set, values))
 
+    @property
+    def supports_consolidated_metadata(self) -> bool:
+        """
+        Does the store support consolidated metadata?.
+
+        If it doesn't an error will be raised on requests to consolidate the metadata.
+        Returning `False` can be useful for stores which implement their own
+        consolidation mechanism outside of the zarr-python implementation.
+        """
+
+        return True
+
     @property
     @abstractmethod
     def supports_deletes(self) -> bool:

src/zarr/api/asynchronous.py

@@ -176,7 +176,8 @@ async def consolidate_metadata(
     Consolidate the metadata of all nodes in a hierarchy.
 
     Upon completion, the metadata of the root node in the Zarr hierarchy will be
-    updated to include all the metadata of child nodes.
+    updated to include all the metadata of child nodes. For Stores that do
+    not support consolidated metadata, this operation raises a ``TypeError``.
 
     Parameters
     ----------
@@ -196,10 +197,18 @@ async def consolidate_metadata(
     -------
     group: AsyncGroup
         The group, with the ``consolidated_metadata`` field set to include
-        the metadata of each child node.
+        the metadata of each child node. If the Store doesn't support
+        consolidated metadata, this function raises a `TypeError`.
+        See ``Store.supports_consolidated_metadata``.
     """
     store_path = await make_store_path(store, path=path)
 
+    if not store_path.store.supports_consolidated_metadata:
+        store_name = type(store_path.store).__name__
+        raise TypeError(
+            f"The Zarr Store in use ({store_name}) doesn't support consolidated metadata",
+        )
+
     group = await AsyncGroup.open(store_path, zarr_format=zarr_format, use_consolidated=False)
     group.store_path.store._check_writable()
 
@@ -512,13 +521,12 @@ async def save_group(
         raise ValueError("at least one array must be provided")
     aws = []
     for i, arr in enumerate(args):
-        _path = f"{path}/arr_{i}" if path is not None else f"arr_{i}"
         aws.append(
             save_array(
                 store_path,
                 arr,
                 zarr_format=zarr_format,
-                path=_path,
+                path=f"arr_{i}",
                 storage_options=storage_options,
             )
         )

src/zarr/api/synchronous.py

@@ -81,7 +81,8 @@ def consolidate_metadata(
     Consolidate the metadata of all nodes in a hierarchy.
 
     Upon completion, the metadata of the root node in the Zarr hierarchy will be
-    updated to include all the metadata of child nodes.
+    updated to include all the metadata of child nodes. For Stores that do
+    not use consolidated metadata, this operation raises a `TypeError`.
 
     Parameters
     ----------
@@ -101,7 +102,10 @@ def consolidate_metadata(
     -------
     group: Group
         The group, with the ``consolidated_metadata`` field set to include
-        the metadata of each child node.
+        the metadata of each child node. If the Store doesn't support
+        consolidated metadata, this function raises a `TypeError`.
+        See ``Store.supports_consolidated_metadata``.
+
     """
     return Group(sync(async_api.consolidate_metadata(store, path=path, zarr_format=zarr_format)))
 

src/zarr/core/group.py

@@ -490,10 +490,11 @@ async def open(
 
             By default, consolidated metadata is used if it's present in the
             store (in the ``zarr.json`` for Zarr format 3 and in the ``.zmetadata`` file
-            for Zarr format 2).
+            for Zarr format 2) and the Store supports it.
 
-            To explicitly require consolidated metadata, set ``use_consolidated=True``,
-            which will raise an exception if consolidated metadata is not found.
+            To explicitly require consolidated metadata, set ``use_consolidated=True``.
+            In this case, if the Store doesn't support consolidation or consolidated metadata is
+            not found, a ``ValueError`` exception is raised.
 
             To explicitly *not* use consolidated metadata, set ``use_consolidated=False``,
             which will fall back to using the regular, non consolidated metadata.
@@ -503,6 +504,16 @@ async def open(
             to load consolidated metadata from a non-default key.
         """
         store_path = await make_store_path(store)
+        if not store_path.store.supports_consolidated_metadata:
+            # Fail if consolidated metadata was requested but the Store doesn't support it
+            if use_consolidated:
+                store_name = type(store_path.store).__name__
+                raise ValueError(
+                    f"The Zarr store in use ({store_name}) doesn't support consolidated metadata."
+                )
+
+            # if use_consolidated was None (optional), the Store dictates it doesn't want consolidation
+            use_consolidated = False
 
         consolidated_key = ZMETADATA_V2_JSON
 

tests/test_api.py

@@ -229,22 +229,23 @@ async def test_open_group_unspecified_version(
 @pytest.mark.parametrize("store", ["local", "memory", "zip"], indirect=["store"])
 @pytest.mark.parametrize("n_args", [10, 1, 0])
 @pytest.mark.parametrize("n_kwargs", [10, 1, 0])
-def test_save(store: Store, n_args: int, n_kwargs: int) -> None:
+@pytest.mark.parametrize("path", [None, "some_path"])
+def test_save(store: Store, n_args: int, n_kwargs: int, path: None | str) -> None:
     data = np.arange(10)
     args = [np.arange(10) for _ in range(n_args)]
     kwargs = {f"arg_{i}": data for i in range(n_kwargs)}
 
     if n_kwargs == 0 and n_args == 0:
         with pytest.raises(ValueError):
-            save(store)
+            save(store, path=path)
     elif n_args == 1 and n_kwargs == 0:
-        save(store, *args)
-        array = zarr.api.synchronous.open(store)
+        save(store, *args, path=path)
+        array = zarr.api.synchronous.open(store, path=path)
         assert isinstance(array, Array)
         assert_array_equal(array[:], data)
     else:
-        save(store, *args, **kwargs)  # type: ignore [arg-type]
-        group = zarr.api.synchronous.open(store)
+        save(store, *args, path=path, **kwargs)  # type: ignore [arg-type]
+        group = zarr.api.synchronous.open(store, path=path)
         assert isinstance(group, Group)
         for array in group.array_values():
             assert_array_equal(array[:], data)
@@ -384,8 +385,8 @@ def test_array_order_warns(order: MemoryOrder | None, zarr_format: ZarrFormat) -
 #     assert "LazyLoader: " in repr(loader)
 
 
-def test_load_array(memory_store: Store) -> None:
-    store = memory_store
+def test_load_array(sync_store: Store) -> None:
+    store = sync_store
     foo = np.arange(100)
     bar = np.arange(100, 0, -1)
     save(store, foo=foo, bar=bar)

tests/test_metadata/test_consolidated.py

@@ -651,3 +651,38 @@ async def test_consolidated_metadata_encodes_special_chars(
     elif zarr_format == 3:
         assert root_metadata["child"]["attributes"]["test"] == expected_fill_value
         assert root_metadata["time"]["fill_value"] == expected_fill_value
+
+
+class NonConsolidatedStore(zarr.storage.MemoryStore):
+    """A store that doesn't support consolidated metadata"""
+
+    @property
+    def supports_consolidated_metadata(self) -> bool:
+        return False
+
+
+async def test_consolidate_metadata_raises_for_self_consolidating_stores():
+    """Verify calling consolidate_metadata on a non supporting stores raises an error."""
+
+    memory_store = NonConsolidatedStore()
+    root = await zarr.api.asynchronous.create_group(store=memory_store)
+    await root.create_group("a/b")
+
+    with pytest.raises(TypeError, match="doesn't support consolidated metadata"):
+        await zarr.api.asynchronous.consolidate_metadata(memory_store)
+
+
+async def test_open_group_in_non_consolidating_stores():
+    memory_store = NonConsolidatedStore()
+    root = await zarr.api.asynchronous.create_group(store=memory_store)
+    await root.create_group("a/b")
+
+    # Opening a group without consolidatedion works as expected
+    await AsyncGroup.open(memory_store, use_consolidated=False)
+
+    # let the Store opt out of consolidation
+    await AsyncGroup.open(memory_store, use_consolidated=None)
+
+    # Opening a group with use_consolidated=True should fail
+    with pytest.raises(ValueError, match="doesn't support consolidated metadata"):
+        await AsyncGroup.open(memory_store, use_consolidated=True)