Skip to content

Commit d787844

Browse files
aaronspringclaude
andcommitted
Surface mode in to_icechunk, keeping current defaults
Add a mode parameter ("w", "w-", "a") to vds.vz.to_icechunk and dt.vz.to_icechunk. The default (None) keeps the existing behaviour of raising ContainsGroupError when the target group already exists, while mode="a" opens the existing group so datasets can e.g. be split across commits by variable, and mode="w" overwrites existing contents. A _resolve_mode helper validates mode and resolves it to the effective zarr group-open mode ("r+" when append_dim/region require an existing group), so both writers open groups through a single zarr.open_group call. Closes #1001. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent a2d7a13 commit d787844

5 files changed

Lines changed: 196 additions & 16 deletions

File tree

docs/about/releases.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
### New Features
66
- Added [VirtualiZarrDatasetAccessor.nrefs][virtualizarr.accessor.VirtualiZarrDatasetAccessor.nrefs] — a method that returns the total number of virtual chunk references in the dataset, ignoring non-virtual variables. Closes #573.
7+
- `vds.vz.to_icechunk` and `vdt.vz.to_icechunk` now accept a `mode` parameter controlling how a pre-existing group at the target path is handled: `"w-"` (create, error if the group exists — the previous and still-default behaviour), `"w"` (overwrite existing contents), or `"a"` (open the existing group and add/update variables in it). `mode="a"` enables e.g. splitting a large virtual dataset across commits by variable. Closes #1001.
8+
By [Aaron Spring](https://github.com/aaronspring).
79

810
### Breaking changes
911

docs/how_to/scaling.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ for i, start in enumerate(range(0, vds.sizes["time"], step)):
334334

335335
If the slice boundaries don't align with chunk edges along that axis, the indexing call raises `SubChunkIndexingError`.
336336

337-
(Remember you can also subset the Dataset to specific variables and commit those separately too if necessary.)
337+
(Remember you can also subset the Dataset to specific variables and commit those separately too if necessary — pass `mode="a"` to `to_icechunk` from the second write onwards, so that writing into the already-existing group doesn't raise a `ContainsGroupError`.)
338338

339339
### Retries
340340

virtualizarr/accessor.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ def to_icechunk(
7575
store: "IcechunkStore",
7676
*,
7777
group: str | None = None,
78+
mode: Literal["w", "w-", "a"] | None = None,
7879
append_dim: str | None = None,
7980
region: Literal["auto"] | Mapping[str, Literal["auto"] | slice] | None = None,
8081
validate_containers: bool = True,
@@ -104,6 +105,16 @@ def to_icechunk(
104105
Store to write dataset into.
105106
group
106107
Path of the group to write the dataset into (default: the root group).
108+
mode
109+
How to handle a pre-existing group at the target path:
110+
111+
- ``"w-"``: create the group, raising a ``ContainsGroupError`` if it already exists.
112+
- ``"w"``: create the group, overwriting any existing contents at that path.
113+
- ``"a"``: open the group if it exists (keeping existing arrays), otherwise create it.
114+
- ``None`` (default): equivalent to ``"w-"``, unless ``append_dim`` or ``region``
115+
is given, in which case the existing group is opened.
116+
117+
``mode="w"`` and ``mode="w-"`` are incompatible with ``append_dim`` and ``region``.
107118
append_dim
108119
Dimension along which to append the virtual dataset.
109120
region
@@ -133,6 +144,7 @@ def to_icechunk(
133144
self.ds,
134145
store,
135146
group=group,
147+
mode=mode,
136148
append_dim=append_dim,
137149
region=region,
138150
validate_containers=validate_containers,
@@ -337,6 +349,7 @@ def to_icechunk(
337349
self,
338350
store: "IcechunkStore",
339351
*,
352+
mode: Literal["w", "w-", "a"] | None = None,
340353
write_inherited_coords: bool = False,
341354
validate_containers: bool = True,
342355
last_updated_at: datetime | None = None,
@@ -361,6 +374,13 @@ def to_icechunk(
361374
----------
362375
store
363376
Store to write dataset into.
377+
mode
378+
How to handle pre-existing groups at the target paths:
379+
380+
- ``"w-"`` or ``None`` (default): create each group, raising a
381+
``ContainsGroupError`` if it already exists.
382+
- ``"w"``: create each group, overwriting any existing contents at that path.
383+
- ``"a"``: open each group if it exists (keeping existing arrays), otherwise create it.
364384
write_inherited_coords
365385
If ``True``, replicate inherited coordinates on all descendant nodes.
366386
Otherwise, only write coordinates at the level at which they are
@@ -400,6 +420,7 @@ def to_icechunk(
400420
virtual_datatree_to_icechunk(
401421
self.dt,
402422
store,
423+
mode=mode,
403424
write_inherited_coords=write_inherited_coords,
404425
validate_containers=validate_containers,
405426
last_updated_at=last_updated_at,

virtualizarr/tests/test_writers/test_icechunk.py

Lines changed: 114 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
from zarr.codecs import BytesCodec
1616
from zarr.core.metadata import ArrayV3Metadata
1717
from zarr.dtype import parse_data_type
18+
from zarr.errors import ContainsGroupError
1819

1920
from virtualizarr import open_virtual_dataset
2021
from virtualizarr.manifests import ChunkManifest, ManifestArray
@@ -62,7 +63,7 @@ def icechunk_filestore(icechunk_repo: "Repository") -> "IcechunkStore":
6263
return session.store
6364

6465

65-
@pytest.mark.parametrize("kwarg", [("group", {}), ("append_dim", {})])
66+
@pytest.mark.parametrize("kwarg", [("group", {}), ("mode", {}), ("append_dim", {})])
6667
def test_invalid_kwarg_type(
6768
icechunk_filestore: "IcechunkStore",
6869
vds_with_manifest_arrays: xr.Dataset,
@@ -113,6 +114,118 @@ def test_write_new_virtual_variable(
113114
assert arr.metadata.dimension_names == ("x", "y")
114115

115116

117+
@pytest.mark.parametrize("mode", [None, "w-"])
118+
def test_write_to_existing_group_fails_by_default(
119+
icechunk_filestore: "IcechunkStore",
120+
vds_with_manifest_arrays: xr.Dataset,
121+
mode: Optional[str],
122+
):
123+
vds = vds_with_manifest_arrays
124+
vds.vz.to_icechunk(icechunk_filestore, validate_containers=False)
125+
126+
with pytest.raises(ContainsGroupError):
127+
vds.vz.to_icechunk(icechunk_filestore, mode=mode, validate_containers=False)
128+
129+
130+
def test_write_variables_across_commits_with_mode_a(
131+
icechunk_repo: "Repository",
132+
synthetic_vds_multiple_vars,
133+
):
134+
# regression test for https://github.com/zarr-developers/VirtualiZarr/issues/1001
135+
vds, arr = synthetic_vds_multiple_vars
136+
137+
session1 = icechunk_repo.writable_session("main")
138+
vds[["foo"]].vz.to_icechunk(session1.store)
139+
session1.commit("wrote foo")
140+
141+
session2 = icechunk_repo.writable_session("main")
142+
vds[["bar"]].vz.to_icechunk(session2.store, mode="a")
143+
session2.commit("wrote bar")
144+
145+
with xr.open_zarr(
146+
icechunk_repo.readonly_session("main").store, zarr_format=3, consolidated=False
147+
) as ds:
148+
# both variables have encoding={"scale_factor": 2}
149+
np.testing.assert_equal(ds["foo"].data, arr * 2)
150+
np.testing.assert_equal(ds["bar"].data, arr * 2)
151+
152+
153+
def test_write_parent_group_after_child_group_with_mode_a(
154+
icechunk_filestore: "IcechunkStore",
155+
vds_with_manifest_arrays: xr.Dataset,
156+
):
157+
# regression test for https://github.com/zarr-developers/VirtualiZarr/issues/1001
158+
vds = vds_with_manifest_arrays
159+
vds.vz.to_icechunk(
160+
icechunk_filestore, group="supgroup/subgroup", validate_containers=False
161+
)
162+
vds.vz.to_icechunk(
163+
icechunk_filestore, group="supgroup", mode="a", validate_containers=False
164+
)
165+
166+
assert "a" in zarr.group(store=icechunk_filestore, path="supgroup")
167+
assert "a" in zarr.group(store=icechunk_filestore, path="supgroup/subgroup")
168+
169+
170+
def test_mode_w_overwrites_existing_group(
171+
icechunk_filestore: "IcechunkStore",
172+
synthetic_vds_multiple_vars,
173+
):
174+
vds, arr = synthetic_vds_multiple_vars
175+
vds.vz.to_icechunk(icechunk_filestore)
176+
vds[["foo"]].vz.to_icechunk(icechunk_filestore, mode="w")
177+
178+
group = zarr.group(store=icechunk_filestore)
179+
assert "foo" in group
180+
assert "bar" not in group
181+
182+
183+
def test_invalid_mode(
184+
icechunk_filestore: "IcechunkStore",
185+
vds_with_manifest_arrays: xr.Dataset,
186+
):
187+
with pytest.raises(ValueError, match="mode"):
188+
vds_with_manifest_arrays.vz.to_icechunk(icechunk_filestore, mode="r+")
189+
190+
191+
@pytest.mark.parametrize("mode", ["w", "w-"])
192+
def test_mode_incompatible_with_append_dim(
193+
icechunk_filestore: "IcechunkStore",
194+
vds_with_manifest_arrays: xr.Dataset,
195+
mode: str,
196+
):
197+
with pytest.raises(ValueError, match="append_dim or region"):
198+
vds_with_manifest_arrays.vz.to_icechunk(
199+
icechunk_filestore, mode=mode, append_dim="x"
200+
)
201+
202+
203+
def test_write_datatree_to_existing_groups_with_mode_a(
204+
icechunk_repo: "Repository",
205+
synthetic_vds_multiple_vars,
206+
):
207+
vds, arr = synthetic_vds_multiple_vars
208+
209+
session1 = icechunk_repo.writable_session("main")
210+
vdt1 = xr.DataTree.from_dict({"nested/group": vds[["foo"]]})
211+
vdt1.vz.to_icechunk(session1.store)
212+
session1.commit("wrote foo")
213+
214+
session2 = icechunk_repo.writable_session("main")
215+
vdt2 = xr.DataTree.from_dict({"nested/group": vds[["bar"]]})
216+
vdt2.vz.to_icechunk(session2.store, mode="a")
217+
session2.commit("wrote bar")
218+
219+
with xr.open_zarr(
220+
icechunk_repo.readonly_session("main").store,
221+
zarr_format=3,
222+
consolidated=False,
223+
group="nested/group",
224+
) as ds:
225+
np.testing.assert_equal(ds["foo"].data, arr * 2)
226+
np.testing.assert_equal(ds["bar"].data, arr * 2)
227+
228+
116229
def test_set_single_virtual_ref_without_encoding(
117230
icechunk_filestore: "IcechunkStore",
118231
icechunk_repo: "Repository",

virtualizarr/writers/icechunk.py

Lines changed: 58 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import xarray as xr
88
from xarray.backends.zarr import ZarrStore as XarrayZarrStore
99
from xarray.backends.zarr import encode_zarr_attr_value
10-
from zarr import Array, Group
10+
from zarr import Array, Group, open_group
1111
from zarr.core.buffer import default_buffer_prototype
1212
from zarr.core.chunk_key_encodings import DefaultChunkKeyEncoding
1313
from zarr.core.sync import sync
@@ -34,12 +34,39 @@
3434

3535
ENCODING_KEYS = {"_FillValue", "missing_value", "scale_factor", "add_offset"}
3636

37+
VALID_MODES = ("w", "w-", "a")
38+
39+
40+
def _resolve_mode(
41+
mode: Optional[Literal["w", "w-", "a"]],
42+
append_dim: Optional[str] = None,
43+
region: object = None,
44+
) -> Literal["w", "w-", "a", "r+"]:
45+
"""Validate ``mode`` and resolve it to the effective zarr group-open mode."""
46+
if not isinstance(mode, (type(None), str)):
47+
raise TypeError(f"mode: expected type Optional[str], but got type {type(mode)}")
48+
49+
if mode is not None and mode not in VALID_MODES:
50+
raise ValueError(f"mode: expected one of {VALID_MODES}, but got {mode!r}")
51+
52+
if append_dim or region:
53+
if mode in ("w", "w-"):
54+
raise ValueError(
55+
f"mode {mode!r} cannot be used together with append_dim or region, "
56+
"which require opening an existing group"
57+
)
58+
# appending or writing to a region requires the group (and arrays) to already exist
59+
return "r+"
60+
61+
return mode or "w-"
62+
3763

3864
def virtual_dataset_to_icechunk(
3965
vds: xr.Dataset,
4066
store: "IcechunkStore",
4167
*,
4268
group: Optional[str] = None,
69+
mode: Optional[Literal["w", "w-", "a"]] = None,
4370
append_dim: Optional[str] = None,
4471
region: Optional[Literal["auto"] | Mapping[str, Literal["auto"] | slice]] = None,
4572
validate_containers: bool = True,
@@ -58,6 +85,16 @@ def virtual_dataset_to_icechunk(
5885
Store to write the dataset to, which must not be read-only.
5986
group
6087
Path to the group in which to store the dataset, defaulting to the root group.
88+
mode
89+
How to handle a pre-existing group at the target path:
90+
91+
- ``"w-"``: create the group, raising a ``ContainsGroupError`` if it already exists.
92+
- ``"w"``: create the group, overwriting any existing contents at that path.
93+
- ``"a"``: open the group if it exists (keeping existing arrays), otherwise create it.
94+
- ``None`` (default): equivalent to ``"w-"``, unless ``append_dim`` or ``region``
95+
is given, in which case the existing group is opened.
96+
97+
``mode="w"`` and ``mode="w-"`` are incompatible with ``append_dim`` and ``region``.
6198
append_dim
6299
Name of the dimension along which to append data. If provided, the dataset must
63100
have a dimension with this name.
@@ -87,7 +124,6 @@ def virtual_dataset_to_icechunk(
87124
"""
88125
try:
89126
from icechunk import IcechunkStore # type: ignore[import-not-found]
90-
from zarr import Group # type: ignore[import-untyped]
91127
from zarr.storage import StorePath # type: ignore[import-untyped]
92128
except ImportError:
93129
raise ImportError(
@@ -104,6 +140,8 @@ def virtual_dataset_to_icechunk(
104140
f"group: expected type Optional[str], but got type {type(group)}"
105141
)
106142

143+
open_mode = _resolve_mode(mode, append_dim=append_dim, region=region)
144+
107145
if not isinstance(append_dim, (type(None), str)):
108146
raise TypeError(
109147
f"append_dim: expected type Optional[str], but got type {type(append_dim)}"
@@ -134,11 +172,9 @@ def virtual_dataset_to_icechunk(
134172
if validate_containers:
135173
validate_virtual_chunk_containers(store.session.config, [vds])
136174

137-
if append_dim or region:
138-
group_object = Group.open(store=store_path, zarr_format=3)
139-
else:
140-
# create the group if it doesn't already exist
141-
group_object = Group.from_store(store=store_path, zarr_format=3)
175+
group_object = open_group(
176+
store_path, mode=open_mode, zarr_format=3, use_consolidated=False
177+
)
142178

143179
write_virtual_dataset_to_icechunk_group(
144180
vds=vds,
@@ -154,6 +190,7 @@ def virtual_datatree_to_icechunk(
154190
vdt: xr.DataTree,
155191
store: "IcechunkStore",
156192
*,
193+
mode: Optional[Literal["w", "w-", "a"]] = None,
157194
write_inherited_coords: bool = False,
158195
validate_containers: bool = True,
159196
last_updated_at: datetime | None = None,
@@ -170,6 +207,13 @@ def virtual_datatree_to_icechunk(
170207
DataTree to write to an Icechunk store. Can contain both "virtual" variables (backed by ManifestArray objects) and "loadable" variables (backed by numpy arrays).
171208
store
172209
Store to write the dataset to, which must not be read-only.
210+
mode
211+
How to handle pre-existing groups at the target paths:
212+
213+
- ``"w-"`` or ``None`` (default): create each group, raising a
214+
``ContainsGroupError`` if it already exists.
215+
- ``"w"``: create each group, overwriting any existing contents at that path.
216+
- ``"a"``: open each group if it exists (keeping existing arrays), otherwise create it.
173217
write_inherited_coords
174218
If ``True``, replicate inherited coordinates on all descendant nodes of the
175219
tree. Otherwise, only write coordinates at the level at which they are
@@ -197,7 +241,6 @@ def virtual_datatree_to_icechunk(
197241
"""
198242
try:
199243
from icechunk import IcechunkStore # type: ignore[import-not-found]
200-
from zarr import Group # type: ignore[import-untyped]
201244
from zarr.storage import StorePath # type: ignore[import-untyped]
202245
except ImportError:
203246
raise ImportError(
@@ -209,6 +252,10 @@ def virtual_datatree_to_icechunk(
209252
f"store: expected type IcechunkStore, but got type {type(store)}"
210253
)
211254

255+
open_mode = _resolve_mode(
256+
mode, append_dim=kwargs.get("append_dim"), region=kwargs.get("region")
257+
)
258+
212259
if not isinstance(last_updated_at, (type(None), datetime)):
213260
raise TypeError(
214261
"last_updated_at: expected type datetime,"
@@ -238,12 +285,9 @@ def get_store_path(subtree, vdt) -> StorePath:
238285

239286
# TODO this serial loop could be slow writing lots of groups to high-latency store, see https://github.com/pydata/xarray/issues/9455
240287
for store_path, vds in paths_and_virtual_datasets:
241-
if kwargs.get("append_dim") or kwargs.get("region"):
242-
# both require the group (and arrays) to already exist
243-
group = Group.open(store=store_path, zarr_format=3)
244-
else:
245-
# create the group if it doesn't already exist
246-
group = Group.from_store(store=store_path, zarr_format=3)
288+
group = open_group(
289+
store_path, mode=open_mode, zarr_format=3, use_consolidated=False
290+
)
247291

248292
write_virtual_dataset_to_icechunk_group(
249293
vds=vds,

0 commit comments

Comments
 (0)