-
-
Notifications
You must be signed in to change notification settings - Fork 466
Fix typo in error message for zarr 3 #2487
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
Conversation
OriolAbril
left a comment
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.
The previous logic blocked only zarr>=3.0.0.dev0, but now that zarr 3.0.0 has been officially released, the check has been updated to block zarr>=3.0.0 instead.
3.0.0.dev0 comes before 3.0.0. This means the change is the opposite of what you describe. All 3.0 pre-releases (alpha, beta or release candidates) will no longer trigger the error even though the code won't work. With the change we will now only check for 3.0 and higher. There shouldn't be a lot of people using these pre-releases but as you can see from https://pypi.org/project/zarr/#history there are a lot of them. So I am not sure the change helps with anything.
The other change is for the error message, swapping "Instead" for "As a workaround". #2486 is still an open discussion given the current refactoring efforts, so the advise in the error message will probably be the only and recommended way of creating inferencedata objects using zarr 3. Calling it a workaround hints to this being updated or fixed in the future in my opinion.
Last but definitely not least, this definitely does not close the linked issue as it does nothing to support zarr 3. Is your goal to add support for zarr 3 or only to try and improve the error messages?
As you can see on the issue, the main reason for not supporting zarr 3 directly is mainteiner availability so if you can and will work on that it would be welcome and merged if it works. That being said, this means the scope of the PR will increase a lot and zarr 2->3 change is a major version update which imo looks deceptively easy
|
Thanks for the detailed clarification My goal with this PR was only to improve the version check and error messaging not to add full zarr 3 support yet. Based on your feedback, I’ll update the PR accordingly and remove the “closes” reference to avoid implying the issue is resolved. Regarding full zarr 3 compatibility, I’m interested in exploring it gradually on the side to understand what changes would be needed. I’m not committing to implementing full support right now, but if I make progress, I can follow up with a separate PR. Let me know if that approach works, and I’ll start by adjusting this PR to focus only on the messaging. |
|
Sounds good! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2487 +/- ##
=======================================
Coverage 86.83% 86.83%
=======================================
Files 124 124
Lines 13018 13018
=======================================
Hits 11304 11304
Misses 1714 1714 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @OriolAbril, I've applied the requested change to restore the workaround wording. |
This PR updates the zarr version check and error message in
InferenceData.to_zarr().The goal is to improve clarity for users now that zarr 3.0.0 has been officially released. The error message was updated to remove references to development builds and to clearly state the supported version range (>=2.5.0,<3). No functional support for zarr 3 is added in this PR.