Skip to content
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

Deprecate the experimental v3 implementation #1802

Merged
merged 6 commits into from
Apr 22, 2024

Conversation

jhamman
Copy link
Member

@jhamman jhamman commented Apr 19, 2024

This PR adds a FutureWarning to assert_zarr_v3_api_available to alert users that this version of Zarr is being replaced with a spec complaint v3 version.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

Great idea. I could also see warning if the environment variable is even set, but I defer on that.

@@ -22,9 +23,21 @@
DEFAULT_ZARR_VERSION = 2

v3_api_available = os.environ.get("ZARR_V3_EXPERIMENTAL_API", "0").lower() not in ["0", "false"]
_has_warned_about_v3 = False # to avoid printing the warning multiple times
Copy link
Member

Choose a reason for hiding this comment

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

I thought this was the default but 🤷🏽

https://docs.python.org/3/library/warnings.html#the-warnings-filter

Copy link
Member Author

Choose a reason for hiding this comment

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

I certainly tried this first. It's possible this would have worked if I had put warning elsewhere, but as implemented, the warning is emitted multiple times without this switch. I think this is due to the fact that assert_zarr_v3_api_available is used in various places around the library.

global _has_warned_about_v3
if v3_api_available and not _has_warned_about_v3:
warnings.warn(
"The experimental Zarr V3 implementation in this version of Zarr-Python is not "
Copy link
Member

@joshmoore joshmoore Apr 19, 2024

Choose a reason for hiding this comment

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

❤️

(TIL emoji names are case sensitive)

@jhamman jhamman added the V3 Related to compatibility with V3 spec label Apr 22, 2024
@jhamman jhamman added this to the 2.18.0 milestone Apr 22, 2024
@jhamman jhamman enabled auto-merge (squash) April 22, 2024 13:37
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.98%. Comparing base (5d566eb) to head (c8e2f12).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1802   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files          38       38           
  Lines       14640    14655   +15     
=======================================
+ Hits        14638    14653   +15     
  Misses          2        2           
Files Coverage Δ
zarr/_storage/store.py 100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py 100.00% <100.00%> (ø)

@jhamman jhamman merged commit dd3dd96 into zarr-developers:main Apr 22, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V3 Related to compatibility with V3 spec
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants