Skip to content

Commit 123268a

Browse files
authored
fix: FusedCodecPipeline falls back to async path for sharded arrays with async-only inner codecs (#4179)
ShardingCodec structurally satisfies SupportsSyncCodec, but its sync methods delegate to the configured inner and index codec chains — so a shard whose inner or index chain contains a codec implementing only the async codec interface passed the fused pipeline's sync gate and then raised TypeError mid-IO in ChunkTransform construction. Sync capability is now answered by _codec_supports_sync, which combines the structural protocol check with a per-instance _sync_capable opt-out (absent means capable). ShardingCodec reports False when any codec in its inner or index chain is not sync-capable (recursively, so a nested shard propagates its opt-out outward), which makes ChunkTransform construction raise at pipeline evolve time and the pipeline decline the sync fast path — such arrays route through the async paths, matching BatchedCodecPipeline. Fully sync-capable chains keep the fast path. Closes #4178 Assisted-by: ClaudeCode:claude-fable-5
1 parent 401e597 commit 123268a

5 files changed

Lines changed: 135 additions & 4 deletions

File tree

changes/4179.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 sharded arrays whose inner or index codec chain contains a codec implementing only the async codec interface (no `SupportsSyncCodec`). Such arrays previously raised `TypeError: All codecs must implement SupportsSyncCodec` on both read and write; the pipeline now declines its synchronous fast path for them and falls back to the async path, matching the behavior of the default `BatchedCodecPipeline`. Fully sync-capable codec chains keep the fast path unchanged.

src/zarr/abc/codec.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,20 @@ def _decode_sync(self, chunk_data: CO, chunk_spec: ArraySpec) -> CI: ...
8282
def _encode_sync(self, chunk_data: CI, chunk_spec: ArraySpec) -> CO | None: ...
8383

8484

85+
def _codec_supports_sync(codec: object) -> bool:
86+
"""Whether `codec` can actually run on a synchronous (no event loop) path.
87+
88+
Structural membership in `SupportsSyncCodec` is necessary but not always
89+
sufficient: a codec can provide `_decode_sync`/`_encode_sync` whose ability
90+
to run depends on runtime configuration the type system cannot see.
91+
`ShardingCodec` is the canonical case — its sync methods delegate to its
92+
configured inner and index codec chains, so they only work when every codec
93+
in those chains is itself sync-capable. Such codecs opt out dynamically via
94+
a `_sync_capable` attribute/property (absent means capable).
95+
"""
96+
return isinstance(codec, SupportsSyncCodec) and getattr(codec, "_sync_capable", True)
97+
98+
8599
class BaseCodec[CI: CodecInput, CO: CodecOutput](Metadata):
86100
"""Generic base class for codecs.
87101

src/zarr/codecs/sharding.py

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
ArrayBytesCodecPartialEncodeMixin,
1515
Codec,
1616
CodecPipeline,
17-
SupportsSyncCodec,
17+
_codec_supports_sync,
1818
)
1919
from zarr.abc.store import (
2020
ByteGetter,
@@ -1402,8 +1402,30 @@ def _is_complete_shard_write(
14021402
is_complete_chunk for *_, is_complete_chunk in indexed_chunks
14031403
)
14041404

1405+
@property
1406+
def _sync_capable(self) -> bool:
1407+
"""Dynamic opt-out consulted by `_codec_supports_sync` / `ChunkTransform`.
1408+
1409+
This codec structurally satisfies `SupportsSyncCodec`, but every sync
1410+
method (`_decode_sync`, `_encode_sync`, `_decode_partial_sync`,
1411+
`_encode_partial_sync`) delegates to the inner and index codec chains
1412+
through `ChunkTransform`, so it can only run synchronously when every
1413+
codec in BOTH chains is itself sync-capable. Reporting False here makes
1414+
`ChunkTransform` construction raise, which in turn makes
1415+
`FusedCodecPipeline.evolve_from_array_spec` set `sync_transform=None` —
1416+
the whole pipeline then declines the sync fast path and routes through
1417+
the async paths (partial shard decode / async fallback write), exactly
1418+
as it does for an async-only TOP-level codec or a non-sync store.
1419+
"""
1420+
return self._inner_codecs_sync_capable() and self._index_codecs_sync_capable()
1421+
1422+
def _inner_codecs_sync_capable(self) -> bool:
1423+
# _codec_supports_sync (not bare isinstance) so a nested sharding codec
1424+
# with an async-only inner chain propagates its opt-out outward.
1425+
return all(_codec_supports_sync(c) for c in self.codecs)
1426+
14051427
def _index_codecs_sync_capable(self) -> bool:
1406-
return all(isinstance(c, SupportsSyncCodec) for c in self.index_codecs)
1428+
return all(_codec_supports_sync(c) for c in self.index_codecs)
14071429

14081430
async def _decode_shard_index(
14091431
self, index_bytes: Buffer, chunks_per_shard: tuple[int, ...]

src/zarr/core/chunk_utils.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from dataclasses import dataclass, field
44
from typing import TYPE_CHECKING, Any, cast
55

6-
from zarr.abc.codec import GetResult, SupportsSyncCodec
6+
from zarr.abc.codec import GetResult, SupportsSyncCodec, _codec_supports_sync
77
from zarr.core.indexing import is_scalar
88

99
if TYPE_CHECKING:
@@ -240,7 +240,13 @@ class ChunkTransform:
240240
def __post_init__(self) -> None:
241241
from zarr.core.codec_pipeline import codecs_from_list
242242

243-
non_sync = [c for c in self.codecs if not isinstance(c, SupportsSyncCodec)]
243+
# _codec_supports_sync, not a bare isinstance check: a codec can satisfy
244+
# the SupportsSyncCodec protocol structurally yet be unable to run
245+
# synchronously (ShardingCodec whose inner/index chain contains an
246+
# async-only codec). Such codecs opt out via `_sync_capable`, and the
247+
# TypeError here is what makes FusedCodecPipeline.evolve_from_array_spec
248+
# decline the sync fast path and fall back to the async pipeline.
249+
non_sync = [c for c in self.codecs if not _codec_supports_sync(c)]
244250
if non_sync:
245251
names = ", ".join(type(c).__name__ for c in non_sync)
246252
raise TypeError(

tests/test_fused_pipeline.py

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import pytest
99

1010
import zarr
11+
from zarr.abc.codec import BytesBytesCodec
1112
from zarr.codecs.bytes import BytesCodec
1213
from zarr.codecs.gzip import GzipCodec
1314
from zarr.codecs.transpose import TransposeCodec
@@ -643,6 +644,93 @@ def spy_write_sync(self: Any, *args: Any, **kwargs: Any) -> Any:
643644
)
644645

645646

647+
# ---------------------------------------------------------------------------
648+
# Async-only codecs inside a shard's inner codec chain
649+
# ---------------------------------------------------------------------------
650+
651+
652+
class _AsyncOnlyNoopCodec(BytesBytesCodec): # type: ignore[misc,unused-ignore]
653+
"""A no-op BB codec implementing ONLY the async codec interface.
654+
655+
Deliberately does NOT satisfy `SupportsSyncCodec` (no `_decode_sync` /
656+
`_encode_sync`), modelling a third-party codec that predates the sync
657+
protocol. Class-level counters prove the codec actually ran.
658+
"""
659+
660+
is_fixed_size = True
661+
encode_calls = 0
662+
decode_calls = 0
663+
664+
def to_dict(self) -> dict[str, Any]:
665+
return {"name": "test-async-only-noop", "configuration": {}}
666+
667+
@classmethod
668+
def from_dict(cls, data: dict[str, Any]) -> _AsyncOnlyNoopCodec:
669+
return cls()
670+
671+
def compute_encoded_size(self, input_byte_length: int, _spec: Any) -> int:
672+
return input_byte_length
673+
674+
async def _encode_single(self, chunk_bytes: Any, chunk_spec: Any) -> Any:
675+
type(self).encode_calls += 1
676+
return chunk_bytes
677+
678+
async def _decode_single(self, chunk_bytes: Any, chunk_spec: Any) -> Any:
679+
type(self).decode_calls += 1
680+
return chunk_bytes
681+
682+
683+
def test_sharded_roundtrip_with_async_only_inner_codec() -> None:
684+
"""A sharded array whose INNER codec chain contains an async-only codec
685+
round-trips under FusedCodecPipeline (full write, partial write, full read,
686+
partial read).
687+
688+
Regression: the pipeline's top-level guard (evolve_from_array_spec ->
689+
sync_transform=None) only inspected the top-level chain. ShardingCodec
690+
structurally satisfies SupportsSyncCodec, so a sync transform was built and
691+
the sync fast path dove into ShardingCodec's sync shard paths, which raised
692+
TypeError from the inner ChunkTransform. The pipeline must instead decline
693+
the sync fast path and fall back to the async inner pipeline, like
694+
BatchedCodecPipeline.
695+
"""
696+
_AsyncOnlyNoopCodec.encode_calls = 0
697+
_AsyncOnlyNoopCodec.decode_calls = 0
698+
699+
with zarr_config.set({"codec_pipeline.path": "zarr.core.codec_pipeline.FusedCodecPipeline"}):
700+
store = MemoryStore()
701+
arr = zarr.create_array(
702+
store=store,
703+
shape=(16, 16),
704+
shards=(8, 8),
705+
chunks=(4, 4),
706+
dtype="int32",
707+
compressors=[_AsyncOnlyNoopCodec()],
708+
fill_value=-1,
709+
)
710+
assert isinstance(arr._async_array.codec_pipeline, FusedCodecPipeline)
711+
712+
data = np.arange(256, dtype="int32").reshape(16, 16)
713+
arr[:] = data # full write
714+
np.testing.assert_array_equal(arr[:], data) # full read
715+
np.testing.assert_array_equal(arr[2:11, 3:14], data[2:11, 3:14]) # partial read
716+
717+
arr[5:7, 5:13] = 0 # partial write (read-merge-write of existing shards)
718+
data[5:7, 5:13] = 0
719+
np.testing.assert_array_equal(arr[:], data)
720+
721+
assert _AsyncOnlyNoopCodec.encode_calls > 0, "async-only inner codec never encoded"
722+
assert _AsyncOnlyNoopCodec.decode_calls > 0, "async-only inner codec never decoded"
723+
724+
# The stored bytes are valid for the default pipeline too: read them back
725+
# under BatchedCodecPipeline (default codec_pipeline.path). Opening from
726+
# metadata needs the codec name in the registry.
727+
from zarr.registry import register_codec
728+
729+
register_codec("test-async-only-noop", _AsyncOnlyNoopCodec)
730+
reread = zarr.open_array(store=store, mode="r")
731+
np.testing.assert_array_equal(reread[:], data)
732+
733+
646734
# ---------------------------------------------------------------------------
647735
# AsyncChunkTransform: the async per-chunk codec chain used on the async
648736
# fallback path. It is the async mirror of ChunkTransform, so it must produce

0 commit comments

Comments
 (0)