Skip to content

Remove/relax erroneous "meta" path check #1123

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

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Sep 6, 2022

[Description of PR]

resolves #1105

This PR removes the check disallowing keys starting with "meta" from the Zarr v2 code path. It was not there previously and appears to have been accidentally introduced along with the preliminary v3 support.

For v3, the check is updated to disallow starting with "meta/" instead of "meta" for consistency with StoreV3._validate_key (used by the __setitem__ method of various v3 store classes).

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)

@grlee77 grlee77 added the bug Potential issues with the zarr-python library label Sep 6, 2022
@grlee77
Copy link
Contributor Author

grlee77 commented Sep 6, 2022

@joshmoore, this seems relatively high priority to get in for v2.13 as it was a regression in behavior for Zarr v2 arrays introduced in 2.12.

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #1123 (1cec9fc) into main (50edad8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1123   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          35       35           
  Lines       13965    13967    +2     
=======================================
+ Hits        13958    13960    +2     
  Misses          7        7           
Impacted Files Coverage Δ
zarr/hierarchy.py 99.78% <100.00%> (-0.01%) ⬇️
zarr/tests/test_storage.py 100.00% <100.00%> (ø)

@joshmoore
Copy link
Member

Thanks, @grlee77! @zndr27 and @tasansal, any chance of your giving this a try?

@tasansal
Copy link
Contributor

tasansal commented Sep 6, 2022

I will try this version on our files later today or tomorrow and let you know.

@tasansal
Copy link
Contributor

tasansal commented Sep 7, 2022

@grlee77 it is working on our datasets that have a "metadata" group now 👍🏻

@joshmoore
Copy link
Member

Thanks, @tasansal. I've added a test (that fails without this PR) and will get this into a pre-release this morning. I'll follow up with 2.13.0 ASAP.

@joshmoore joshmoore merged commit ce129a5 into zarr-developers:main Sep 8, 2022
@joshmoore joshmoore mentioned this pull request Sep 8, 2022
6 tasks
@joshmoore
Copy link
Member

Oddly, the test I added here is failing on conda-forge: conda-forge/zarr-feedstock#65

cc: @jakirkham

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Potential issues with the zarr-python library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with xarray and zarr when dimensions start with "meta"
3 participants