Skip to content

Add compatibility with Zarr-Python 3.2.0#957

Open
maxrjones wants to merge 4 commits intozarr-developers:mainfrom
maxrjones:zarr-python-3.2-compat
Open

Add compatibility with Zarr-Python 3.2.0#957
maxrjones wants to merge 4 commits intozarr-developers:mainfrom
maxrjones:zarr-python-3.2-compat

Conversation

@maxrjones
Copy link
Copy Markdown
Member

There are some internal changes coming in Zarr-Python that impact VirtualiZarr, which are addressed by this PR.

Acceptance criteria:

  • Closes #xxxx
  • Tests added
  • Tests passing
  • No test coverage regression
  • Full type hint coverage
  • Changes are documented in docs/releases.md
  • New functions/methods are listed in an appropriate *.md file under docs/api
  • New functionality has documentation

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.35%. Comparing base (1b8abe9) to head (962a371).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
+ Coverage   89.30%   89.35%   +0.04%     
==========================================
  Files          33       33              
  Lines        2039     2038       -1     
==========================================
  Hits         1821     1821              
+ Misses        218      217       -1     
Files with missing lines Coverage Δ
virtualizarr/manifests/array.py 85.00% <100.00%> (+0.62%) ⬆️
virtualizarr/parsers/zarr.py 97.50% <100.00%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@maxrjones
Copy link
Copy Markdown
Member Author

failures should be fixed upstream by earth-mover/icechunk#1937

@maxrjones maxrjones requested review from a team April 8, 2026 21:07
@TomNicholas
Copy link
Copy Markdown
Member

failures should be fixed upstream by earth-mover/icechunk#1937

Note that this is only for Icechunk 2.0. We actually have the same problem for the latest release of IC 1.0 . But do we care about that?

@TomNicholas
Copy link
Copy Markdown
Member

Also can we perhaps release a patch version of VZ first? We have multiple things going on here:

  • Broken sharding feature already released that needs patching
  • Compatibility with IC2 (should work out of the box, but should we pin for any reason?)
  • Your changes in this PR

@maxrjones
Copy link
Copy Markdown
Member Author

failures should be fixed upstream by earth-mover/icechunk#1937

Note that this is only for Icechunk 2.0. We actually have the same problem for the latest release of IC 1.0 . But do we care about that?

I think that any release of Icechunk with a fix for the missing readme would solve our CI problems, as long as we don't pin to <2.

@maxrjones
Copy link
Copy Markdown
Member Author

Also can we perhaps release a patch version of VZ first? We have multiple things going on here:

  • Broken sharding feature already released that needs patching
  • Compatibility with IC2 (should work out of the box, but should we pin for any reason?)
  • Your changes in this PR

I'd support merging and releasing #952 before any other changes. fwiw I don't think it's bad to have merged #897 before the patch release - that's a long overdue bug fix.

@TomNicholas
Copy link
Copy Markdown
Member

I think that any release of Icechunk with a fix for the missing readme would solve our CI problems, as long as we don't pin to <2.

That release should come in a sec / by tomorrow.

I'd support merging and releasing #952 before any other changes. fwiw I don't think it's bad to have merged #897 before the patch release - that's a long overdue bug fix.

Agreed.

@TomNicholas TomNicholas added the zarr-python Relevant to zarr-python upstream label Apr 9, 2026
Copy link
Copy Markdown
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Released, so as long as this passes (particularly on min-deps!) then lets go

Comment thread virtualizarr/manifests/array.py Outdated
Comment on lines +8 to +13
try:
from zarr.core.metadata.v3 import RegularChunkGridMetadata # zarr-python>3.1.6
except ImportError:
from zarr.core.metadata.v3 import (
RegularChunkGrid as RegularChunkGridMetadata, # zarr-python<=3.1.6
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should actually look for specific versions of zarr-python instead of the blind try...except?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zarr-Python uses hatch-vcs, which builds versions from git tags. If a user installs from a git source that does not have the tags fetched, the version can display as a low number despite being based on the most recent commits to main, which breaks the code with an obscure error message. This try...except leads to informative error messages by describing exactly what is needed rather than implicitly relying on the version.

@TomNicholas TomNicholas added the test-upstream Run the upstream tests on this PR label Apr 20, 2026
@TomNicholas
Copy link
Copy Markdown
Member

Running the upstream tests shows a breaking change - it seems that zarr now forbids chunks of length 0, which it didn't forbid before?

https://github.com/zarr-developers/VirtualiZarr/actions/runs/24685336933/job/72193091799

AFAICT the Zarr spec does not forbid length-0 chunks, so it's zarr-python that is wrong here?

https://zarr-specs.readthedocs.io/en/latest/v3/chunk-grids/regular-grid/index.html

@maxrjones
Copy link
Copy Markdown
Member Author

maxrjones commented Apr 20, 2026

Running the upstream tests shows a breaking change - it seems that zarr now forbids chunks of length 0, which it didn't forbid before?

zarr-developers/VirtualiZarr/actions/runs/24685336933/job/72193091799

AFAICT the Zarr spec does not forbid length-0 chunks, so it's zarr-python that is wrong here?

zarr-specs.readthedocs.io/en/latest/v3/chunk-grids/regular-grid/index.html

This was discussed in zarr-developers/zarr-python#3802 (comment). We leaned towards this being a mistake in the core spec since modular arithmetic is undefined for 0. Please raise an upstream issue if you still think this should be addressed in zarr-python rather than the spec.

@LDeakin
Copy link
Copy Markdown
Member

LDeakin commented Apr 20, 2026

More context on that

@TomNicholas
Copy link
Copy Markdown
Member

Thanks for the context guys.

@sharkinsspatial you wrote this test and the fixture it uses - do you have any concerns with HDF compatibility if I simply change the behaviour of the HDFParser to raise on length-0 chunks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-upstream Run the upstream tests on this PR zarr-python Relevant to zarr-python upstream

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants