Repository: zarr-developers/zarr-python

Issue:
Open mode is lost when opening via FSStore
This issue describes failed writes to xarray groups using `FSStore` where I'd expect the write to succeed. My current assumption is, that these issues occure due to mode-flags getting lost when the store is created implicitly (i.e. only an url is passed to zarr functions). I don't yet know how to fix this or if this is intended behaviour, thus I'm creating an issue.

#### Minimal, reproducible code sample, a copy-pastable example if possible

```python
import zarr
import os

here = os.path.abspath(os.path.dirname(__file__))
url = "file://" + here + "/bug.zarr"  # intentionally using fsspec, problem occurs when using any FSStore backend

root = zarr.group(url, overwrite=True)
```

#### Problem description

The code above raises:
```
Traceback (most recent call last):
  File "/Users/tobi/Documents/code/zarrbug/test_write.py", line 7, in <module>
    root = zarr.group(url, overwrite=True)
  File "/usr/local/lib/python3.9/site-packages/zarr/hierarchy.py", line 1112, in group
    init_group(store, overwrite=overwrite, chunk_store=chunk_store,
  File "/usr/local/lib/python3.9/site-packages/zarr/storage.py", line 490, in init_group
    _init_group_metadata(store=store, overwrite=overwrite, path=path,
  File "/usr/local/lib/python3.9/site-packages/zarr/storage.py", line 504, in _init_group_metadata
    rmdir(store, path)
  File "/usr/local/lib/python3.9/site-packages/zarr/storage.py", line 138, in rmdir
    store.rmdir(path)  # type: ignore
  File "/usr/local/lib/python3.9/site-packages/zarr/storage.py", line 1259, in rmdir
    raise ReadOnlyError()
zarr.errors.ReadOnlyError: object is read-only
```

I'd expect that the code works (in particular due to `overwrite=True`)

#### (likely) related variant

```python3
import zarr    
import os    
    
here = os.path.abspath(os.path.dirname(__file__))    
url = "file://" + here + "/bug.zarr"    
store = zarr.storage.FSStore(url)    
    
root = zarr.group(store, overwrite=True)    
root.zeros('baz', shape=(10000, 10000), chunks=(1000, 1000), dtype='i4')    
zarr.consolidate_metadata(store)    
    
    
root = zarr.open_consolidated(url, mode="r+")    
root["baz"][0,0] = 7 
```

raises:

```
Traceback (most recent call last):
  File "/Users/tobi/Documents/code/zarrbug/test.py", line 14, in <module>
    root["baz"][0,0] = 7
  File "/usr/local/lib/python3.9/site-packages/zarr/core.py", line 1285, in __setitem__
    self.set_basic_selection(pure_selection, value, fields=fields)
  File "/usr/local/lib/python3.9/site-packages/zarr/core.py", line 1380, in set_basic_selection
    return self._set_basic_selection_nd(selection, value, fields=fields)
  File "/usr/local/lib/python3.9/site-packages/zarr/core.py", line 1680, in _set_basic_selection_nd
    self._set_selection(indexer, value, fields=fields)
  File "/usr/local/lib/python3.9/site-packages/zarr/core.py", line 1732, in _set_selection
    self._chunk_setitem(chunk_coords, chunk_selection, chunk_value, fields=fields)
  File "/usr/local/lib/python3.9/site-packages/zarr/core.py", line 1994, in _chunk_setitem
    self._chunk_setitem_nosync(chunk_coords, chunk_selection, value,
  File "/usr/local/lib/python3.9/site-packages/zarr/core.py", line 2005, in _chunk_setitem_nosync
    self.chunk_store[ckey] = self._encode_chunk(cdata)
  File "/usr/local/lib/python3.9/site-packages/zarr/storage.py", line 1177, in __setitem__
    raise ReadOnlyError()
zarr.errors.ReadOnlyError: object is read-only
```

I've explicitly instantiated the store when creating the dataset (which circuments the previous error).
But when trying to use the url directly for updating the dataset, I recieve a similar error (note that this error is raised on the line `root["baz"][0,0] = 7`).

#### context

The following xarray-based code raises the same (second) exception:

```python3
import numpy as np
import xarray as xr
import os

here = os.path.abspath(os.path.dirname(__file__))
url = "file://" + here + "/bug.zarr"

ds = xr.Dataset({"baz": (("x", "y"), np.zeros((10000, 10000), dtype="i4"))}).chunk({"x": 1000, "y": 1000})
ds.to_zarr(url, compute=False)

ds.isel(x=slice(1000), y=slice(1000)).to_zarr(url, mode="r+", region={"x": slice(1000), "y": slice(1000)})
```

Thanks @observingClouds wo showed me this example.

#### Version and installation information

* Value of ``zarr.__version__``: '2.11.0'
* Value of ``numcodecs.__version__``: '0.7.3'
* Python 3.9.10
* Operating system: Linux
* zarr was installed using pip globally

Discussion:
pep8speaks: Hello @d70-t! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:









* In the file [`zarr/tests/test_storage.py`](https://github.com/zarr-developers/zarr-python/blob/0068828e5dfcc49b218bc2928aa70890de21657a/zarr/tests/test_storage.py):

> [Line 1035:22](https://github.com/zarr-developers/zarr-python/blob/0068828e5dfcc49b218bc2928aa70890de21657a/zarr/tests/test_storage.py#L1035): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ','


codecov[bot]: # [Codecov](https://codecov.io/gh/zarr-developers/zarr-python/pull/976?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#976](https://codecov.io/gh/zarr-developers/zarr-python/pull/976?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (ab410f0) into [master](https://codecov.io/gh/zarr-developers/zarr-python/commit/79307fc71a3fa1549577045028f67db9609a52dc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (79307fc) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##           master     #976   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          32       32           
  Lines       11225    11229    +4     
=======================================
+ Hits        11219    11223    +4     
  Misses          6        6           
```

| [Impacted Files](https://codecov.io/gh/zarr-developers/zarr-python/pull/976?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [zarr/convenience.py](https://codecov.io/gh/zarr-developers/zarr-python/pull/976/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-emFyci9jb252ZW5pZW5jZS5weQ==) | `100.00% <100.00%> (ø)` | |
| [zarr/creation.py](https://codecov.io/gh/zarr-developers/zarr-python/pull/976/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-emFyci9jcmVhdGlvbi5weQ==) | `100.00% <100.00%> (ø)` | |
| [zarr/hierarchy.py](https://codecov.io/gh/zarr-developers/zarr-python/pull/976/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-emFyci9oaWVyYXJjaHkucHk=) | `100.00% <100.00%> (ø)` | |
| [zarr/storage.py](https://codecov.io/gh/zarr-developers/zarr-python/pull/976/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-emFyci9zdG9yYWdlLnB5) | `100.00% <100.00%> (ø)` | |
| [zarr/tests/test\_storage.py](https://codecov.io/gh/zarr-developers/zarr-python/pull/976/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-emFyci90ZXN0cy90ZXN0X3N0b3JhZ2UucHk=) | `100.00% <100.00%> (ø)` | |

joshmoore: @martindurant: would you be up for taking a closer look here?
martindurant: If no one else has any comments, I'll merge this by tomorrow.

Patch:
diff --git a/docs/release.rst b/docs/release.rst
index 16c884ef..c1ca0355 100644
--- a/docs/release.rst
+++ b/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
diff --git a/zarr/convenience.py b/zarr/convenience.py
index 4d079685..0cb20220 100644
--- a/zarr/convenience.py
+++ b/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))
diff --git a/zarr/creation.py b/zarr/creation.py
index 7e7adcb1..9d6902a6 100644
--- a/zarr/creation.py
+++ b/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
diff --git a/zarr/hierarchy.py b/zarr/hierarchy.py
index 763a5f16..31c9e2a8 100644
--- a/zarr/hierarchy.py
+++ b/zarr/hierarchy.py
@@ -1051,10 +1051,10 @@ class Group(MutableMapping):
         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
diff --git a/zarr/storage.py b/zarr/storage.py
index 7f572d35..35e1fdb0 100644
--- a/zarr/storage.py
+++ b/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:


Test patch:
diff --git a/zarr/tests/test_convenience.py b/zarr/tests/test_convenience.py
index e5ccbd49..a6041b78 100644
--- a/zarr/tests/test_convenience.py
+++ b/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,
diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py
index e85973b5..f39b1d87 100644
--- a/zarr/tests/test_storage.py
+++ b/zarr/tests/test_storage.py
@@ -1021,6 +1021,42 @@ class TestFSStore(StoreTests):
         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)