-
Notifications
You must be signed in to change notification settings - Fork 4
Update dependencies #29
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?
Conversation
|
The CI isn't testing with the right version of Python, so #30 should be merged before this. I don't have time to investigate Zarr today so marking this PR as a draft for now. |
8570e02 to
4022a04
Compare
|
This updates the dependencies, and all tests except one passes. I don't know anything about differences between Zarr v2 and v3 and how consolidated metadata should be "emulated" in v3, so disable the failing test on Python 3.11+ for now. I'm both swamped right now and I'm pretty sure we don't use odc-loader, so someone else needs to take it from here if consolidated metadata with Zarr v3 should be handled in odc-loader. |
src/odc/loader/test_memreader.py
Outdated
| @pytest.mark.skipif( | ||
| sys.version_info >= (3, 11), | ||
| reason="Zarr 3 does not currently support consolidated metadata", | ||
| ) |
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.
should probably test actual zarr version not Python version where such zarr might be installed.
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.
Ok, fixed, and left a comment so it's easy to find the information in this PR for the person who will update the test to also work for Zarr v3.
|
Sorry for the drive by review, but Zarr 3 does support consolidated metadata it is just in a different location. Instead of being in its own file it is under the "consolidated_metadata" key in the root zarr.json file. |
|
@jsignell thank you! Is https://github.com/zarr-developers/zarr-python/blob/3d0e40e171ed91d8315be7e4850c301a2093ca47/src/zarr/api/asynchronous.py#L234 wrong, or is my reading comprehension lacking? |
|
Your reading comprehension is good 😅. It's true that consolidated metadata is not part of the Zarr v3 spec, but it has never been part of the spec. It's just a convention. People are still using it though. The usefulness of consolidated metadata depends on the type of store you have and how much it changes, basically how much of a pain it is to keep consolidated metadata uptodate, so for instance icechunk - which wants to handle everything about how stores get updated - does not support consolidated metadata. Here are some relevant issues: pydata/xarray#10122, zarr-developers/zarr-python#3119 |
20b81e1 to
67b4978
Compare
This makes it possible to run "uv run black/isort" with a fixed version of black/isort.
This enables Zarr v3 with Python versions that are compatible.
This enables Zarr v3 with Python
versions that are compatible.
Disable the failing memreader Zarr test on Zarr v3 for now.
Also add a commit that adds black/isort to
the dev dependencies so it's possible to
format/isort the repository easily.