Skip to content

Surface mode in vz.to_icechunk, keeping current defaults#1035

Open
aaronspring wants to merge 2 commits into
zarr-developers:mainfrom
aaronspring:surface-mode-to-icechunk
Open

Surface mode in vz.to_icechunk, keeping current defaults#1035
aaronspring wants to merge 2 commits into
zarr-developers:mainfrom
aaronspring:surface-mode-to-icechunk

Conversation

@aaronspring

@aaronspring aaronspring commented Jul 8, 2026

Copy link
Copy Markdown

Closes #1001.

Exposes mode in vz.to_icechunk

claude

What this does

Adds a mode parameter to vds.vz.to_icechunk and dt.vz.to_icechunk (and the underlying virtual_dataset_to_icechunk / virtual_datatree_to_icechunk writers) controlling how a pre-existing group at the target path is handled:

  • "w-": create the group, raising ContainsGroupError if it already exists — the previous and still-default behaviour (mode=None).
  • "w": create the group, overwriting any existing contents at that path.
  • "a": open the group if it exists (keeping existing arrays), otherwise create it. This enables e.g. splitting a large virtual dataset across commits by variable, and writing a parent group after a child group — both scenarios from vz.to_icechunk throws ContainsGroupError when group already exists and doesn't surface mode #1001.

mode="w"/"w-" combined with append_dim or region raises a ValueError, since those require opening an existing group.

Implementation

Internally, a _resolve_mode helper validates mode and resolves it to the effective zarr group-open mode ("r+" when append_dim/region are given), and both writers now open groups through a single unconditional zarr.open_group(...) call instead of branching between Group.open and Group.from_store.

A side effect of that unification: writing a virtual DataTree with region/append_dim forwarded to each node previously always failed with ContainsGroupError (every node group was unconditionally created); existing groups are now opened instead, and the previously-xfailing test_write_datatree_region passes, so its xfail marker is removed.

Tests & docs

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.91%. Comparing base (e1ebb4b) to head (d787844).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1035      +/-   ##
==========================================
+ Coverage   89.87%   89.91%   +0.04%     
==========================================
  Files          36       36              
  Lines        2202     2211       +9     
==========================================
+ Hits         1979     1988       +9     
  Misses        223      223              
Files with missing lines Coverage Δ
virtualizarr/accessor.py 95.74% <ø> (ø)
virtualizarr/writers/icechunk.py 92.00% <100.00%> (+0.43%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aaronspring and others added 2 commits July 8, 2026 11:34
virtual_datatree_to_icechunk unconditionally created each node's group
with Group.from_store, so forwarding region or append_dim (which require
the group and its arrays to already exist) always failed with
ContainsGroupError. Mirror the branch virtual_dataset_to_icechunk
already had: open the existing group for region/append_dim, create it
otherwise.

Removes the now-passing xfail on test_write_datatree_region, whose
stated reason (an xarray region limitation) was a misdiagnosis of this
ContainsGroupError.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 zarr-developers#1001.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aaronspring aaronspring force-pushed the surface-mode-to-icechunk branch from 397e065 to d787844 Compare July 8, 2026 09:36
@aaronspring

Copy link
Copy Markdown
Author

Split the DataTree region/append_dim ContainsGroupError fix out into #1036, since it's an independent bug and backportable on its own. This PR is now stacked on top of #1036 — until that merges, the diff here includes its one commit; once #1036 lands on main, this PR reduces to the mode feature only.

@aaronspring

Copy link
Copy Markdown
Author

CI note: the 3 red build jobs (test-py312, test-py313, minimum-versions) fail only on test_parsers/test_grib.py (test_grib_virtual_dataset, test_grib_only_variables, test_grib_chunk_decodes_via_codec) — the GRIB/gribberish parser returning empty data_vars / a NodeTypeValidationError. These are pre-existing and unrelated to this PR: it touches only the icechunk writer/accessor and doesn't go near GRIB, and all 560 other tests pass (including the new mode tests). The same 3 failures show up on #1034 (a pre-commit.ci autoupdate that changes no source), #1032, #1033, and on scheduled main runs. codecov/* is just coverage thresholds, not test failures.

Comment thread virtualizarr/accessor.py

- ``"w-"``: create the group, raising a ``ContainsGroupError`` if it already exists.
- ``"w"``: create the group, overwriting any existing contents at that path.
- ``"a"``: open the group if it exists (keeping existing arrays), otherwise create it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I also allow r+ if region and a-?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vz.to_icechunk throws ContainsGroupError when group already exists and doesn't surface mode

1 participant