remove clobber from normalize_store_arg to enable writes to consolidated FSStore groups (#976)

Author: d70-t

docs/release.rst

@@ -6,6 +6,14 @@ Release notes
 Unreleased
 ----------
 
+
+Bug fixes
+~~~~~~~~~
+
+* Removed `clobber` argument from `normalize_store_arg`. This enables to change
+  data within a opened consolidated group using mode `"r+"` (i.e region write).
+  By :user:`Tobias Kölling <d70-t>` :issue:`975`.
+
 .. _release_2.11.0:
 
 2.11.0

zarr/convenience.py

@@ -77,11 +77,10 @@ def open(store: StoreLike = None, mode: str = "a", **kwargs):
 
     path = kwargs.get('path')
     # handle polymorphic store arg
-    clobber = mode == 'w'
     # we pass storage options explicitly, since normalize_store_arg might construct
     # a store if the input is a fsspec-compatible URL
     _store: BaseStore = normalize_store_arg(
-        store, clobber=clobber, storage_options=kwargs.pop("storage_options", {})
+        store, storage_options=kwargs.pop("storage_options", {}), mode=mode
     )
     path = normalize_storage_path(path)
 
@@ -142,7 +141,7 @@ def save_array(store: StoreLike, arr, **kwargs):
 
     """
     may_need_closing = _might_close(store)
-    _store: BaseStore = normalize_store_arg(store, clobber=True)
+    _store: BaseStore = normalize_store_arg(store, mode="w")
     try:
         _create_array(arr, store=_store, overwrite=True, **kwargs)
     finally:
@@ -213,7 +212,7 @@ def save_group(store: StoreLike, *args, **kwargs):
         raise ValueError('at least one array must be provided')
     # handle polymorphic store arg
     may_need_closing = _might_close(store)
-    _store: BaseStore = normalize_store_arg(store, clobber=True)
+    _store: BaseStore = normalize_store_arg(store, mode="w")
     try:
         grp = _create_group(_store, overwrite=True)
         for i, arr in enumerate(args):
@@ -1117,7 +1116,7 @@ def consolidate_metadata(store: StoreLike, metadata_key=".zmetadata"):
     open_consolidated
 
     """
-    store = normalize_store_arg(store, clobber=True)
+    store = normalize_store_arg(store, mode="w")
 
     def is_zarr_key(key):
         return (key.endswith('.zarray') or key.endswith('.zgroup') or
@@ -1179,7 +1178,7 @@ def open_consolidated(store: StoreLike, metadata_key=".zmetadata", mode="r+", **
     from .storage import ConsolidatedMetadataStore
 
     # normalize parameters
-    store = normalize_store_arg(store, storage_options=kwargs.get("storage_options"))
+    store = normalize_store_arg(store, storage_options=kwargs.get("storage_options"), mode=mode)
     if mode not in {'r', 'r+'}:
         raise ValueError("invalid mode, expected either 'r' or 'r+'; found {!r}"
                          .format(mode))

zarr/creation.py

@@ -490,11 +490,11 @@ def open_array(
     # a : read/write if exists, create otherwise (default)
 
     # handle polymorphic store arg
-    clobber = (mode == 'w')
-    store = normalize_store_arg(store, clobber=clobber, storage_options=storage_options, mode=mode)
+    store = normalize_store_arg(store, storage_options=storage_options, mode=mode)
     if chunk_store is not None:
-        chunk_store = normalize_store_arg(chunk_store, clobber=clobber,
-                                          storage_options=storage_options)
+        chunk_store = normalize_store_arg(chunk_store,
+                                          storage_options=storage_options,
+                                          mode=mode)
     path = normalize_storage_path(path)
 
     # API compatibility with h5py

zarr/hierarchy.py

@@ -1051,10 +1051,10 @@ def move(self, source, dest):
         self._write_op(self._move_nosync, source, dest)
 
 
-def _normalize_store_arg(store, *, clobber=False, storage_options=None, mode=None):
+def _normalize_store_arg(store, *, storage_options=None, mode="r"):
     if store is None:
         return MemoryStore()
-    return normalize_store_arg(store, clobber=clobber,
+    return normalize_store_arg(store,
                                storage_options=storage_options, mode=mode)
 
 
@@ -1164,13 +1164,13 @@ def open_group(store=None, mode='a', cache_attrs=True, synchronizer=None, path=N
     """
 
     # handle polymorphic store arg
-    clobber = mode != "r"
     store = _normalize_store_arg(
-        store, clobber=clobber, storage_options=storage_options, mode=mode
+        store, storage_options=storage_options, mode=mode
     )
     if chunk_store is not None:
-        chunk_store = _normalize_store_arg(chunk_store, clobber=clobber,
-                                           storage_options=storage_options)
+        chunk_store = _normalize_store_arg(chunk_store,
+                                           storage_options=storage_options,
+                                           mode=mode)
     path = normalize_storage_path(path)
 
     # ensure store is initialized

zarr/storage.py

@@ -104,13 +104,12 @@ def contains_group(store: StoreLike, path: Path = None) -> bool:
     return key in store
 
 
-def normalize_store_arg(store: Any, clobber=False, storage_options=None, mode="w") -> BaseStore:
+def normalize_store_arg(store: Any, storage_options=None, mode="r") -> BaseStore:
     if store is None:
         return BaseStore._ensure_store(dict())
     elif isinstance(store, os.PathLike):
         store = os.fspath(store)
     if isinstance(store, str):
-        mode = mode if clobber else "r"
         if "://" in store or "::" in store:
             return FSStore(store, mode=mode, **(storage_options or {}))
         elif storage_options:

zarr/tests/test_convenience.py

@@ -175,6 +175,8 @@ def test_consolidate_metadata():
         open_consolidated(store, mode='a')
     with pytest.raises(ValueError):
         open_consolidated(store, mode='w')
+    with pytest.raises(ValueError):
+        open_consolidated(store, mode='w-')
 
     # make sure keyword arguments are passed through without error
     open_consolidated(store, cache_attrs=True, synchronizer=None)
@@ -224,6 +226,8 @@ def test_consolidated_with_chunk_store():
         open_consolidated(store, mode='a', chunk_store=chunk_store)
     with pytest.raises(ValueError):
         open_consolidated(store, mode='w', chunk_store=chunk_store)
+    with pytest.raises(ValueError):
+        open_consolidated(store, mode='w-', chunk_store=chunk_store)
 
     # make sure keyword arguments are passed through without error
     open_consolidated(store, cache_attrs=True, synchronizer=None,

zarr/tests/test_storage.py

@@ -1021,6 +1021,42 @@ def test_create(self):
         with pytest.raises(PermissionError):
             g.data[:] = 1
 
+    @pytest.mark.parametrize('mode,allowed', [("r", False), ("r+", True)])
+    def test_modify_consolidated(self, mode, allowed):
+        import zarr
+        url = "file://" + tempfile.mkdtemp()
+
+        # create
+        root = zarr.open_group(url, mode="w")
+        root.zeros('baz', shape=(10000, 10000), chunks=(1000, 1000), dtype='i4')
+        zarr.consolidate_metadata(url)
+
+        # reopen and modify
+        root = zarr.open_consolidated(url, mode=mode)
+        if allowed:
+            root["baz"][0, 0] = 7
+
+            root = zarr.open_consolidated(url, mode="r")
+            assert root["baz"][0, 0] == 7
+        else:
+            with pytest.raises(zarr.errors.ReadOnlyError):
+                root["baz"][0, 0] = 7
+
+    @pytest.mark.parametrize('mode', ["r", "r+"])
+    def test_modify_consolidated_metadata_raises(self, mode):
+        import zarr
+        url = "file://" + tempfile.mkdtemp()
+
+        # create
+        root = zarr.open_group(url, mode="w")
+        root.zeros('baz', shape=(10000, 10000), chunks=(1000, 1000), dtype='i4')
+        zarr.consolidate_metadata(url)
+
+        # reopen and modify
+        root = zarr.open_consolidated(url, mode=mode)
+        with pytest.raises(zarr.errors.ReadOnlyError):
+            root["baz"].resize(100, 100)
+
     def test_read_only(self):
         path = tempfile.mkdtemp()
         atexit.register(atexit_rmtree, path)