-
-
Notifications
You must be signed in to change notification settings - Fork 434
fix: informative errors for None in chunk specifications #4177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9bfe855
16b4ff2
273dd73
df56f7e
6a83a11
4d57b1b
27d8b3b
45a1b2c
4203be6
f254c77
f053f45
a9c59b6
8470404
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4461,7 +4461,17 @@ async def init_array( | |||||||||||||
| "chunks=(inner_size, ...), shards=[[shard_sizes], ...]" | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| # Normalize the user's chunks into canonical ChunksTuple form | ||||||||||||||
| # Normalize the user's chunks into canonical ChunksTuple form. | ||||||||||||||
| # Auto-chunking is an API-level concept, so the guidance toward it is | ||||||||||||||
| # raised here rather than in the mechanical normalizer. Validate through | ||||||||||||||
| # an object-typed view: None/True are outside ChunksLike but reachable | ||||||||||||||
| # from untyped callers. | ||||||||||||||
| chunks_input: object = chunks | ||||||||||||||
| if chunks_input is None or chunks_input is True: | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Perhaps we can actually verify if the value is valid or not
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude suggested something like this. def _is_valid_chunks(value):
if value == "auto":
return True
if isinstance(value, int) and not isinstance(value, bool):
return True
if isinstance(value, tuple) and all(
isinstance(v, int) and not isinstance(v, bool) for v in value
):
return True
return False
if not _is_valid_chunks(chunks_input):
raise ValueError(
f'{chunks_input!r} is not a valid chunk input. Use chunks="auto" or omit the chunks '
"argument for automatic chunking, or pass an int / tuple of ints."
)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already have type narrowing routines for this. we just need to handle |
||||||||||||||
| raise ValueError( | ||||||||||||||
| f'{chunks!r} is not a valid chunk input. Use chunks="auto" or omit the chunks ' | ||||||||||||||
| "argument for automatic chunking, or pass an int / iterable of ints." | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| if chunks == "auto": | ||||||||||||||
| max_bytes = None if shards is None else SHARDED_INNER_CHUNK_MAX_BYTES | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if
chunks_input is False?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than checking for a few invalid values, should the check be if chunks is either "auto", an int, or a tuple of ints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunks=false is valid for 2.x compatibility. it creates a single chunk for the whole array 🙃
that leaves out iterables of ints, numpy arrays, etc. we have lower-level normalization for that.