From f1e9fa917f8fbfca1cd8e3b6028a0defcda6ccfb Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Thu, 23 Jul 2026 19:58:16 -0300 Subject: [PATCH 01/10] Pin issue-from-pytest-log until upstream bug is fixed --- .github/workflows/python-upstream.yaml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/python-upstream.yaml b/.github/workflows/python-upstream.yaml index d1bb3fbc3..593ddcfbd 100644 --- a/.github/workflows/python-upstream.yaml +++ b/.github/workflows/python-upstream.yaml @@ -219,7 +219,10 @@ jobs: - name: Generate and publish the pytest report if: needs.build.outputs.pytest-outcome == 'failure' - uses: xarray-contrib/issue-from-pytest-log@87351a8f864e969567cda22a25a2f214cbe2340f # v1.6.0 + # Pinned to the last working commit: newer versions never write ISSUE_BODY to + # GITHUB_OUTPUT, so the issue is never created/updated. Unpin once + # https://github.com/scientific-python/issue-from-pytest-log-action/pull/74 is merged. + uses: xarray-contrib/issue-from-pytest-log@8e905db353437cda1d6a773de245343fbfc940dd # v1.6.0 with: log-path: icechunk-python/output-pytest-log.jsonl @@ -294,7 +297,10 @@ jobs: path: xarray-backends-logs - name: Generate and publish the xarray backends report - uses: xarray-contrib/issue-from-pytest-log@87351a8f864e969567cda22a25a2f214cbe2340f # v1.6.0 + # Pinned to the last working commit: newer versions never write ISSUE_BODY to + # GITHUB_OUTPUT, so the issue is never created/updated. Unpin once + # https://github.com/scientific-python/issue-from-pytest-log-action/pull/74 is merged. + uses: xarray-contrib/issue-from-pytest-log@8e905db353437cda1d6a773de245343fbfc940dd # v1.6.0 with: log-path: xarray-backends-logs/output-pytest-log.jsonl issue-title: "Nightly Xarray backends tests failed" From 1e65b00d9e9503b8021dc7e494af3b1a410af337 Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Thu, 23 Jul 2026 19:58:59 -0300 Subject: [PATCH 02/10] alternative solution to #2175 --- .../test_zarr/test_store/test_icechunk_store.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/icechunk-python/tests/test_zarr/test_store/test_icechunk_store.py b/icechunk-python/tests/test_zarr/test_store/test_icechunk_store.py index 36945cef0..e22345992 100644 --- a/icechunk-python/tests/test_zarr/test_store/test_icechunk_store.py +++ b/icechunk-python/tests/test_zarr/test_store/test_icechunk_store.py @@ -7,7 +7,6 @@ import pytest -import zarr from icechunk import IcechunkError, IcechunkStore, local_filesystem_storage from icechunk.repository import Repository from zarr.abc.store import OffsetByteRequest, RangeByteRequest, Store, SuffixByteRequest @@ -546,7 +545,8 @@ async def test_getsize_raises(self, store: IcechunkStore) -> None: await store.getsize("not-a-real-key") @pytest.mark.skipif( - zarr.__version__ < "3.1.6", reason="Store._get_bytes added in zarr 3.1.6" + not hasattr(Store, "_get_bytes"), + reason="Store._get_bytes only exists in zarr >=3.1.6,<3.3", ) async def test_get_bytes(self, store: IcechunkStore) -> None: # Override: icechunk validates metadata on zarr.json keys, @@ -561,7 +561,8 @@ async def test_get_bytes(self, store: IcechunkStore) -> None: ) @pytest.mark.skipif( - zarr.__version__ < "3.1.6", reason="Store._get_bytes_sync added in zarr 3.1.6" + not hasattr(Store, "_get_bytes_sync"), + reason="Store._get_bytes_sync only exists in zarr >=3.1.6,<3.3", ) def test_get_bytes_sync(self, store: IcechunkStore) -> None: # Override: icechunk validates metadata on zarr.json keys @@ -571,7 +572,8 @@ def test_get_bytes_sync(self, store: IcechunkStore) -> None: assert store._get_bytes_sync(key, prototype=default_buffer_prototype()) == data @pytest.mark.skipif( - zarr.__version__ < "3.1.6", reason="Store._get_json added in zarr 3.1.6" + not hasattr(Store, "_get_json"), + reason="Store._get_json only exists in zarr >=3.1.6,<3.3", ) async def test_get_json(self, store: IcechunkStore) -> None: # Override: icechunk validates metadata on zarr.json keys, @@ -583,7 +585,8 @@ async def test_get_json(self, store: IcechunkStore) -> None: assert await store._get_json(key, prototype=default_buffer_prototype()) == data @pytest.mark.skipif( - zarr.__version__ < "3.1.6", reason="Store._get_json_sync added in zarr 3.1.6" + not hasattr(Store, "_get_json_sync"), + reason="Store._get_json_sync only exists in zarr >=3.1.6,<3.3", ) def test_get_json_sync(self, store: IcechunkStore) -> None: # Override: icechunk validates metadata on zarr.json keys From 1a862a87559ba9b044218d62dfb98710378d2004 Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Thu, 23 Jul 2026 20:58:58 -0300 Subject: [PATCH 03/10] Fix shift_array stateful rule to use the storage-key chunk grid cdata_shape counts inner chunks for sharded arrays, but store keys and Session.shift_array operate on the outer (shard) grid. zarr main's strategies now generate sharded arrays, so the mismatched granularity made the model shuffle keys over a grid that doesn't exist (nightly upstream-dev failure in #1921). --- .../tests/test_zarr/test_stateful.py | 37 ++++++++++++------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/icechunk-python/tests/test_zarr/test_stateful.py b/icechunk-python/tests/test_zarr/test_stateful.py index b305b2772..f055847a9 100644 --- a/icechunk-python/tests/test_zarr/test_stateful.py +++ b/icechunk-python/tests/test_zarr/test_stateful.py @@ -34,6 +34,24 @@ Frequency = TypeVar("Frequency", bound=Callable[..., Any]) + +def storage_chunk_sizes(arr: Array) -> tuple[tuple[int, ...], ...]: + """Per-dimension sizes of the storage-key chunk grid (shards when sharded). + + Store keys and icechunk's shift_array operate on this grid, not on the + inner (read) chunks that cdata_shape/read_chunk_sizes count for sharded + arrays. Older zarr lacks write_chunk_sizes but also lacks rectilinear + grids, so cells there are regular and can be computed directly. + """ + if hasattr(arr, "write_chunk_sizes"): + return arr.write_chunk_sizes # type: ignore[no-any-return] + cell = arr.shards or arr.chunks + return tuple( + tuple(min(c, s - i * c) for i in range(-(-s // c))) + for s, c in zip(arr.shape, cell, strict=True) + ) + + # pytestmark = [ # pytest.mark.filterwarnings( # "ignore::zarr.core.dtype.common.UnstableSpecificationWarning" @@ -266,7 +284,8 @@ def shift_array(self, data: st.DataObject) -> None: arr_model = zarr.open_array(self.model, path=array_path) arr_store = zarr.open_array(self.store, path=array_path) - num_chunks = arr_model.cdata_shape + grid_sizes = storage_chunk_sizes(arr_model) + num_chunks = tuple(len(sizes) for sizes in grid_sizes) # Draw offset: negative shifts left, positive shifts right offset = data.draw( @@ -290,26 +309,18 @@ def shift_array(self, data: st.DataObject) -> None: # - Without resize: data shifting beyond bounds is lost should_resize = data.draw(st.booleans()) if should_resize and any(o > 0 for o in offset): - # read_chunk_sizes was added alongside rectilinear support; on older - # zarr it doesn't exist but grids are always regular, so synthesize - # the same tuple-of-tuples shape from the declared chunk size. - if hasattr(arr_model, "read_chunk_sizes"): - read_sizes = arr_model.read_chunk_sizes - else: - read_sizes = tuple( - (c,) * n for c, n in zip(arr_model.chunks, num_chunks, strict=True) - ) # Shifting right by offset[i] pushes the last offset[i] chunks # past the original extent; grow by their sizes so they fit. new_shape = tuple( arr_model.shape[i] - + (sum(read_sizes[i][-offset[i] :]) if offset[i] > 0 else 0) - for i in range(len(read_sizes)) + + (sum(grid_sizes[i][-offset[i] :]) if offset[i] > 0 else 0) + for i in range(len(grid_sizes)) ) note(f"resizing array '{array_path}' from {arr_model.shape} to {new_shape}") arr_model.resize(new_shape) arr_store.resize(new_shape) - num_chunks = arr_model.cdata_shape + grid_sizes = storage_chunk_sizes(arr_model) + num_chunks = tuple(len(sizes) for sizes in grid_sizes) note(f"shifting array '{array_path}' by {offset}") self.store.session.shift_array(f"/{array_path}", offset) From 5a7099b7f74a6eb6b72750ce384e17cd92282a36 Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Thu, 23 Jul 2026 20:59:25 -0300 Subject: [PATCH 04/10] Guard create_branch/create_tag against V1 expired-ancestor chains V1 expiration rewrites only ref-reachable chains, so an unreachable commit's on-disk ancestry can still pass through expired snapshots the model dropped. Creating a ref there resurrects history the model no longer tracks, failing check_ancestry. Apply the same full-ancestry assume() that reset_branch already uses. --- .../tests/test_stateful_repo_ops.py | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/icechunk-python/tests/test_stateful_repo_ops.py b/icechunk-python/tests/test_stateful_repo_ops.py index 529848439..e407c158c 100644 --- a/icechunk-python/tests/test_stateful_repo_ops.py +++ b/icechunk-python/tests/test_stateful_repo_ops.py @@ -930,8 +930,14 @@ def create_branch(self, name: str, commit: str) -> str: note(f"Creating branch {name!r} for commit {commit!r}") # V1 expired snapshots stay on disk so create_branch succeeds, but - # the model no longer tracks their store contents. - assume(not (self.model.spec_version == 1 and commit not in self.model.commits)) + # the model no longer tracks their store contents. This also covers a + # commit whose ancestry *reaches* an expired-but-on-disk snapshot: the + # real ancestry resurrects it while the model has popped it, so skip + # unless the whole chain is still modelled. + if self.model.spec_version == 1: + assume(commit in self.model.commits) + ancestry = {s.id for s in self.repo.ancestry(snapshot_id=commit)} + assume(ancestry <= set(self.model.commits)) # we can create a tag and branch with the same name if name not in self.model.branch_heads and commit in self.model.commits: @@ -950,8 +956,14 @@ def create_branch(self, name: str, commit: str) -> str: def create_tag(self, name: str, commit_id: str) -> str: note(f"Creating tag {name!r} for commit {commit_id!r}") # V1 expired snapshots stay on disk so create_tag succeeds, but - # the model no longer tracks their store contents. - assume(not (self.model.spec_version == 1 and commit_id not in self.model.commits)) + # the model no longer tracks their store contents. This also covers a + # commit whose ancestry *reaches* an expired-but-on-disk snapshot: the + # real ancestry resurrects it while the model has popped it, so skip + # unless the whole chain is still modelled. + if self.model.spec_version == 1: + assume(commit_id in self.model.commits) + ancestry = {s.id for s in self.repo.ancestry(snapshot_id=commit_id)} + assume(ancestry <= set(self.model.commits)) if ( name in self.model.created_tags or name in self.model.tags From eb8cf6e401ad1c533dd4ca09793d864f4f731d80 Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Fri, 24 Jul 2026 10:13:41 -0300 Subject: [PATCH 05/10] Match model GC re-parenting to Rust's nearest-surviving-ancestor rule The model re-parented kept snapshots whose parent was GC'd to the initial snapshot, but delete_snapshots_from_repo_info re-parents over the deleted run to the nearest surviving ancestor. After reset_branch onto such a snapshot the two disagreed on reachability, so the next garbage_collect predicted the wrong snapshots_deleted count (nightly upstream-dev failure in #1921). --- icechunk-python/tests/test_stateful_repo_ops.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/icechunk-python/tests/test_stateful_repo_ops.py b/icechunk-python/tests/test_stateful_repo_ops.py index e407c158c..222ff9ecb 100644 --- a/icechunk-python/tests/test_stateful_repo_ops.py +++ b/icechunk-python/tests/test_stateful_repo_ops.py @@ -556,6 +556,7 @@ def garbage_collect( if created_at_by_id[k] < older_than } + new_parents: dict[str, str | None] = {} if self.spec_version >= 2: # GC re-parents a kept snapshot whose parent run was deleted to its # nearest surviving ancestor, harvesting the deleted ancestors' tx @@ -576,6 +577,7 @@ def garbage_collect( harvested.extend(self.pruned_ancestor_tx_logs.get(x, [])) harvested.append(x) self.pruned_ancestor_tx_logs[cid] = harvested + new_parents[cid] = anc for k in deleted: self.commits.pop(k, None) @@ -583,11 +585,14 @@ def garbage_collect( self.pruned_ancestor_tx_logs.pop(k, None) if self.spec_version >= 2: - # V2's delete_snapshots_from_repo_info rewrites parent pointers - # for kept snapshots whose parent was GC'd. - for c in self.commits.values(): - if c.parent_id is not None and c.parent_id in deleted: - c.parent_id = self.initial_snapshot_id + # V2's delete_snapshots_from_repo_info re-parents a kept snapshot + # whose parent was GC'd to its nearest surviving ancestor (the + # harvest walk's endpoint), falling back to the initial snapshot + # when the whole chain was deleted. + for cid, anc in new_parents.items(): + self.commits[cid].parent_id = ( + anc if anc is not None else self.initial_snapshot_id + ) else: # V1 doesn't rewrite parent pointers, so commits whose parents # were GC'd have broken ancestry and are effectively unusable. From 0e2dc47c57a5975c803c481808e4f160749f0933 Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Fri, 24 Jul 2026 10:17:32 -0300 Subject: [PATCH 06/10] Add regression tests for shift_array storage-key grid handling Cover storage_chunk_sizes on sharded and plain arrays, and a deterministic model-vs-store shift of a sharded array mirroring the shift_array stateful rule. --- .../tests/test_zarr/test_stateful.py | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/icechunk-python/tests/test_zarr/test_stateful.py b/icechunk-python/tests/test_zarr/test_stateful.py index f055847a9..529df05f1 100644 --- a/icechunk-python/tests/test_zarr/test_stateful.py +++ b/icechunk-python/tests/test_zarr/test_stateful.py @@ -4,6 +4,7 @@ from typing import Any, TypeVar import hypothesis.strategies as st +import numpy as np import pytest from hypothesis import note from hypothesis.stateful import ( @@ -28,6 +29,7 @@ from icechunk.testing.utils import update_paths_after_move from zarr import Array from zarr.core.buffer import default_buffer_prototype +from zarr.core.sync import collect_aiterator, sync from zarr.testing.stateful import ZarrHierarchyStateMachine, split_prefix_name PROTOTYPE = default_buffer_prototype() @@ -409,3 +411,59 @@ def test_zarr_store() -> None: # run_state_machine_as_test( # mk_test_instance_sync, settings=settings(report_multiple_bugs=False) # ) + + +def test_storage_chunk_sizes_granularity() -> None: + model = ModelStore() + sharded = zarr.create_array( + model, name="s", shape=(4,), shards=(4,), chunks=(2,), dtype="i1", fill_value=1 + ) + plain = zarr.create_array( + model, name="p", shape=(100, 80), chunks=(30, 40), dtype="i1" + ) + assert storage_chunk_sizes(sharded) == ((4,),) + # cdata_shape counts inner chunks for sharded arrays; using it as the + # storage-key grid is the bug storage_chunk_sizes exists to avoid. + assert sharded.cdata_shape == (2,) + assert storage_chunk_sizes(plain) == ((30, 30, 30, 10), (40, 40)) + + +def test_shift_sharded_model_vs_store() -> None: + """Shifting a sharded array keeps ModelStore and IcechunkStore keys in sync. + + Mirrors the shift_array rule: the array is generated sharded on the model + and recreated on the store with only the outer chunk grid, then both are + shifted on the storage-key grid. + """ + repo = Repository.create(in_memory_storage(), spec_version=2) + session = repo.writable_session("main") + store = session.store + + model = ModelStore() + zarr.group(store=model) + + arr_model = zarr.create_array( + model, name="0", shape=(4,), shards=(4,), chunks=(2,), dtype="i1", fill_value=1 + ) + arr_store = zarr.create_array( + store, name="0", shape=(4,), chunks=(4,), dtype="i1", fill_value=1 + ) + data = np.zeros((4,), dtype="i1") + arr_model[:] = data + arr_store[:] = data + + model_keys = sorted(collect_aiterator(model.list_prefix(""))) + store_keys = sorted(collect_aiterator(store.list_prefix(""))) + assert model_keys == store_keys, (model_keys, store_keys) + assert "0/c/0" in model_keys + + grid_sizes = storage_chunk_sizes(arr_model) + num_chunks = tuple(len(sizes) for sizes in grid_sizes) + assert num_chunks == (1,) + + session.shift_array("/0", (1,)) + sync(model.shift_array("0", (1,), num_chunks)) + + model_keys = sorted(collect_aiterator(model.list_prefix(""))) + store_keys = sorted(collect_aiterator(store.list_prefix(""))) + assert model_keys == store_keys, (model_keys, store_keys) From e22251d86d4bbcb6eb0b7f54f1b1afb6a3ab3df4 Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Fri, 24 Jul 2026 10:31:19 -0300 Subject: [PATCH 07/10] Bump issue-from-pytest-log to v1.6.1 with the ISSUE_BODY output fix --- .github/workflows/python-upstream.yaml | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/.github/workflows/python-upstream.yaml b/.github/workflows/python-upstream.yaml index 593ddcfbd..11ba9120c 100644 --- a/.github/workflows/python-upstream.yaml +++ b/.github/workflows/python-upstream.yaml @@ -219,10 +219,7 @@ jobs: - name: Generate and publish the pytest report if: needs.build.outputs.pytest-outcome == 'failure' - # Pinned to the last working commit: newer versions never write ISSUE_BODY to - # GITHUB_OUTPUT, so the issue is never created/updated. Unpin once - # https://github.com/scientific-python/issue-from-pytest-log-action/pull/74 is merged. - uses: xarray-contrib/issue-from-pytest-log@8e905db353437cda1d6a773de245343fbfc940dd # v1.6.0 + uses: xarray-contrib/issue-from-pytest-log@054799b34bd75a5fd6c86277a4a8a575224e60c6 # v1.6.1 with: log-path: icechunk-python/output-pytest-log.jsonl @@ -297,10 +294,7 @@ jobs: path: xarray-backends-logs - name: Generate and publish the xarray backends report - # Pinned to the last working commit: newer versions never write ISSUE_BODY to - # GITHUB_OUTPUT, so the issue is never created/updated. Unpin once - # https://github.com/scientific-python/issue-from-pytest-log-action/pull/74 is merged. - uses: xarray-contrib/issue-from-pytest-log@8e905db353437cda1d6a773de245343fbfc940dd # v1.6.0 + uses: xarray-contrib/issue-from-pytest-log@054799b34bd75a5fd6c86277a4a8a575224e60c6 # v1.6.1 with: log-path: xarray-backends-logs/output-pytest-log.jsonl issue-title: "Nightly Xarray backends tests failed" From 9ea73133f54a51b808f2a8a3630d6450a213010a Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Fri, 24 Jul 2026 12:18:56 -0300 Subject: [PATCH 08/10] fix compat_test --- .../tests/test_stateful_repo_ops.py | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/icechunk-python/tests/test_stateful_repo_ops.py b/icechunk-python/tests/test_stateful_repo_ops.py index 222ff9ecb..c96120d59 100644 --- a/icechunk-python/tests/test_stateful_repo_ops.py +++ b/icechunk-python/tests/test_stateful_repo_ops.py @@ -930,19 +930,29 @@ def checkout_branch(self, ref: str) -> None: note(f"Expecting error when checking out branch {ref!r}") self.repo.writable_session(ref) + def _v1_chain_fully_modelled(self, commit: str) -> bool: + """True when a commit's full on-disk ancestry is still in the model. + + V1 expiration rewrites only ref-reachable chains, so an unreachable + commit's on-disk ancestry can still pass through expired snapshots the + model dropped — or, after GC deletes their files, become unreadable + entirely. Refs must not be created or reset onto such chains. + """ + try: + chain = {s.id for s in self.repo.ancestry(snapshot_id=commit)} + except IcechunkError: + return False + return chain <= set(self.model.commits) + @rule(name=ref_name_text, commit=commits, target=branches) def create_branch(self, name: str, commit: str) -> str: note(f"Creating branch {name!r} for commit {commit!r}") # V1 expired snapshots stay on disk so create_branch succeeds, but - # the model no longer tracks their store contents. This also covers a - # commit whose ancestry *reaches* an expired-but-on-disk snapshot: the - # real ancestry resurrects it while the model has popped it, so skip - # unless the whole chain is still modelled. + # the model no longer tracks their store contents. if self.model.spec_version == 1: assume(commit in self.model.commits) - ancestry = {s.id for s in self.repo.ancestry(snapshot_id=commit)} - assume(ancestry <= set(self.model.commits)) + assume(self._v1_chain_fully_modelled(commit)) # we can create a tag and branch with the same name if name not in self.model.branch_heads and commit in self.model.commits: @@ -961,14 +971,10 @@ def create_branch(self, name: str, commit: str) -> str: def create_tag(self, name: str, commit_id: str) -> str: note(f"Creating tag {name!r} for commit {commit_id!r}") # V1 expired snapshots stay on disk so create_tag succeeds, but - # the model no longer tracks their store contents. This also covers a - # commit whose ancestry *reaches* an expired-but-on-disk snapshot: the - # real ancestry resurrects it while the model has popped it, so skip - # unless the whole chain is still modelled. + # the model no longer tracks their store contents. if self.model.spec_version == 1: assume(commit_id in self.model.commits) - ancestry = {s.id for s in self.repo.ancestry(snapshot_id=commit_id)} - assume(ancestry <= set(self.model.commits)) + assume(self._v1_chain_fully_modelled(commit_id)) if ( name in self.model.created_tags or name in self.model.tags @@ -1006,14 +1012,10 @@ def discard_changes(self) -> None: @rule(branch=branches, commit=commits) def reset_branch(self, branch: str, commit: str) -> None: # V1 expired snapshots stay on disk so reset_branch would succeed, - # but modelling that divergence isn't worthwhile — just skip. This also - # covers resetting to a commit whose ancestry *reaches* an expired-but- - # on-disk snapshot: the real ancestry resurrects it while the model has - # popped it, so skip unless the whole chain is still modelled. + # but modelling that divergence isn't worthwhile — just skip. if self.model.spec_version == 1: assume(commit in self.model.commits) - ancestry = {s.id for s in self.repo.ancestry(snapshot_id=commit)} - assume(ancestry <= set(self.model.commits)) + assume(self._v1_chain_fully_modelled(commit)) if branch not in self.model.branch_heads or commit not in self.model.commits: note(f"resetting branch {branch}, expecting error.") with pytest.raises(IcechunkError): From a249452e09105a91073cf7dd5a1d52dfc7437454 Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Fri, 24 Jul 2026 12:31:26 -0300 Subject: [PATCH 09/10] fix mypy (zarr 3.1.0 import compatibility) --- icechunk-python/tests/test_zarr/test_stateful.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/icechunk-python/tests/test_zarr/test_stateful.py b/icechunk-python/tests/test_zarr/test_stateful.py index 529df05f1..eca37e5f4 100644 --- a/icechunk-python/tests/test_zarr/test_stateful.py +++ b/icechunk-python/tests/test_zarr/test_stateful.py @@ -37,7 +37,7 @@ Frequency = TypeVar("Frequency", bound=Callable[..., Any]) -def storage_chunk_sizes(arr: Array) -> tuple[tuple[int, ...], ...]: +def storage_chunk_sizes(arr: "Array[Any]") -> tuple[tuple[int, ...], ...]: """Per-dimension sizes of the storage-key chunk grid (shards when sharded). Store keys and icechunk's shift_array operate on this grid, not on the From d2bf9267912117f1d35e1c8fd432dac960ac62dd Mon Sep 17 00:00:00 2001 From: Luiz Irber Date: Fri, 24 Jul 2026 17:05:40 -0300 Subject: [PATCH 10/10] address Ian's comments --- .../tests/test_zarr/test_stateful.py | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/icechunk-python/tests/test_zarr/test_stateful.py b/icechunk-python/tests/test_zarr/test_stateful.py index eca37e5f4..369eec5fa 100644 --- a/icechunk-python/tests/test_zarr/test_stateful.py +++ b/icechunk-python/tests/test_zarr/test_stateful.py @@ -29,7 +29,6 @@ from icechunk.testing.utils import update_paths_after_move from zarr import Array from zarr.core.buffer import default_buffer_prototype -from zarr.core.sync import collect_aiterator, sync from zarr.testing.stateful import ZarrHierarchyStateMachine, split_prefix_name PROTOTYPE = default_buffer_prototype() @@ -40,10 +39,14 @@ def storage_chunk_sizes(arr: "Array[Any]") -> tuple[tuple[int, ...], ...]: """Per-dimension sizes of the storage-key chunk grid (shards when sharded). - Store keys and icechunk's shift_array operate on this grid, not on the - inner (read) chunks that cdata_shape/read_chunk_sizes count for sharded - arrays. Older zarr lacks write_chunk_sizes but also lacks rectilinear - grids, so cells there are regular and can be computed directly. + Store keys are one object per *write* chunk: with sharding, the whole + shard is a single ``c/`` object and read (inner) chunks are byte + ranges inside it, never separate keys. Shifts move store keys, so + offsets and the grid must be in write-chunk units; + ``cdata_shape``/``read_chunk_sizes`` count inner chunks and give the + wrong grid for sharded arrays. Older zarr lacks write_chunk_sizes but + also lacks rectilinear grids, so cells there are regular and can be + computed directly. """ if hasattr(arr, "write_chunk_sizes"): return arr.write_chunk_sizes # type: ignore[no-any-return] @@ -428,7 +431,7 @@ def test_storage_chunk_sizes_granularity() -> None: assert storage_chunk_sizes(plain) == ((30, 30, 30, 10), (40, 40)) -def test_shift_sharded_model_vs_store() -> None: +async def test_shift_sharded_model_vs_store() -> None: """Shifting a sharded array keeps ModelStore and IcechunkStore keys in sync. Mirrors the shift_array rule: the array is generated sharded on the model @@ -452,8 +455,8 @@ def test_shift_sharded_model_vs_store() -> None: arr_model[:] = data arr_store[:] = data - model_keys = sorted(collect_aiterator(model.list_prefix(""))) - store_keys = sorted(collect_aiterator(store.list_prefix(""))) + model_keys = sorted([k async for k in model.list_prefix("")]) + store_keys = sorted([k async for k in store.list_prefix("")]) assert model_keys == store_keys, (model_keys, store_keys) assert "0/c/0" in model_keys @@ -462,8 +465,8 @@ def test_shift_sharded_model_vs_store() -> None: assert num_chunks == (1,) session.shift_array("/0", (1,)) - sync(model.shift_array("0", (1,), num_chunks)) + await model.shift_array("0", (1,), num_chunks) - model_keys = sorted(collect_aiterator(model.list_prefix(""))) - store_keys = sorted(collect_aiterator(store.list_prefix(""))) + model_keys = sorted([k async for k in model.list_prefix("")]) + store_keys = sorted([k async for k in store.list_prefix("")]) assert model_keys == store_keys, (model_keys, store_keys)