don't serialize empty tuples for v2 filters, and warn when reading such metadata #2847
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
needs release notes
github-actions
bot
removed
the
needs release notes
|
i don't think the coverage report is accurate |
d-v-b
commented
Feb 18, 2025
| @@ -113,6 +113,13 @@ async def test_roundtrip_array_metadata( | |||
| assert actual == expected | |||
|
|
|||
|
|
|||
| @given(store=stores, meta=array_metadata()) # type: ignore[misc] | |||
| def test_array_metadata_meets_spec(store: Store, meta: ArrayV2Metadata | ArrayV3Metadata) -> None: | |||
There was a problem hiding this comment.
since this test does nothing for v3 metadata, would it be possible to only run it on ArrayV2Metadata?
There was a problem hiding this comment.
here I made it do something ;) . really we should fill it out.
There was a problem hiding this comment.
i was more thinking that it would make sense to test the v2 and v3 specs in separate tests, since they are logically independent, but we can do that refactor later
|
i will look over this again tomorrow. I think we should definitely try to fix the underlying bug this week, e.g. via this PR |
dcherian
added a commit
to moradology/zarr-python
that referenced
this pull request
Feb 20, 2025
* main: don't serialize empty tuples for v2 filters, and warn when reading such metadata (zarr-developers#2847)
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were violating the v2 spec by serializing "no filters" as the empty array. We created arrays in the wild with this metadata, so any fix should be backwards compatible.
To fix this bug, this PR changes the filters parsing function so that it no longer returns empty tuples, and instead return
Nonewhen no filters are specified. For backwards compatibility with datasets created with earlier versions of zarr-python 3.x, thefrom_dictmethod will replace empty lists / tuples with the valueNoneand emit a warning.After a reasonable amount of time (a few minor releases?) we should remove this special-casing and treat
filters = []as an error. Happy to chat about the timeline.closes #2842
TODO:
docs/user-guide/*.rstchanges/