merge upstream changes

Author: K-Meech

changes/3288.misc.rst

@@ -0,0 +1 @@
+Sort dictionary keys before returning consolidated metadata to ensure deterministic output.

docs/user-guide/consolidated_metadata.rst

@@ -45,7 +45,7 @@ that can be used.:
    >>> consolidated = zarr.open_group(store=store)
    >>> consolidated_metadata = consolidated.metadata.consolidated_metadata.metadata
    >>> from pprint import pprint
-   >>> pprint(dict(sorted(consolidated_metadata.items())))
+   >>> pprint(dict(consolidated_metadata.items()))
    {'a': ArrayV3Metadata(shape=(1,),
                           data_type=Float64(endianness='little'),
                           chunk_grid=RegularChunkGrid(chunk_shape=(1,)),
@@ -100,6 +100,14 @@ With nested groups, the consolidated metadata is available on the children, recu
    >>> consolidated['child'].metadata.consolidated_metadata
    ConsolidatedMetadata(metadata={'child': GroupMetadata(attributes={'kind': 'grandchild'}, zarr_format=3, consolidated_metadata=ConsolidatedMetadata(metadata={}, kind='inline', must_understand=False), node_type='group')}, kind='inline', must_understand=False)
 
+.. versionadded:: 3.1.1
+
+    The keys in the consolidated metadata are sorted prior to writing. Keys are
+    sorted in ascending order by path depth, where a path is defined as a sequence
+    of strings joined by ``"/"``. For keys with the same path length, lexicographic
+    order is used to break the tie.  This behaviour ensures deterministic metadata
+    output for a given group.
+
 Synchronization and Concurrency
 -------------------------------
 

src/zarr/core/group.py

@@ -4,6 +4,7 @@
 import itertools
 import json
 import logging
+import unicodedata
 import warnings
 from collections import defaultdict
 from dataclasses import asdict, dataclass, field, fields, replace
@@ -141,7 +142,16 @@ def to_dict(self) -> dict[str, JSON]:
         return {
             "kind": self.kind,
             "must_understand": self.must_understand,
-            "metadata": {k: v.to_dict() for k, v in self.flattened_metadata.items()},
+            "metadata": {
+                k: v.to_dict()
+                for k, v in sorted(
+                    self.flattened_metadata.items(),
+                    key=lambda item: (
+                        item[0].count("/"),
+                        unicodedata.normalize("NFKC", item[0]).casefold(),
+                    ),
+                )
+            },
         }
 
     @classmethod

src/zarr/storage/_common.py

@@ -305,54 +305,60 @@ async def make_store(
     ------
     TypeError
         If the StoreLike object is not one of the supported types, or if storage_options is provided but not used.
-    ValueError
-        If storage_options is provided for a store that does not support it.
     """
     from zarr.storage._fsspec import FsspecStore  # circular import
 
-    used_storage_options = False
-    assert mode in (None, "r", "r+", "a", "w", "w-")
+    if (
+        not (isinstance(store_like, str) and _is_fsspec_uri(store_like))
+        and storage_options is not None
+    ):
+        raise TypeError(
+            "'storage_options' was provided but unused. "
+            "'storage_options' is only used when the store is passed as a FSSpec URI string.",
+        )
 
-    # if mode 'r' was provided, we'll open any new stores as read-only
+    assert mode in (None, "r", "r+", "a", "w", "w-")
     _read_only = mode == "r"
 
     if isinstance(store_like, StorePath):
-        store = store_like.store
+        # Already a StorePath
+        return store_like.store
+
     elif isinstance(store_like, Store):
-        store = store_like
+        # Already a Store
+        return store_like
+
+    elif isinstance(store_like, dict):
+        # Already a dictionary that can be a MemoryStore
+        #
+        # We deliberate only consider dict[str, Buffer] here, and not arbitrary mutable mappings.
+        # By only allowing dictionaries, which are in-memory, we know that MemoryStore appropriate.
+        return await MemoryStore.open(store_dict=store_like, read_only=_read_only)
+
     elif store_like is None:
-        store = await MemoryStore.open(read_only=_read_only)
+        # Create a new in-memory store
+        return await make_store({}, mode=mode, storage_options=storage_options)
+
     elif isinstance(store_like, Path):
-        store = await LocalStore.open(root=store_like, read_only=_read_only)
-    elif isinstance(store_like, str):
-        storage_options = storage_options or {}
+        # Create a new LocalStore
+        return await LocalStore.open(root=store_like, read_only=_read_only)
 
+    elif isinstance(store_like, str):
+        # Either a FSSpec URI or a local filesystem path
         if _is_fsspec_uri(store_like):
-            used_storage_options = True
-            store = FsspecStore.from_url(
+            return FsspecStore.from_url(
                 store_like, storage_options=storage_options, read_only=_read_only
             )
         else:
-            store = await LocalStore.open(root=Path(store_like), read_only=_read_only)
-    elif isinstance(store_like, dict):
-        # We deliberate only consider dict[str, Buffer] here, and not arbitrary mutable mappings.
-        # By only allowing dictionaries, which are in-memory, we know that MemoryStore appropriate.
-        store = await MemoryStore.open(store_dict=store_like, read_only=_read_only)
+            # Assume a filesystem path
+            return await make_store(Path(store_like), mode=mode, storage_options=storage_options)
+
     elif _has_fsspec and isinstance(store_like, FSMap):
-        if storage_options:
-            raise ValueError(
-                "'storage_options was provided but is not used for FSMap store_like objects. Specify the storage options when creating the FSMap instance instead."
-            )
-        store = FsspecStore.from_mapper(store_like, read_only=_read_only)
+        return FsspecStore.from_mapper(store_like, read_only=_read_only)
+
     else:
         raise TypeError(f"Unsupported type for store_like: '{type(store_like).__name__}'")
 
-    if storage_options and not used_storage_options:
-        msg = "'storage_options' was provided but unused. 'storage_options' is only used for fsspec filesystem stores."
-        raise TypeError(msg)
-
-    return store
-
 
 async def make_store_path(
     store_like: StoreLike | None,
@@ -391,7 +397,7 @@ async def make_store_path(
     TypeError
         If the StoreLike object is not one of the supported types, or if storage_options is provided but not used.
     ValueError
-        If storage_options is provided for a store that does not support it.
+        If path is provided for a store that does not support it.
 
     See Also
     --------
@@ -400,14 +406,19 @@ async def make_store_path(
     path_normalized = normalize_path(path)
 
     if isinstance(store_like, StorePath):
+        # Already a StorePath
         if storage_options:
-            msg = "'storage_options' was provided but unused. 'storage_options' is only used for fsspec filesystem stores."
-            raise TypeError(msg)
+            raise TypeError(
+                "'storage_options' was provided but unused. "
+                "'storage_options' is only used when the store is passed as a FSSpec URI string.",
+            )
         return store_like / path_normalized
+
     elif _has_fsspec and isinstance(store_like, FSMap) and path:
         raise ValueError(
             "'path' was provided but is not used for FSMap store_like objects. Specify the path when creating the FSMap instance instead."
         )
+
     else:
         store = await make_store(store_like, mode=mode, storage_options=storage_options)
         return await StorePath.open(store, path=path_normalized, mode=mode)

tests/test_metadata/test_consolidated.py

@@ -467,6 +467,35 @@ def test_to_dict_empty(self):
         }
         assert result == expected
 
+    @pytest.mark.parametrize("zarr_format", [2, 3])
+    async def test_to_dict_order(
+        self, memory_store: zarr.storage.MemoryStore, zarr_format: ZarrFormat
+    ) -> None:
+        with zarr.config.set(default_zarr_format=zarr_format):
+            g = await group(store=memory_store)
+
+            # Create groups in non-lexicographix order
+            dtype = "float32"
+            await g.create_array(name="b", shape=(1,), dtype=dtype)
+            child = await g.create_group("c", attributes={"key": "child"})
+            await g.create_array(name="a", shape=(1,), dtype=dtype)
+
+            await child.create_array("e", shape=(1,), dtype=dtype)
+            await child.create_array("d", shape=(1,), dtype=dtype)
+
+            # Consolidate metadata and re-open store
+            await zarr.api.asynchronous.consolidate_metadata(memory_store)
+            g2 = await zarr.api.asynchronous.open_group(store=memory_store)
+
+            assert list(g2.metadata.consolidated_metadata.metadata) == ["a", "b", "c"]
+            assert list(g2.metadata.consolidated_metadata.flattened_metadata) == [
+                "a",
+                "b",
+                "c",
+                "c/d",
+                "c/e",
+            ]
+
     @pytest.mark.parametrize("zarr_format", [2, 3])
     async def test_open_consolidated_raises_async(self, zarr_format: ZarrFormat):
         store = zarr.storage.MemoryStore()

tests/test_store/test_fsspec.py

@@ -388,8 +388,8 @@ def test_open_s3map_raises() -> None:
     ):
         zarr.open(store=mapper, path="bar", mode="w", shape=(3, 3))
     with pytest.raises(
-        ValueError,
-        match="'storage_options was provided but is not used for FSMap store_like objects",
+        TypeError,
+        match="'storage_options' is only used when the store is passed as a FSSpec URI string.",
     ):
         zarr.open(store=mapper, storage_options={"anon": True}, mode="w", shape=(3, 3))