Skip to content

Migrate to zarr-python 3 #49

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

dstansby
Copy link
Collaborator

@dstansby dstansby commented Apr 3, 2025

This switches exisiting code from zarr-python 2 to zarr-python 3, which is part of fixing #33.

@dstansby
Copy link
Collaborator Author

@d-v-b I think this is all good for review now. The only thing broken is the docstests, because memory stores have a different name each time they're printed (because the memory address changes). I'm not too sure the best way to fix this...

@dstansby dstansby marked this pull request as ready for review April 16, 2025 13:38
@d-v-b
Copy link
Collaborator

d-v-b commented Apr 19, 2025

@d-v-b I think this is all good for review now. The only thing broken is the docstests, because memory stores have a different name each time they're printed (because the memory address changes). I'm not too sure the best way to fix this...

I think the best solution would be for zarr-python to stop using the memory address in the repr for memory stores :)

@d-v-b
Copy link
Collaborator

d-v-b commented Apr 19, 2025

is it OK if I make commits against dstansby:zarr-python-3 branch? I think there are some changes to the v3 logic I'd like to add

@dstansby
Copy link
Collaborator Author

Yeah go for it!

@dstansby
Copy link
Collaborator Author

dstansby commented May 6, 2025

This is currently blocked by zarr-developers/zarr-python#3038

@dstansby dstansby force-pushed the zarr-python-3 branch 3 times, most recently from a1b1960 to 41b579a Compare May 17, 2025 08:26
@dstansby
Copy link
Collaborator Author

In good news using zarr main after zarr-developers/zarr-python#3038 was merged works 🎉 . Guess we just need to wait for a new zarr release now, and this should be good to go, but this is now ready for review.

@dstansby dstansby requested a review from d-v-b May 17, 2025 08:34
docs/index.md Outdated
@@ -35,15 +37,9 @@ print(spec.model_dump())
'dtype': '|u1',
'fill_value': 0,
'order': 'C',
'filters': None,
'filters': [],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For whoever reviews this: I updated filters to be an empty list instead of None if there are no filters. This matches zarr-python 3 behaviour. I would be easy enough to revert to previous behaviour though, and set this back to None. Would appreciate thoughts on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the goal is to accurately model what's in zarr-python 2 metadata documents, then filters: [] is invalid

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will definitely fix this to revert to filters=None then 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

fwiw I think the spec is wrong here, and it should allow filters: [], but it is what it is 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants