Consider Iterable[Iterable[int]] for ChunksLike instead of Sequence[Sequence[int]] #3869

Activity

1. chuckwondo commented on May 13, 2026

   

   chuckwondo

   on May 13, 2026

   Contributor

   More actions

   Without more context, generally speaking, I opt for Iterable over Sequence when there's a reasonable case for supporting laziness.

   I also noticed that the referenced comment is on a code block containing the following line:

   ChunksLike = ShapeLike | Sequence[Sequence[int]] | None

   This is tangential, but I strongly recommend against ever including None in a type alias. Doing so precludes using the alias in places where we want to require a value (i.e., forbid a None value), and forces inclusion of None checks that could otherwise be avoided.

   Instead, where we want to allow None, we annotate a param/var as ChunksLike | None.
2. maxrjones

   changed the title [-]Consider `Iterable[Iterable[int]]` for `ChunksLike` instead of `Sequence[Sequence[int]][/-] [+]Consider `Iterable[Iterable[int]]` for `ChunksLike` instead of `Sequence[Sequence[int]]`[/+] on May 13, 2026
3. maxrjones commented on May 13, 2026

   

   maxrjones

   on May 13, 2026

   MemberAuthor

   More actions

   Without more context, generally speaking, I opt for Iterable over Sequence when there's a reasonable case for supporting laziness.

   I also noticed that the referenced comment is on a code block containing the following line:

   ChunksLike = ShapeLike | Sequence[Sequence[int]] | None
   This is tangential, but I strongly recommend against ever including None in a type alias. Doing so precludes using the alias in places where we want to require a value (i.e., forbid a None value), and forces inclusion of None checks that could otherwise be avoided.

   Instead, where we want to allow None, we annotate a param/var as ChunksLike | None.

   this sounds good, thanks Chuck. I'd support you making both changes. If I remember correctly this change was deferred in the interest of speed rather than any substantive debate.
4. chuckwondo commented on May 13, 2026

   

   chuckwondo

   on May 13, 2026

   Contributor

   More actions

   Happy to pick this up. Does it make sense for ChunksLike to be typed as Iterable? At this point, I lack the familiarity to know if it makes sense to support laziness with "chunks-like" values. If not, then I would leave it as Sequence.

   @d-v-b, given this stems from a comment you made on the other issue, can you shed some light on your thinking in this regard?
5. d-v-b commented on May 13, 2026

   

   d-v-b

   on May 13, 2026

   Contributor

   More actions

   that would be great @chuckwondo! I totally agree that we want to leave None out of this kind of type alias. There are probably a few other places where we do this in the codebase, and I would love to clean them up.
6. chuckwondo

   self-assigned this

   on May 20, 2026
7. chuckwondo commented on May 20, 2026

   

   chuckwondo

   on May 20, 2026

   Contributor

   More actions

   @d-v-b, I ran into a mypy conundrum while working on this issue.

   I had to remove a None-check after removing None from the ChunksLike alias definition, which makes sense.

   However, I also found mypy complaining about a handful of other things completely unaffected by the change to the alias definition.

   At that point, I wondered why CI wasn't failing on those other, unrelated mypy errors. Here's what I discovered:

   In CI, when mypy is run via prek, it doesn't run with all extras installed. This causes different mypy results than when all extras are installed. This is because when all extras are installed, both typer and cast-value-rs are installed.

   When those 2 libs are not installed, there are a handful of "untyped" spots that mypy complains about, thus those spots currently include "type: ignore" comments to make mypy happy.

   However, when those extra deps are installed (my case -- when developing, I always install all groups and extras), everything is properly typed, so mypy complains about unused "type: ignore" comments. This means that pre-commit fails for me unless I remove those "type: ignore" comments.

   Of course, if I were to push such changes (i.e., the removed "ignores"), then CI would fail linting.

   Offhand, I'm not sure how best to handle this discrepancy:

   1. We could install all extras for linting in CI (allows us to remove the "type: ignore" comments in question), but this would then require that all devs locally install all extras in order for pre-commit to pass.
   2. We could find a way for mypy to succeed even without the extras, perhaps by adding more "ignores," but that might just move the discrepancy to other spots, leaving us still in the same conundrum. Maybe we can tweak the mypy config in pyproject.toml to deal with it.

   My preference is the first option, since I think that's what devs should do, and the second option may be too convoluted to achieve, if achievable at all, but I'm open to suggestion.

   In the meantime, I'm going to clean my venv to remove the extras so the mypy pre-commit hook succeeds (without needing to remove the "ignores" in question).
8. chuckwondo commented on May 20, 2026

   

   chuckwondo

   on May 20, 2026

   Contributor

   More actions

   @d-v-b, for the "devs should install all extras" option, we could pare this down to simply including "zarr[cast-array-rs,cli]" in the dev group. This would be the simplest and most robust way to eliminate the mypy discrepancy, but I don't know how you (or others) feel about doing that.
9. d-v-b commented on May 20, 2026

   

   d-v-b

   on May 20, 2026

   Contributor

   More actions

   I'm fine with the dev dependencies broadening here, but I would love to hear other perspectives. I wouldn't want to make changes that would somehow be problematic for other developers, although I dont see how that would happen with this change.
10. d-v-b

    closed this as completedin #3990on May 20, 2026
11. d-v-b

    added a commit that references this issue on May 20, 2026

    Widen ChunksLike type alias (#3990)

    ...

    This issue will close once commit 27abff2 is merged into the 'main' branch.

    27abff2