Skip to content

Commit 944dbd6

Browse files
committed
fix(sharding): address adversarial-review findings on clipped grids
Fixes from a multi-agent adversarial review of the semi-regular grid change: - nchunks_initialized used floor division for chunks-per-shard, which undercounts on clipped grids and reports 0 when chunk_shape exceeds the shard shape; use ceiling division to match the inner grid - the scalar-broadcast write memo in _encode_partial_sync gated on ArraySpec object identity, which never holds because _get_chunk_spec is uncached (#3054); gate on shape equality so the memo fires again (one encode instead of one per complete chunk) - the rectilinear clipping test used shard edges [[6, 4]], which normalize to a regular grid; use [[4, 6]] with the rectilinear config flag so RectilinearChunkGridMetadata is actually exercised Adds regression tests for the first two. Assisted-by: ClaudeCode:claude-fable-5
1 parent 45c67bc commit 944dbd6

3 files changed

Lines changed: 76 additions & 13 deletions

File tree

src/zarr/codecs/sharding.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -763,7 +763,6 @@ def _encode_partial_sync(
763763
"""
764764
shard_shape = shard_spec.shape
765765
chunks_per_shard = self._get_chunks_per_shard(shard_spec)
766-
nominal_chunk_spec = self._get_chunk_spec(shard_spec)
767766
get_chunk_spec = self._make_chunk_spec_getter(shard_spec)
768767
inner_transform = self._get_inner_chunk_transform(shard_spec)
769768

@@ -818,7 +817,7 @@ def _encode_partial_sync(
818817

819818
for chunk_coords, chunk_sel, out_sel, is_complete_chunk in indexer:
820819
chunk_spec = get_chunk_spec(chunk_coords)
821-
if is_scalar and is_complete_chunk and chunk_spec is nominal_chunk_spec:
820+
if is_scalar and is_complete_chunk and chunk_spec.shape == self.chunk_shape:
822821
if scalar_complete_result is _sentinel:
823822
scalar_complete_result = merge_and_encode_chunk(
824823
None,
@@ -1510,9 +1509,12 @@ def _make_chunk_spec_getter(
15101509
15111510
Inner chunks clipped by the shard boundary are encoded at their clipped
15121511
shape, so their spec differs from the nominal `chunk_shape` spec.
1513-
Chunks with the nominal shape all share one spec object, which keeps
1514-
per-spec caches downstream (e.g. `ChunkTransform._resolve_specs`)
1515-
effective and makes `spec is nominal_spec` a valid uniformity check.
1512+
Within one getter, chunks with the nominal shape all share one spec
1513+
object, which keeps per-spec caches downstream (e.g.
1514+
`ChunkTransform._resolve_specs`) effective. Note that `_get_chunk_spec`
1515+
is not cached (see #3054), so the nominal spec here is NOT the same
1516+
object as one obtained from a separate `_get_chunk_spec` call — callers
1517+
must compare specs by shape, not identity.
15161518
"""
15171519
nominal = self._get_chunk_spec(shard_spec)
15181520
if self._is_evenly_divided(shard_spec.shape):

src/zarr/core/array.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5345,8 +5345,10 @@ async def _nchunks_initialized(
53455345
if array.shards is None:
53465346
chunks_per_shard = 1
53475347
else:
5348+
# Inner chunks are clipped by the shard shape, so the count per shard
5349+
# is the ceiling division of the shard shape by the chunk shape.
53485350
chunks_per_shard = product(
5349-
tuple(a // b for a, b in zip(array.shards, array.chunks, strict=True))
5351+
tuple(ceildiv(a, b) for a, b in zip(array.shards, array.chunks, strict=True))
53505352
)
53515353
return (await _nshards_initialized(array)) * chunks_per_shard
53525354

tests/test_codecs/test_sharding.py

Lines changed: 66 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -887,17 +887,27 @@ def test_sharding_clipped_inner_chunks_byte_layout() -> None:
887887

888888
def test_sharding_clipped_inner_chunks_rectilinear() -> None:
889889
"""A rectilinear outer grid whose shard edges are not divisible by the inner
890-
chunk shape round-trips: the inner grid is clipped per shard."""
891-
arr = zarr.create_array(
892-
{}, shape=(10,), shards=[[6, 4]], chunks=(4,), dtype="uint8", fill_value=0
893-
)
890+
chunk shape round-trips: the inner grid is clipped per shard.
891+
892+
Shard edges `[4, 6]` cannot be normalized to a regular grid (the larger edge
893+
comes last), so this exercises `RectilinearChunkGridMetadata` for real —
894+
unlike e.g. `[6, 4]`, which coincides with a clipped regular grid of size 6.
895+
"""
896+
from zarr.core.metadata.v3 import RectilinearChunkGridMetadata
897+
898+
with zarr.config.set({"array.rectilinear_chunks": True}):
899+
arr = zarr.create_array(
900+
{}, shape=(10,), shards=[[4, 6]], chunks=(4,), dtype="uint8", fill_value=0
901+
)
902+
assert isinstance(arr.metadata, ArrayV3Metadata)
903+
assert isinstance(arr.metadata.chunk_grid, RectilinearChunkGridMetadata)
894904
data = np.arange(10, dtype="uint8")
895905
arr[...] = data
896906
assert np.array_equal(arr[...], data)
897-
assert arr.read_chunk_sizes == ((4, 2, 4),)
907+
assert arr.read_chunk_sizes == ((4, 4, 2),)
898908

899-
arr[5:9] = 42
900-
data[5:9] = 42
909+
arr[3:9] = 42
910+
data[3:9] = 42
901911
assert np.array_equal(arr[...], data)
902912

903913

@@ -935,6 +945,55 @@ def test_sharding_clipped_inner_chunk_sizes() -> None:
935945
assert arr.nchunks == 6
936946

937947

948+
def test_sharding_clipped_nchunks_initialized() -> None:
949+
"""`nchunks_initialized` counts ceiling-division inner chunks per shard.
950+
951+
Regression test: it previously used floor division, which undercounts for
952+
clipped grids and reports 0 when the inner chunk shape exceeds the shard
953+
shape.
954+
"""
955+
arr = zarr.create_array({}, shape=(16,), shards=(8,), chunks=(3,), dtype="uint8", fill_value=0)
956+
arr[...] = np.arange(16, dtype="uint8")
957+
# 2 shards, each holding ceil(8/3) = 3 inner chunks
958+
assert arr.nchunks_initialized == 6
959+
960+
arr2 = zarr.create_array({}, shape=(11,), shards=(4,), chunks=(9,), dtype="uint8", fill_value=0)
961+
arr2[...] = np.arange(11, dtype="uint8")
962+
# 3 shards, each holding one inner chunk clipped to the shard shape
963+
assert arr2.nchunks_initialized == 3
964+
965+
966+
def test_sharding_scalar_write_memoizes_complete_chunks(monkeypatch: pytest.MonkeyPatch) -> None:
967+
"""A scalar broadcast partial write encodes the repeated complete inner
968+
chunk once, not once per chunk.
969+
970+
Regression test: the memo gate must compare chunk specs by shape — an
971+
object-identity gate never fires because `_get_chunk_spec` is uncached.
972+
"""
973+
from zarr.core import chunk_utils
974+
975+
calls = 0
976+
real_encode = chunk_utils.ChunkTransform.encode_chunk
977+
978+
def counting_encode(self: Any, *args: Any, **kwargs: Any) -> Any:
979+
nonlocal calls
980+
calls += 1
981+
return real_encode(self, *args, **kwargs)
982+
983+
with zarr.config.set({"codec_pipeline.path": "zarr.core.codec_pipeline.FusedCodecPipeline"}):
984+
arr = zarr.create_array(
985+
{}, shape=(100,), shards=(100,), chunks=(10,), dtype="uint8", fill_value=0
986+
)
987+
monkeypatch.setattr(chunk_utils.ChunkTransform, "encode_chunk", counting_encode)
988+
arr[5:] = 7
989+
expected = np.full(100, 7, dtype="uint8")
990+
expected[:5] = 0
991+
assert np.array_equal(arr[...], expected)
992+
# one merge for the partial edge chunk, one memoized encode shared by the
993+
# nine complete chunks, one shard-index encode
994+
assert calls <= 4, f"scalar write encoded {calls} times; memo is not firing"
995+
996+
938997
@pytest.mark.parametrize("store", ["local"], indirect=["store"])
939998
def test_sharding_mixed_integer_list_indexing(store: Store) -> None:
940999
"""Regression test for https://github.com/zarr-developers/zarr-python/issues/3691.

0 commit comments

Comments
 (0)