Parametrize Array with v2/v3 metadata

[dstansby]

It has been a longstanding bugbear of mine that there's no easy way to specify a v2 or v3 array type. This especially came up in the context of #3257, which deals specifically with v2/v3 arrays.

This PR ads type parametrization to the Array class.

After this PR, there is lots of improvements to adding overloads to functions and methods that could be made, but to keep review easier I'd like to leave that for a follow up PR.

[codecov]

Codecov Report

❌ Patch coverage is 29.11392% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.95%. Comparing base (54dcede) to head (7f802ff).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/zarr/core/group.py	4.54%	21 Missing ⚠️
src/zarr/api/asynchronous.py	0.00%	12 Missing ⚠️
src/zarr/api/synchronous.py	0.00%	10 Missing ⚠️
src/zarr/core/indexing.py	0.00%	6 Missing ⚠️
src/zarr/core/array.py	20.00%	4 Missing ⚠️
src/zarr/core/attributes.py	0.00%	1 Missing ⚠️
src/zarr/core/metadata/__init__.py	0.00%	1 Missing ⚠️
src/zarr/core/sync_group.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3304      +/-   ##
==========================================
+ Coverage   61.89%   61.95%   +0.06%     
==========================================
  Files          85       86       +1     
  Lines       10153    10170      +17     
==========================================
+ Hits         6284     6301      +17     
  Misses       3869     3869              

Files with missing lines	Coverage Δ
src/zarr/metadata/migrate_v3.py	98.36% <100.00%> (+0.01%)	⬆️
src/zarr/testing/strategies.py	97.82% <100.00%> (+0.01%)	⬆️
src/zarr/types.py	100.00% <100.00%> (ø)
src/zarr/core/attributes.py	46.15% <0.00%> (ø)
src/zarr/core/metadata/__init__.py	0.00% <0.00%> (ø)
src/zarr/core/sync_group.py	42.85% <0.00%> (ø)
src/zarr/core/array.py	68.48% <20.00%> (-0.04%)	⬇️
src/zarr/core/indexing.py	69.46% <0.00%> (ø)
src/zarr/api/synchronous.py	36.61% <0.00%> (ø)
src/zarr/api/asynchronous.py	72.20% <0.00%> (ø)
... and 1 more

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[d-v-b]

these imports will prevent zarr.core.array from using anything defined in zarr.types, is that intentional?

[d-v-b]

(outside of a type-checking context, I mean)

[d-v-b]

thinking more about this, I think as long as everything in zarr.types is imported exclusively in if TYPE_CHECKING blocks, then there's no risk of circular imports. we would only have circular imports if we tried to work with types as values outside of type-checking, for example using get_args(ZarrFormat). We can always avoid this by defining a typed constant value somewhere else in the codebase.

[dstansby]

I certinaly didn't think about this! So neither intentional or unintentional. I'm not sure I see a better solution, but open to suggestions.

[d-v-b]

What was the problem that necessitated defining type aliases in the top-level types file? I wonder if there's another way to solve that. This is my only concern about this PR btw, we can get it in soon after this is resolved

[dstansby]

I think there wasn't a problem, and this was a choice 😄 - the circular import mess seems a good reason to move these out and next to the array/group classes. Might be a little while until I find time, so someone else is very free to finish this PR off if I don't get there first.

[d-v-b]

I think these changes are good, as they make it easier to track the v2-ness of v3-ness of the Array class.

But since this PR changes the Array class, and it will also require anyone who annotated variables with the Array type to update that annotation, I think it's important that we get a lot of eyes on this PR.

cc @zarr-developers/core-devs

[d-v-b]

I think these changes are good, as they make it easier to track the v2-ness of v3-ness of the Array class.

But since this PR changes the Array class, and it will also require anyone who annotated variables with the Array type to update that annotation, I think it's important that we get a lot of eyes on this PR.

cc @zarr-developers/core-devs

ping @zarr-developers/core-devs

[d-v-b]

this looks good to me, I'm going to merge it as soon as everything is green, thanks for the work here @dstansby

[d-v-b]

@dstansby I pushed some fixes to merge conflicts, let me know those changes are OK

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

git checkout 3.1.x
git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 edd47db7959fb8a355bd7560878fdafc11277fc2

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #3304: Parametrize Array with v2/v3 metadata'

4. Push to a named branch:

git push YOURFORK 3.1.x:auto-backport-of-pr-3304-on-3.1.x

5. Create a PR against branch 3.1.x, I would have named this PR:

"Backport PR #3304 on branch 3.1.x (Parametrize Array with v2/v3 metadata)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.