Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pull] main from pydata:main #536

Merged
merged 1 commit into from
Nov 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/whats-new.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
~~~~~~~~~~~~~
Expand Down
17 changes: 16 additions & 1 deletion xarray/backends/zarr.py
Original file line number Diff line number Diff line change
Expand Up @@ -675,8 +675,23 @@ def set_variables(self, variables, check_encoding_set, writer, unlimited_dims=No
# 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,
)
Expand Down
13 changes: 9 additions & 4 deletions xarray/tests/test_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -2459,7 +2459,8 @@ def test_no_warning_from_open_emptydim_with_chunks(self) -> None:
@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:
Expand Down Expand Up @@ -2491,6 +2492,7 @@ def test_write_region(self, consolidated, compute, use_dask) -> None:
store,
region=region,
consolidated=consolidated,
write_empty_chunks=write_empty,
**self.version_kwargs,
)
with xr.open_zarr(
Expand Down Expand Up @@ -2772,9 +2774,12 @@ def roundtrip_dir(
) 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 = [
Expand Down
Loading