fix: informative errors for None in chunk specifications#236
Open
d-v-b wants to merge 7 commits into
Open
Conversation
…arr-developers#4165) * fix: byte-order handling for structured dtypes in the bytes codec (#220) * fix: byte-order handling for structured dtypes in the bytes codec The bytes codec neither byte-swapped structured-dtype fields to its configured endian on encode (numpy reports byteorder '|' for void dtypes, so the top-level byteorder comparison never detected a mismatch) nor honored its endian when decoding, silently corrupting any structured data whose field byte order differed from the stored one (e.g. virtual references to external big-endian data). Encode now detects byte-order mismatches by comparing full dtypes via newbyteorder, and decode reinterprets raw bytes in the stored byte order before converting to the data type's declared byte order, so the stored layout (codec state) and the in-memory layout (array data type) are independent. Closes zarr-developers#4141 Assisted-by: ClaudeCode:claude-fable-5 * test: fold structured byte-order cases into existing bytes codec tests Extend test_endian's parametrization with structured dtypes and test_bytes_codec_sync_roundtrip with endian/dtype parametrization plus stored-layout and decoded-dtype assertions, instead of adding parallel test functions for the same properties. Assisted-by: ClaudeCode:claude-fable-5 * refactor: rename stored_dtype to view_dtype in BytesCodec decode The variable is the dtype used to view the raw chunk bytes (byte order from the codec's endian configuration), not a property of the stored data or of the returned buffer, which always carries the array's declared dtype. Assisted-by: ClaudeCode:claude-fable-5 * docs: note that the decode-side byte-order conversion copies the chunk Assisted-by: ClaudeCode:claude-fable-5 * fix(store): FsspecStore.close() no longer closes the filesystem FsspecStore.close() closed the underlying filesystem's session, on the premise that a store built by from_url "owns" the filesystem it created. That premise does not hold: fsspec caches and shares filesystem instances across callers (its instance cache keys on storage options, not path), and users can hand one filesystem to many stores directly. Closing one store therefore killed the session that sibling stores were still using, and left the dead filesystem in fsspec's cache for later callers. Determining whether a filesystem is actually shared requires reaching into fsspec's private instance cache (_cache, _fs_token, cachable) and walking wrapper chains for caching/proxy filesystems — an implementation detail that leaks upward and that we would have to keep in sync with fsspec forever, getting it subtly wrong in between. The wrapper case alone (simplecache::/dir://) already slipped through a cache-membership check. The filesystem's lifecycle is simply not the store's to manage. This removes the ownership model added in the unreleased zarr-developersgh-4003: no _owns_fs, no _close_fs, no ownership transfer in with_read_only, and close() just marks the store not-open. The only thing given up is suppressing an "Unclosed client session" ResourceWarning, which was true anyway — the session belongs to a cached filesystem that outlives the store. Since zarr-developersgh-4003 never shipped (latest release is v3.2.1), its changelog fragment is removed rather than superseded. Assisted-by: ClaudeCode:claude-opus-4.8 * test: skip with_read_only fs test when AsyncFileSystemWrapper is absent test_with_read_only_shares_filesystem replaced an ownership test that carried a guard for fsspec < 2024.12.0, and the guard was dropped in the rewrite. The test still opens a file:// URL, which needs AsyncFileSystemWrapper, so it failed the min_deps job. Assisted-by: ClaudeCode:claude-opus-4.8 * docs: correct changelog claim about zarr-developersgh-4003 release status The fragment said zarr-developersgh-4003 was unreleased with no net change for released versions. Its text is already in the staged 3.3.0 release notes, so the revert is a real behavior change for anyone relying on close() releasing the session. Assisted-by: ClaudeCode:claude-opus-4.8 * docs: remove changelog entry for unreleased versions
Two pre-release fixes for chunk normalization error messages in 3.3.0:
- A per-dimension None chunk size (e.g. chunks=(None, 5)), which worked in
3.2.1 as "full extent for this dimension", previously crashed with an
uninformative TypeError ('NoneType' object is not iterable) from
normalize_chunks_1d. It now raises a ValueError directing the user to
the -1 sentinel, as promised by the zarr-developers#3899 release notes entry.
- zarr.create_array(..., chunks=None) raised a self-contradictory message
telling the user to pass chunks=None "from the top-level API". The
message now points at chunks="auto" or omitting the chunks argument.
Assisted-by: ClaudeCode:claude-fable-5
Per review on PR #236, normalize_chunks_1d and normalize_chunks_nd now take object and narrow with explicit isinstance/identity checks instead of growing an ad-hoc union annotation. Behavior changes: - A per-dimension bool chunk size (e.g. chunks=(True, 5)) is now rejected with an informative ValueError. Previously bool being a subclass of int let True through as a silent size-1 chunk — the exact behavior the zarr-developers#3899 release notes say was removed. - Strings/bytes and non-iterable values (e.g. chunks=2.5 or chunks=(2.5, 5)) now raise informative TypeErrors instead of bare crashes (list(2.5), len(generator)) or a misleading dimension-count error for whole-argument strings. - Generator inputs to normalize_chunks_nd are now materialized and accepted, both as the whole argument and as a per-dimension size list in rectilinear specs, consistent with numpy-style APIs. The type: ignore[call-overload] on int(c) is no longer needed after proper narrowing. Assisted-by: ClaudeCode:claude-fable-5
Owner
Author
|
🤖 AI text below 🤖 Pushed Behavior changes beyond the retyping:
Evidence: 🤖 Generated with Claude Code |
tests/test_api.py::test_create pinned the old bare TypeError message
('float' object is not iterable) that the chunk-normalizer refactor
deliberately replaced with an informative one. Match the new message.
Assisted-by: ClaudeCode:claude-fable-5
…izers The str/bytes rejection and the non-iterable rejection raised identical errors from separate branches in both normalize_chunks_1d and normalize_chunks_nd. Fold each pair into a single condition; str/bytes only need naming because they are iterable. Assisted-by: ClaudeCode:claude-fable-5
normalize_chunks_nd is a mechanical routine and should not refer to chunks="auto", which it does not itself accept. Its None/True rejection now states only what the normalizer expects; the guidance pointing users at chunks="auto" (or omitting the argument) is raised in init_array, the layer where auto-chunking is actually interpreted. Assisted-by: ClaudeCode:claude-fable-5
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Bug
Two pre-release problems in 3.3.0's chunk normalization (
src/zarr/core/chunk_grids.py), found in the 3.3.0 pre-release audit:Nonechunk size — e.g.zarr.create(store={}, shape=(10, 10), dtype='i4', chunks=(None, 5)), which on 3.2.1 meant "full extent for this dimension" — crashed with an uninformativeTypeError: 'NoneType' object is not iterablefromlist(chunks)innormalize_chunks_1d. The 3.3.0 release notes (refactor: simplify internal chunk representation zarr-developers/zarr-python#3899 entry) promise this raises an informative error.zarr.create_array(..., chunks=None)raisedValueError: None is not a valid chunk input. Use chunks=None or chunks="auto" from the top-level API...— self-contradictory, sincecreate_arrayis the top-level API and its auto-chunking sentinel is"auto".Fix
normalize_chunks_1drejectsNoneup front withValueError: None is not a valid chunk size for a dimension. Use -1 for a single chunk covering the full extent of an axis.This is the single chokepoint for every entry path (zarr.create,zarr.zeros,zarr.create_array, v2 and v3, shard shapes), all of which funnel throughnormalize_chunks_nd→normalize_chunks_1d.None/Truerejection innormalize_chunks_ndnow reads:... Use chunks="auto" or omit the chunks argument for automatic chunking, or pass an int / tuple of ints.— accurate forcreate_array(where"auto"is the default) and for the legacycreatepath (where omitting the argument auto-chunks).No changelog fragment: this fixes an unreleased regression and makes the existing 3.3.0 notes for zarr-developers#3899 accurate.
Evidence
tests/test_chunk_grids.py: 75 passed — includes new error-case tests for per-dimensionNonevianormalize_chunks_1d/normalize_chunks_ndand end-to-end viazarr.create/zarr.create_array, a test for the rewordedchunks=Nonemessage, and a parametrized test thatchunks=-1,(-1, -1),(-1, 5), and"auto"keep working.tests/test_array.py+tests/test_api: 1351 passed, 31 skipped (no regressions from the message rewording).mypyon the changed files: no issues.🤖 Generated with Claude Code