Repository: pydata/xarray

Issue:
to_zarr with region and write_empty_chunks keywords raises ReadOnlyError if using consolidated metadata
### What happened?

When using `Dataset.to_zarr` with the `region` keyword provided, if the keyword `write_empty_chunks` is set to False (or True), the operation will fail with a `ReadOnlyError: object is read-only`. This occurs when keyword `consolidated` is not provided (and thus defaults to None) or if it is set to True. It does not happen when `consolidated=False` is provided.

From what I can find, while the error stems from the zarr library, the root cause of this bug occurs in [line 677 of xarray/backends/zarr.py](https://github.com/pydata/xarray/blob/df0ddaf2e68a6b033b4e39990d7006dc346fcc8c/xarray/backends/zarr.py#L679C45-L679C45). Basically, a new zarr Array is created with `store=self.zarr_group.store` (which is a read-only `zarr.storage.ConsolidatedMetadataStore` instance when using consolidated metadata), instead of `store=self.zarr_group.chunk_store` (which is a normal `zarr.storage.DirectoryStore` instance), which is effectively the instance that is used in all other cases.

Replacing `store=self.zarr_group.store` with `store=self.zarr_group.chunk_store` seems to solve the issue.

### What did you expect to happen?

I expected not to get an error and instead to write the data to the given zarr region, whilst respecting the `write_empty_chunks=False` behavior.

### Minimal Complete Verifiable Example

```Python
import xarray as xr
import numpy as np

store_path = "test.zarr"
# First, create an empty Zarr store using compute=False
ds = xr.Dataset(coords={"x": range(1000), "y": range(2000)}, data_vars={"foo": (("x", "y"), np.zeros((1000, 2000), dtype="float32"))})
ds.to_zarr(store_path, compute=False)

# Attempts to write to the zarr using 'region' and 'write_empty_chunks=False'
region = {"x": slice(0, 1000), "y": slice(0, 2000)}
# Without using consolidated metadata works:
ds.to_zarr(store_path, region=region, write_empty_chunks=False, consolidated=False)
# But when using consolidated metadata, it raises a ReadOnlyError
ds.to_zarr(store_path, region=region, write_empty_chunks=False)
```


### MVCE confirmation

- [X] Minimal example — the example is as focused as reasonably possible to demonstrate the underlying issue in xarray.
- [X] Complete example — the example is self-contained, including all data and the text of any traceback.
- [X] Verifiable example — the example copy & pastes into an IPython prompt or [Binder notebook](https://mybinder.org/v2/gh/pydata/xarray/main?urlpath=lab/tree/doc/examples/blank_template.ipynb), returning the result.
- [X] New issue — a search of GitHub Issues suggests this is not a duplicate.
- [X] Recent environment — the issue occurs with the latest version of xarray and its dependencies.

### Relevant log output

```Python
ReadOnlyError                             Traceback (most recent call last)
<ipython-input-2-9da43054efc1> in <module>
----> 1 ds.to_zarr(store_path, region=region, write_empty_chunks=False)

venv/lib/python3.10/site-packages/xarray/core/dataset.py in to_zarr(self, store, chunk_store, mode, synchronizer, group, encoding, compute, consolidated, append_dim, region, safe_chunks, storage_options, zarr_version, write_empty_chunks, chunkmanager_store_kwargs)
   2472         from xarray.backends.api import to_zarr
   2473
-> 2474         return to_zarr(  # type: ignore[call-overload,misc]
   2475             self,
   2476             store=store,

venv/lib/python3.10/site-packages/xarray/backends/api.py in to_zarr(dataset, store, chunk_store, mode, synchronizer, group, encoding, compute, consolidated, append_dim, region, safe_chunks, storage_options, zarr_version, write_empty_chunks, chunkmanager_store_kwargs)
   1704     writer = ArrayWriter()
   1705     # TODO: figure out how to properly handle unlimited_dims
-> 1706     dump_to_store(dataset, zstore, writer, encoding=encoding)
   1707     writes = writer.sync(
   1708         compute=compute, chunkmanager_store_kwargs=chunkmanager_store_kwargs

venv/lib/python3.10/site-packages/xarray/backends/api.py in dump_to_store(dataset, store, writer, encoder, encoding, unlimited_dims)
   1306         variables, attrs = encoder(variables, attrs)
   1307
-> 1308     store.store(variables, attrs, check_encoding, writer, unlimited_dims=unlimited_dims)
   1309
   1310

venv/lib/python3.10/site-packages/xarray/backends/zarr.py in store(self, variables, attributes, check_encoding_set, writer, unlimited_dims)
    627             self.set_dimensions(variables_encoded, unlimited_dims=unlimited_dims)
    628
--> 629         self.set_variables(
    630             variables_encoded, check_encoding_set, writer, unlimited_dims=unlimited_dims
    631         )

venv/lib/python3.10/site-packages/xarray/backends/zarr.py in set_variables(self, variables, check_encoding_set, writer, unlimited_dims)
    732
    733             region = tuple(write_region[dim] for dim in dims)
--> 734             writer.add(v.data, zarr_array, region)
    735
    736     def close(self):

venv/lib/python3.10/site-packages/xarray/backends/common.py in add(self, source, target, region)
    239         else:
    240             if region:
--> 241                 target[region] = source
    242             else:
    243                 target[...] = source

venv/lib/python3.10/site-packages/zarr/core.py in __setitem__(self, selection, value)
   1493             self.vindex[selection] = value
   1494         elif is_pure_orthogonal_indexing(pure_selection, self.ndim):
-> 1495             self.set_orthogonal_selection(pure_selection, value, fields=fields)
   1496         else:
   1497             self.set_basic_selection(pure_selection, value, fields=fields)

venv/lib/python3.10/site-packages/zarr/core.py in set_orthogonal_selection(self, selection, value, fields)
   1682         indexer = OrthogonalIndexer(selection, self)
   1683
-> 1684         self._set_selection(indexer, value, fields=fields)
   1685
   1686     def set_coordinate_selection(self, selection, value, fields=None):

venv/lib/python3.10/site-packages/zarr/core.py in _set_selection(self, indexer, value, fields)
   2036
   2037                 # put data
-> 2038                 self._chunk_setitem(chunk_coords, chunk_selection, chunk_value, fields=fields)
   2039         else:
   2040             lchunk_coords, lchunk_selection, lout_selection = zip(*indexer)

venv/lib/python3.10/site-packages/zarr/core.py in _chunk_setitem(self, chunk_coords, chunk_selection, value, fields)
   2302
   2303         with lock:
-> 2304             self._chunk_setitem_nosync(chunk_coords, chunk_selection, value, fields=fields)
   2305
   2306     def _chunk_setitem_nosync(self, chunk_coords, chunk_selection, value, fields=None):

venv/lib/python3.10/site-packages/zarr/core.py in _chunk_setitem_nosync(self, chunk_coords, chunk_selection, value, fields)
   2312             self._chunk_delitem(ckey)
   2313         else:
-> 2314             self.chunk_store[ckey] = self._encode_chunk(cdata)
   2315
   2316     def _process_for_setitem(self, ckey, chunk_selection, value, fields=None):

venv/lib/python3.10/site-packages/zarr/storage.py in __setitem__(self, key, value)
   2973
   2974     def __setitem__(self, key, value):
-> 2975         raise ReadOnlyError()
   2976
   2977     def getsize(self, path):

ReadOnlyError: object is read-only
```


### Anything else we need to know?

_No response_

### Environment

<details>

INSTALLED VERSIONS
------------------
commit: None
python: 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0]
python-bits: 64
OS: Linux
OS-release: 5.15.90.1-microsoft-standard-WSL2
machine: x86_64
processor: x86_64
byteorder: little
LC_ALL: None
LANG: C.UTF-8
LOCALE: ('en_US', 'UTF-8')
libhdf5: None
libnetcdf: None

xarray: 2023.9.0
pandas: 2.1.1
numpy: 1.25.2
scipy: 1.11.2
netCDF4: None
pydap: None
h5netcdf: None
h5py: None
Nio: None
zarr: 2.16.1
cftime: None
nc_time_axis: None
PseudoNetCDF: None
iris: None
bottleneck: 1.3.7
dask: 2023.9.3
distributed: None
matplotlib: 3.5.1
cartopy: None
seaborn: None
numbagg: None
fsspec: 2023.6.0
cupy: None
pint: None
sparse: None
flox: None
numpy_groupies: None
setuptools: 59.6.0
pip: 23.3
conda: None
pytest: 7.4.2
mypy: 1.5.1
IPython: 7.31.1
sphinx: None

</details>


Discussion:
max-sixty: This looks good @Metamess . Thank you!

I'll merge it soon unless anyone knows it better (I don't know this code that well, though the tests look good)
dcherian: Excellent work @Metamess . Thank you!

Patch:
diff --git a/doc/whats-new.rst b/doc/whats-new.rst
index 3d9d3f6f..3a9be494 100644
--- a/doc/whats-new.rst
+++ b/doc/whats-new.rst
@@ -142,6 +142,11 @@ Bug fixes
   (:issue:`8290`, :pull:`8297`).
   By `Oliver McCormack <https://github.com/olimcc>`_.
 
+- Fix to_zarr ending in a ReadOnlyError when consolidated metadata was used and the
+  write_empty_chunks was provided.
+  (:issue:`8323`, :pull:`8326`)
+  By `Matthijs Amesz <https://github.com/Metamess>`_.
+
 
 Documentation
 ~~~~~~~~~~~~~
diff --git a/xarray/backends/zarr.py b/xarray/backends/zarr.py
index d6ad15f4..2b41fa52 100644
--- a/xarray/backends/zarr.py
+++ b/xarray/backends/zarr.py
@@ -675,8 +675,23 @@ class ZarrStore(AbstractWritableDataStore):
                 # metadata. This would need some case work properly with region
                 # and append_dim.
                 if self._write_empty is not None:
+                    # Write to zarr_group.chunk_store instead of zarr_group.store
+                    # See https://github.com/pydata/xarray/pull/8326#discussion_r1365311316 for a longer explanation
+                    #    The open_consolidated() enforces a mode of r or r+
+                    #    (and to_zarr with region provided enforces a read mode of r+),
+                    #    and this function makes sure the resulting Group has a store of type ConsolidatedMetadataStore
+                    #    and a 'normal Store subtype for chunk_store.
+                    #    The exact type depends on if a local path was used, or a URL of some sort,
+                    #    but the point is that it's not a read-only ConsolidatedMetadataStore.
+                    #    It is safe to write chunk data to the chunk_store because no metadata would be changed by
+                    #    to_zarr with the region parameter:
+                    #     - Because the write mode is enforced to be r+, no new variables can be added to the store
+                    #       (this is also checked and enforced in xarray.backends.api.py::to_zarr()).
+                    #     - Existing variables already have their attrs included in the consolidated metadata file.
+                    #     - The size of dimensions can not be expanded, that would require a call using `append_dim`
+                    #        which is mutually exclusive with `region`
                     zarr_array = zarr.open(
-                        store=self.zarr_group.store,
+                        store=self.zarr_group.chunk_store,
                         path=f"{self.zarr_group.name}/{name}",
                         write_empty_chunks=self._write_empty,
                     )


Test patch:
diff --git a/xarray/tests/test_backends.py b/xarray/tests/test_backends.py
index 59e9f655..73352c3f 100644
--- a/xarray/tests/test_backends.py
+++ b/xarray/tests/test_backends.py
@@ -2459,7 +2459,8 @@ class ZarrBase(CFEncodedBase):
     @pytest.mark.parametrize("consolidated", [False, True, None])
     @pytest.mark.parametrize("compute", [False, True])
     @pytest.mark.parametrize("use_dask", [False, True])
-    def test_write_region(self, consolidated, compute, use_dask) -> None:
+    @pytest.mark.parametrize("write_empty", [False, True, None])
+    def test_write_region(self, consolidated, compute, use_dask, write_empty) -> None:
         if (use_dask or not compute) and not has_dask:
             pytest.skip("requires dask")
         if consolidated and self.zarr_version > 2:
@@ -2491,6 +2492,7 @@ class ZarrBase(CFEncodedBase):
                     store,
                     region=region,
                     consolidated=consolidated,
+                    write_empty_chunks=write_empty,
                     **self.version_kwargs,
                 )
             with xr.open_zarr(
@@ -2772,9 +2774,12 @@ class TestZarrWriteEmpty(TestZarrDirectoryStore):
         ) as ds:
             yield ds
 
-    @pytest.mark.parametrize("write_empty", [True, False])
-    def test_write_empty(self, write_empty: bool) -> None:
-        if not write_empty:
+    @pytest.mark.parametrize("consolidated", [True, False, None])
+    @pytest.mark.parametrize("write_empty", [True, False, None])
+    def test_write_empty(
+        self, consolidated: bool | None, write_empty: bool | None
+    ) -> None:
+        if write_empty is False:
             expected = ["0.1.0", "1.1.0"]
         else:
             expected = [