Updates for Zarr 3 Dtypes #10456
Updates for Zarr 3 Dtypes #10456
Conversation
github-actions
bot
added
topic-backends
topic-documentation
topic-zarr
ianhi
force-pushed
the
zarr-dtype-tests
branch
2 times, most recently
from
8e40e73 to
3877091
Compare
run plz run for real use true dev xarray get versions right for wacky dev version something
|
heads up @d-v-b I think we are workign in parallel? I saw a commit on your equivalent pr |
|
I'm not making any changes over in that PR, it's just for tracking the test failures between upstream zarr python and xarray main |
|
@d-v-b I think everything passes now with the recent zarr fixes and the small fill value interaction in this PR. I have a little bit of test clean up to do. And I can't get the upsstream tests to run which iss annoying, but i think we're good! |
|
which tests in CI are running with the main version of zarr? |
| @@ -23,8 +23,7 @@ jobs: | |||
| name: detect upstream-dev ci trigger | |||
| runs-on: ubuntu-latest | |||
| if: | | |||
| github.repository == 'pydata/xarray' | |||
| && (github.event_name == 'push' || github.event_name == 'pull_request') | |||
| github.event_name == 'push' || github.event_name == 'pull_request' | |||
There was a problem hiding this comment.
An attempt to get upstream tests to run on this PR
| # probably be 3.1 | ||
|
|
||
| # if Version(zarr.__version__) > Version("3.0.10"): | ||
| if hasattr(zarr_array.metadata.data_type, "to_native_dtype"): |
There was a problem hiding this comment.
Had to check by attrs here because of version confusion. Though maybe this is a better check here anyway.
There was a problem hiding this comment.
better check than version comparison*
There was a problem hiding this comment.
Will this check ever be used for arrays using Zarr 2.x? In 2.x didn't have the .metadata attr.
In [4]: a
Out[4]: <zarr.core.Array (2,) int64>
In [5]: a.metadata._data_type
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Cell In[5], line 1
----> 1 a.metadata._data_type
AttributeError: 'Array' object has no attribute 'metadata'
There was a problem hiding this comment.
This is a good callout, I'm a little surprised the tests didn't pick that up.
For merging we can do >=3.1.0 i think, but I ran into problems testing with version checking because the release of 3.0.10 is higher than 3.0.9.dev (and for some reason the instlalation from git was not turning it into 3.0.10.dev)
|
|
||
| # if Version(zarr.__version__) > Version("3.0.10"): | ||
| if hasattr(zarr_array.metadata.data_type, "to_native_dtype"): | ||
| native_dtype = zarr_array.metadata.data_type.to_native_dtype() |
There was a problem hiding this comment.
zarr_array.dtype is still a regular numpy dtype after my changes. so I don't think you need to go through the metadata
| ) | ||
| ) | ||
| else: | ||
| dtype_value = zarr_array.metadata.data_type.value |
There was a problem hiding this comment.
same here -- I don't think you need to go through the metadata to get the numpy dtype
There was a problem hiding this comment.
also I'm confused because I thought I fixed this already
There was a problem hiding this comment.
I've had this since my initial test, never thought to try to remove it
There was a problem hiding this comment.
when was the last time you pulled in the latest changes from main? because my fix has been in there for a while
There was a problem hiding this comment.
ok this PR started by overwriting my fix :) I would recommend reverting that change
| @@ -816,11 +816,29 @@ def open_store_variable(self, name): | |||
| if zarr_array.fill_value is not None: | |||
| attributes["_FillValue"] = zarr_array.fill_value | |||
| elif "_FillValue" in attributes: | |||
| attributes["_FillValue"] = FillValueCoder.decode( | |||
| attributes["_FillValue"], zarr_array.dtype | |||
| ) | |||
There was a problem hiding this comment.
this code is correct and should be restored
Updates for Zarr Dtypes. Currently most functional, but still debugging the case of Zarr format 2.
Important Changes so far: