Skip to content

Commit 3203018

Browse files
authored
test: give the sharded coordinate-selection write path real weight in test_oindex (#4331)
* test: give the sharded coordinate-selection write path real weight in test_oindex Add zarr.testing.strategies.sharded_arrays, a strategy that always yields a sharded v3 array (drawing chunk shape, shard shape, subchunk write order and inner codec chain), and draw it as a third arm in test_oindex and test_vindex. simple_arrays shards only a few percent of its draws, so under the derandomized ci profile test_oindex reached the ShardingCodec value-reshape guard (GH4284/GH4316) in 2 of 300 examples; it now reaches it in about 50 (13-27% of writes across random seeds), and the test fails without the guard. The sharded arm in test_oindex draws at least two dimensions, since the guard only applies to a value with two or more axes. A Hypothesis event at the write site reports the reach under --hypothesis-show-statistics. Narrow the docstring of test_set_selection_rejects_value_with_wrong_rank to the cases it pins: storage layout can change which writes are rejected, e.g. oindex[[3, 1], [0, 2]] with a (2, 2, 1) value is accepted chunked but raises sharded. Assisted-by: ClaudeCode:claude-fable-5-1 * chore: rename changelog fragment to the PR number Assisted-by: ClaudeCode:claude-fable-5-1 * test(strategies): let sharded_arrays nest one level of sharding Review asked for recursive sharding in the new strategy; one level is enough to reach the shard-within-shard paths, which is where the original GH4280 failure lived. Building it exposed that the strategy already nested by accident: passing `shards=` makes `create_array` wrap the drawn `ShardingCodec` in a second one with the same chunk shape, so every "sharded" draw was a shard of one-chunk inner shards and the drawn subchunk write order only ever reached the inner codec, never the outer. `sharded_arrays` now makes the shard the array's chunk grid and passes the drawn codec as the serializer, so a single-level draw really is single-level and the outer codec carries the drawn write order. A drawn (or `nested=`) flag then wraps that codec in another `ShardingCodec` whose chunk shape is an integral number of chunks and whose shard is an integral number of those. `_sharding_codecs` takes an optional inner codec chain to build the outer layer. (`arrays()` still uses the `shards=` form and so keeps the accidental one-chunk nesting; left alone here.) Measured on test_oindex under the ci profile: 24% of writes take the sharded coordinate-selection path, 16% of examples are nested, and the outer codec sees all four write orders instead of only `morton`. Assisted-by: ClaudeCode:claude-fable-5-1 * chore: strip trailing whitespace from the #3285 changelog fragment Introduced on main by #4330; it fails pre-commit.ci's trailing-whitespace hook on every PR that merges main from now on. Fixed here so this PR is green and main is fixed when it merges. Assisted-by: ClaudeCode:claude-fable-5-1
1 parent 9e5fcc1 commit 3203018

5 files changed

Lines changed: 134 additions & 16 deletions

File tree

changes/3285.feature.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
JSON metadata validation now delegates to ``msgspec.convert`` for the type
22
coercions it supports (``Literal`` membership, ``int`` / ``bool`` strictness,
33
list-to-tuple), replacing the per-field hand-written ``parse_*`` logic.
4-
User-defined attributes retain their existing JSON handling.
4+
User-defined attributes retain their existing JSON handling.
55
A latent generator-exhaustion bug in
66
``parse_storage_transformers`` is also fixed. See #3285.
77

changes/4331.misc.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Added `zarr.testing.strategies.sharded_arrays`, a Hypothesis strategy that always generates a sharded Zarr v3 array, drawing the chunk shape, shard shape, subchunk write order and inner codec chain, and — for half of its draws, or as selected by its `nested` argument — one level of recursive sharding, where the chunks are grouped into inner shards that are in turn grouped into the stored shards. The `test_oindex` and `test_vindex` property tests now draw it as a third arm alongside `simple_arrays` and `rectilinear_arrays`, so the sharding codec's write path for orthogonal selections with two or more array-indexed axes is exercised in tens of examples per run instead of about one.

src/zarr/testing/strategies.py

Lines changed: 104 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import itertools
22
import math
33
import sys
4-
from collections.abc import Callable, Mapping
4+
from collections.abc import Callable, Mapping, Sequence
55
from typing import Any, Literal
66

77
import hypothesis.extra.numpy as npst
@@ -12,6 +12,7 @@
1212
from hypothesis.strategies import SearchStrategy
1313

1414
import zarr
15+
from zarr.abc.codec import Codec
1516
from zarr.abc.store import (
1617
ByteRequest,
1718
OffsetByteRequest,
@@ -255,6 +256,30 @@ def shard_shapes(
255256
return tuple(m * c for m, c in zip(multiples, chunk_shape, strict=True))
256257

257258

259+
@st.composite
260+
def _sharding_codecs(
261+
draw: st.DrawFn,
262+
*,
263+
chunk_shape: tuple[int, ...],
264+
codecs: Sequence[Codec] | None = None,
265+
) -> ShardingCodec:
266+
"""A ``ShardingCodec`` over ``chunk_shape`` with a drawn subchunk write order.
267+
268+
The inner codec chain is drawn from ``sharding_inner_codecs`` unless ``codecs``
269+
is given, which lets a caller nest another ``ShardingCodec`` inside.
270+
"""
271+
subchunk_write_order = draw(subchunk_write_orders)
272+
inner_codecs: Sequence[Codec] = (
273+
draw(sharding_inner_codecs, label="sharding inner codecs") if codecs is None else codecs
274+
)
275+
return ShardingCodec(
276+
subchunk_write_order=subchunk_write_order,
277+
codecs=inner_codecs,
278+
index_codecs=[BytesCodec(), Crc32cCodec()],
279+
chunk_shape=chunk_shape,
280+
)
281+
282+
258283
@st.composite
259284
def np_array_and_chunks(
260285
draw: st.DrawFn,
@@ -334,14 +359,7 @@ def arrays(
334359
)
335360
event("sharded" if shard_shape is not None else "unsharded")
336361
if shard_shape is not None:
337-
subchunk_write_order = draw(subchunk_write_orders)
338-
inner_codecs = draw(sharding_inner_codecs, label="sharding inner codecs")
339-
serializer = ShardingCodec(
340-
subchunk_write_order=subchunk_write_order,
341-
codecs=inner_codecs,
342-
index_codecs=[BytesCodec(), Crc32cCodec()],
343-
chunk_shape=chunks_param,
344-
)
362+
serializer = draw(_sharding_codecs(chunk_shape=chunks_param))
345363
compressors_unsearched = None
346364
else:
347365
chunks_param = draw(chunk_shapes(shape=nparray.shape), label="chunk shape")
@@ -533,6 +551,83 @@ def rectilinear_arrays(
533551
return a
534552

535553

554+
# Sharded arrays need min_side >= 1: a shard must hold at least one chunk on every axis.
555+
_sharded_shapes = npst.array_shapes(max_dims=4, min_side=1, max_side=8)
556+
557+
558+
@st.composite
559+
def sharded_arrays(
560+
draw: st.DrawFn,
561+
*,
562+
shapes: st.SearchStrategy[tuple[int, ...]] = _sharded_shapes,
563+
nested: bool | None = None,
564+
) -> Any:
565+
"""Generate a zarr v3 array whose chunks are grouped into shards.
566+
567+
``arrays`` shards only a small fraction of its draws (a v3 array with a
568+
regular chunk grid, every axis larger than a chunk that is itself larger
569+
than 1, and then only half the time), so a property test that must
570+
exercise the sharding codec should draw from this strategy directly. Every
571+
draw is sharded: the chunk shape and the shard shape (an integral number of
572+
chunks per axis, possibly a single chunk) are drawn from ``shapes``, and
573+
the codec's subchunk write order and inner codec chain are drawn as in
574+
``arrays``. ``shapes`` must generate shapes with at least one element on
575+
every axis.
576+
577+
``nested`` selects one level of recursive sharding: the drawn chunks are
578+
grouped into inner shards, which are themselves grouped into the shards
579+
stored in the array, so the outer ``ShardingCodec`` wraps an inner one with
580+
its own subchunk write order. ``None`` (the default) draws it, so half the
581+
examples nest. For a nested array ``Array.chunks`` is the inner shard shape
582+
(the outer codec's chunk shape); the innermost chunk shape is the inner
583+
codec's ``chunk_shape``.
584+
"""
585+
shape = draw(shapes)
586+
chunk_shape = draw(chunk_shapes(shape=shape), label="chunk shape")
587+
serializer = draw(_sharding_codecs(chunk_shape=chunk_shape))
588+
nest = draw(st.booleans(), label="nested sharding") if nested is None else nested
589+
if nest:
590+
# Each level's shard is an integral number of the level below's chunks.
591+
codec_chunk_shape = draw(
592+
shard_shapes(shape=shape, chunk_shape=chunk_shape), label="inner shard shape"
593+
)
594+
serializer = draw(_sharding_codecs(chunk_shape=codec_chunk_shape, codecs=[serializer]))
595+
else:
596+
codec_chunk_shape = chunk_shape
597+
shard_shape = draw(
598+
shard_shapes(shape=shape, chunk_shape=codec_chunk_shape), label="shard shape"
599+
)
600+
event("nested sharding" if nest else "single-level sharding")
601+
602+
nparray = draw(numpy_arrays(shapes=st.just(shape)), label="array data")
603+
fill_value = draw(st.one_of([st.none(), npst.from_dtype(nparray.dtype)]))
604+
dim_names = draw(dimension_names(ndim=len(shape)), label="dimension names")
605+
606+
# The shard is the array's chunk grid and the drawn codec is its serializer.
607+
# Passing ``shards=`` instead would make ``create_array`` wrap the codec in a
608+
# second ``ShardingCodec`` of the same chunk shape, hiding the drawn write
609+
# order behind a default outer one.
610+
a = zarr.create_array(
611+
store=MemoryStore(),
612+
shape=shape,
613+
chunks=shard_shape,
614+
dtype=nparray.dtype,
615+
fill_value=fill_value,
616+
dimension_names=dim_names,
617+
serializer=serializer,
618+
filters=None,
619+
compressors=None,
620+
)
621+
assert a.shards == shard_shape
622+
assert a.chunks == codec_chunk_shape
623+
assert isinstance(a.metadata, ArrayV3Metadata)
624+
(codec,) = a.metadata.codecs
625+
assert isinstance(codec, ShardingCodec)
626+
assert codec.subchunk_write_order == serializer.subchunk_write_order
627+
a[:] = nparray
628+
return a
629+
630+
536631
def is_negative_slice(idx: Any) -> bool:
537632
return isinstance(idx, slice) and idx.step is not None and idx.step < 0
538633

tests/test_indexing.py

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2234,14 +2234,19 @@ def test_set_selection_rejects_value_with_wrong_rank(
22342234
shards: tuple[int, ...] | None,
22352235
pipeline_path: str,
22362236
) -> None:
2237-
"""A value whose rank does not fit the selection raises regardless of storage layout.
2237+
"""These wrong-rank values are rejected on chunked and sharded arrays alike.
22382238
22392239
The sharding codec re-derives an indexer from the selection it is handed
22402240
and ravels the value when it is the selection's broadcast shape minus
2241-
integer-indexed axes. Any other rank must fail on a sharded array exactly
2242-
as it does on a chunked one; an element count that happens to match is
2243-
not grounds to accept it. Only the rejection is asserted: a write that
2244-
fails inside the chunk merge may already have touched other chunks.
2241+
integer-indexed axes. The cases here pin that a matching element count
2242+
alone does not make the codec accept a value the chunked path rejects.
2243+
That is not a general law: storage layout can change which writes are
2244+
rejected. ``oindex[np.array([3, 1]), np.array([0, 2])]`` with a
2245+
``(2, 2, 1)`` value is accepted on a chunked ``(4, 4)`` array with
2246+
``(2, 2)`` chunks, because each chunk receives a ``(1, 1, 1)`` piece numpy
2247+
can broadcast, while the sharded array raises ``ValueError`` and numpy
2248+
rejects it outright. Only the rejection is asserted: a write that fails
2249+
inside the chunk merge may already have touched other chunks.
22452250
"""
22462251
a = np.zeros((4, 4), dtype=np.int32)
22472252
value = np.arange(np.prod(value_shape), dtype=np.int32).reshape(value_shape)

tests/test_properties.py

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515

1616
import hypothesis.extra.numpy as npst
1717
import hypothesis.strategies as st
18-
from hypothesis import assume, given, settings
18+
from hypothesis import assume, event, given, settings
1919

2020
from zarr.abc.store import Store
2121
from zarr.core.common import ZARR_JSON, ZARRAY_JSON, ZATTRS_JSON
@@ -31,6 +31,7 @@
3131
numpy_arrays,
3232
orthogonal_indices,
3333
rectilinear_arrays,
34+
sharded_arrays,
3435
simple_arrays,
3536
stores,
3637
zarr_formats,
@@ -158,10 +159,17 @@ async def test_basic_indexing_complex_rectilinear(data: st.DataObject) -> None:
158159
@pytest.mark.filterwarnings("ignore::zarr.core.dtype.common.UnstableSpecificationWarning")
159160
async def test_oindex(data: st.DataObject) -> None:
160161
# integer_array_indices can't handle 0-size dimensions.
162+
# A sharded array is drawn as its own arm: simple_arrays shards only a few
163+
# percent of its draws, and the sharding codec's write path for a selection
164+
# with two or more array-indexed axes (GH4284) needs real weight here. That
165+
# path only exists for a value with two or more axes, hence min_dims=2.
161166
zarray = data.draw(
162167
st.one_of(
163168
simple_arrays(shapes=npst.array_shapes(max_dims=4, min_side=1)),
164169
rectilinear_arrays(shapes=npst.array_shapes(max_dims=4, min_side=1, max_side=20)),
170+
sharded_arrays(
171+
shapes=npst.array_shapes(min_dims=2, max_dims=4, min_side=1, max_side=8)
172+
),
165173
)
166174
)
167175
nparray = zarray[:]
@@ -182,6 +190,14 @@ async def test_oindex(data: st.DataObject) -> None:
182190
# behaviour of setitem with repeated indices is not guaranteed in practice
183191
# Negative and positive spellings of the same index are duplicates too.
184192
assume(False)
193+
# The sharding codec sees a coordinate selection (the GH4284 path) when the
194+
# chunk selection has more than one array axis or drops an integer axis.
195+
n_array_axes = sum(isinstance(idxr, np.ndarray) for idxr in zindexer)
196+
coordinate_path = n_array_axes > 1 or any(isinstance(idxr, int) for idxr in zindexer)
197+
event(
198+
f"oindex write: {'sharded' if zarray.shards is not None else 'unsharded'}, "
199+
f"{'coordinate' if coordinate_path else 'orthogonal'} chunk selection"
200+
)
185201
new_data = data.draw(numpy_arrays(shapes=st.just(actual.shape), dtype=nparray.dtype))
186202
nparray[npindexer] = new_data
187203
zarray.oindex[zindexer] = new_data
@@ -198,6 +214,7 @@ async def test_vindex(data: st.DataObject) -> None:
198214
st.one_of(
199215
simple_arrays(shapes=npst.array_shapes(max_dims=4, min_side=1)),
200216
rectilinear_arrays(shapes=npst.array_shapes(max_dims=3, min_side=1, max_side=20)),
217+
sharded_arrays(),
201218
)
202219
)
203220
nparray = zarray[:]

0 commit comments

Comments
 (0)