Skip to content

Commit 0b0bb08

Browse files
committed
fix: fused pipeline falls back for partial-mixin codecs without sync partial methods
The partial dispatch in FusedCodecPipeline.read_sync/write_sync asserted the private _decode_partial_sync/_encode_partial_sync methods, which only ShardingCodec implements. A codec advertising the public partial mixins (ArrayBytesCodecPartialDecodeMixin/-EncodeMixin) with only the documented async partial methods died with a bare AssertionError — or, under python -O, an AttributeError mid-IO. The asserts are now capability gates: codecs without the sync partial methods take the full-chunk sync path instead. The related crash for sharded arrays with async-only inner codecs is fixed separately in #4179. Assisted-by: ClaudeCode:claude-fable-5
1 parent 123268a commit 0b0bb08

3 files changed

Lines changed: 155 additions & 11 deletions

File tree

changes/254.bugfix.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fixed the opt-in `FusedCodecPipeline` for serializers that advertise the partial-decode/encode mixins with only the documented async partial methods: the partial dispatch previously asserted on the private `_decode_partial_sync`/`_encode_partial_sync` hooks (an `AssertionError`, or an `AttributeError` mid-IO under `python -O`); such codecs now take the full-chunk sync path.

src/zarr/core/codec_pipeline.py

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1039,10 +1039,14 @@ def read_sync(
10391039

10401040
# Partial-decode fast path: the AB codec owns IO (read only the
10411041
# byte ranges needed for the requested selection). Same condition
1042-
# and dispatch as BatchedCodecPipeline.read_batch.
1043-
if self.supports_partial_decode:
1044-
codec = self.array_bytes_codec
1045-
assert hasattr(codec, "_decode_partial_sync")
1042+
# and dispatch as BatchedCodecPipeline.read_batch, plus a gate on the
1043+
# sync partial method: the public partial-decode contract
1044+
# (`ArrayBytesCodecPartialDecodeMixin`) only requires the async
1045+
# `_decode_partial_single`, so a codec may support partial decode
1046+
# without `_decode_partial_sync` — such codecs take the full-chunk
1047+
# path below instead.
1048+
codec = self.array_bytes_codec
1049+
if self.supports_partial_decode and hasattr(codec, "_decode_partial_sync"):
10461050

10471051
def _read_one(
10481052
item: tuple[Any, ArraySpec, SelectorTuple, SelectorTuple, bool],
@@ -1111,10 +1115,14 @@ def write_sync(
11111115

11121116
# Partial-encode path: the AB codec owns IO (read, merge, encode,
11131117
# write). Same condition and calling convention as
1114-
# BatchedCodecPipeline.write_batch.
1115-
if self.supports_partial_encode:
1116-
codec = self.array_bytes_codec
1117-
assert hasattr(codec, "_encode_partial_sync")
1118+
# BatchedCodecPipeline.write_batch, plus a gate on the sync partial
1119+
# method: the public partial-encode contract
1120+
# (`ArrayBytesCodecPartialEncodeMixin`) only requires the async
1121+
# `_encode_partial_single`, so a codec may support partial encode
1122+
# without `_encode_partial_sync` — such codecs take the full-chunk
1123+
# path below instead.
1124+
codec = self.array_bytes_codec
1125+
if self.supports_partial_encode and hasattr(codec, "_encode_partial_sync"):
11181126
scalar = len(value.shape) == 0
11191127

11201128
def _write_one(

tests/test_fused_pipeline.py

Lines changed: 138 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,32 @@
22

33
from __future__ import annotations
44

5-
from typing import Any
5+
from dataclasses import dataclass, field, replace
6+
from typing import TYPE_CHECKING, Any
67

78
import numpy as np
89
import pytest
910

1011
import zarr
11-
from zarr.abc.codec import BytesBytesCodec
12+
from zarr.abc.codec import (
13+
ArrayBytesCodec,
14+
ArrayBytesCodecPartialDecodeMixin,
15+
ArrayBytesCodecPartialEncodeMixin,
16+
BytesBytesCodec,
17+
)
1218
from zarr.codecs.bytes import BytesCodec
1319
from zarr.codecs.gzip import GzipCodec
1420
from zarr.codecs.transpose import TransposeCodec
1521
from zarr.codecs.zstd import ZstdCodec
1622
from zarr.core.codec_pipeline import FusedCodecPipeline
1723
from zarr.core.config import config as zarr_config
24+
from zarr.registry import register_codec
1825
from zarr.storage import MemoryStore, StorePath
1926

27+
if TYPE_CHECKING:
28+
from zarr.core.array_spec import ArraySpec
29+
from zarr.core.buffer import Buffer, NDBuffer
30+
2031

2132
@pytest.mark.parametrize(
2233
"codecs",
@@ -261,7 +272,7 @@ def test_chunk_transform_uses_runtime_prototype() -> None:
261272
"""
262273
from zarr.abc.codec import BytesBytesCodec
263274
from zarr.core.array_spec import ArrayConfig, ArraySpec
264-
from zarr.core.buffer import Buffer, BufferPrototype, default_buffer_prototype
275+
from zarr.core.buffer import BufferPrototype, default_buffer_prototype
265276
from zarr.core.chunk_utils import ChunkTransform
266277
from zarr.core.dtype import get_data_type_from_native_dtype
267278

@@ -831,3 +842,127 @@ def test_async_decode_encode_passes_through_none_chunks() -> None:
831842
assert decoded[1] is None
832843
assert decoded[0] is not None
833844
np.testing.assert_array_equal(decoded[0].as_numpy_array(), data)
845+
846+
847+
# ---------------------------------------------------------------------------
848+
# Graceful fallback for partial-mixin codecs without private sync-partial hooks
849+
#
850+
# The public partial-decode/encode contract (`ArrayBytesCodecPartialDecodeMixin`
851+
# / `ArrayBytesCodecPartialEncodeMixin`) only requires the async
852+
# `_decode_partial_single` / `_encode_partial_single`. The fused pipeline must
853+
# route such codecs through its full-chunk sync path instead of asserting on
854+
# the private `_decode_partial_sync` / `_encode_partial_sync` hooks. The double
855+
# below is a minimal conforming implementer of that contract; it guards the
856+
# public extension API, so it must not grow the private sync-partial methods.
857+
# ---------------------------------------------------------------------------
858+
859+
860+
@dataclass(frozen=True)
861+
class PartialMixinCodec(
862+
ArrayBytesCodec, ArrayBytesCodecPartialDecodeMixin, ArrayBytesCodecPartialEncodeMixin
863+
):
864+
"""Serializer with sync whole-chunk methods plus ONLY async partial methods.
865+
866+
This is the pre-fused public contract for partial-capable codecs: the
867+
mixins' `_decode_partial_single` / `_encode_partial_single`. It must not
868+
implement `_decode_partial_sync` / `_encode_partial_sync`.
869+
"""
870+
871+
inner: BytesCodec = field(default_factory=BytesCodec)
872+
873+
@classmethod
874+
def from_dict(cls, data: dict[str, Any]) -> PartialMixinCodec:
875+
return cls()
876+
877+
def to_dict(self) -> dict[str, Any]:
878+
return {"name": "test-partial-mixin"}
879+
880+
def evolve_from_array_spec(self, array_spec: ArraySpec) -> PartialMixinCodec:
881+
return replace(self, inner=self.inner.evolve_from_array_spec(array_spec))
882+
883+
def compute_encoded_size(self, input_byte_length: int, chunk_spec: ArraySpec) -> int:
884+
return self.inner.compute_encoded_size(input_byte_length, chunk_spec)
885+
886+
def _decode_sync(self, chunk_bytes: Buffer, chunk_spec: ArraySpec) -> NDBuffer:
887+
return self.inner._decode_sync(chunk_bytes, chunk_spec)
888+
889+
def _encode_sync(self, chunk_array: NDBuffer, chunk_spec: ArraySpec) -> Buffer | None:
890+
return self.inner._encode_sync(chunk_array, chunk_spec)
891+
892+
async def _decode_single(self, chunk_bytes: Buffer, chunk_spec: ArraySpec) -> NDBuffer:
893+
return self._decode_sync(chunk_bytes, chunk_spec)
894+
895+
async def _encode_single(self, chunk_array: NDBuffer, chunk_spec: ArraySpec) -> Buffer | None:
896+
return self._encode_sync(chunk_array, chunk_spec)
897+
898+
async def _decode_partial_single(
899+
self, byte_getter: Any, selection: Any, chunk_spec: ArraySpec
900+
) -> NDBuffer | None:
901+
chunk_bytes = await byte_getter.get(prototype=chunk_spec.prototype)
902+
if chunk_bytes is None:
903+
return None
904+
return self._decode_sync(chunk_bytes, chunk_spec)[selection]
905+
906+
async def _encode_partial_single(
907+
self, byte_setter: Any, chunk_array: NDBuffer, selection: Any, chunk_spec: ArraySpec
908+
) -> None:
909+
existing = await byte_setter.get(prototype=chunk_spec.prototype)
910+
if existing is None:
911+
full = chunk_spec.prototype.nd_buffer.create(
912+
shape=chunk_spec.shape,
913+
dtype=chunk_spec.dtype.to_native_dtype(),
914+
fill_value=chunk_spec.fill_value,
915+
)
916+
else:
917+
full = self._decode_sync(existing, chunk_spec)
918+
full[selection] = chunk_array
919+
encoded = self._encode_sync(full, chunk_spec)
920+
assert encoded is not None
921+
await byte_setter.set(encoded)
922+
923+
924+
register_codec("test-partial-mixin", PartialMixinCodec)
925+
926+
_FUSED = {"codec_pipeline.path": "zarr.core.codec_pipeline.FusedCodecPipeline"}
927+
_BATCHED = {"codec_pipeline.path": "zarr.core.codec_pipeline.BatchedCodecPipeline"}
928+
929+
930+
@pytest.mark.filterwarnings("ignore::zarr.errors.UnstableSpecificationWarning")
931+
@pytest.mark.parametrize("dtype", ["uint8", "float64"])
932+
def test_partial_mixin_codec_async_partial_only_round_trip(dtype: str) -> None:
933+
"""A serializer advertising the partial mixins with only async partial
934+
methods must round-trip under the fused pipeline: full write, full read,
935+
partial read, partial write, plus cross-pipeline parity with
936+
BatchedCodecPipeline."""
937+
data = np.arange(64, dtype=dtype).reshape(8, 8)
938+
939+
with zarr_config.set(_FUSED):
940+
store = MemoryStore()
941+
arr = zarr.create_array(
942+
store,
943+
shape=(8, 8),
944+
chunks=(4, 4),
945+
dtype=dtype,
946+
serializer=PartialMixinCodec(),
947+
compressors=None,
948+
filters=None,
949+
fill_value=0,
950+
)
951+
952+
pipeline = arr._async_array.codec_pipeline
953+
assert isinstance(pipeline, FusedCodecPipeline)
954+
assert pipeline.supports_partial_decode
955+
assert pipeline.supports_partial_encode
956+
assert pipeline.sync_transform is not None
957+
958+
arr[:] = data
959+
np.testing.assert_array_equal(arr[:], data)
960+
np.testing.assert_array_equal(arr[1:5, 2:7], data[1:5, 2:7])
961+
962+
expected = data.copy()
963+
expected[2:6, 1:3] = 7
964+
arr[2:6, 1:3] = expected[2:6, 1:3]
965+
np.testing.assert_array_equal(arr[:], expected)
966+
967+
with zarr_config.set(_BATCHED):
968+
np.testing.assert_array_equal(zarr.open_array(store, mode="r")[:], expected)

0 commit comments

Comments
 (0)