Fix zarr 3.0 compatibility #7215
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7215 +/- ##
==========================================
- Coverage 92.88% 92.75% -0.14%
==========================================
Files 623 623
Lines 57547 57563 +16
==========================================
- Hits 53453 53390 -63
- Misses 4094 4173 +79 ☔ View full report in Codecov by Sentry. |
|
It looks also like |
|
Looks like need to block |
|
There is even issue for this pydantic/pydantic#10239 initially reported here https://x.com/Czaki_PL/status/1828178992332955873 |
napari_builtins/io/_read.py
Outdated
| import zarr | ||
|
|
||
| image = zarr.open(store=path) | ||
| shape = image.shape |
There was a problem hiding this comment.
@Czaki this use case probably also needs to be split into single array/group of arrays? I presume that we can make the earlier ifs more complex: "if there's a .zarray or there's a zarr.json and it only contains one array"
There was a problem hiding this comment.
I'm not familiar with zarr arrays. I just follow the tests. And now waiting on new Zarr release.
| @@ -1,4 +1,3 @@ | |||
| matplotlib !=3.9.1 ; platform_system == 'Windows' | |||
There was a problem hiding this comment.
yes. This release is yanked from PyPI
There was a problem hiding this comment.
Ok. In general this should probably be a separate PR but let's ignore this time. 🙏
There was a problem hiding this comment.
I need to edit this file to trigger --pre tests
There was a problem hiding this comment.
It seems like we should have a better mechanism for that 😂 but ok!
There was a problem hiding this comment.
There is, but I prefer to validate it in another PR.
| @@ -133,6 +133,18 @@ def read_zarr_dataset(path: str): | |||
| ] | |||
| assert image, 'No arrays found in zarr group' | |||
| shape = image[0].shape | |||
| elif (path / 'zarr.json').exists(): | |||
There was a problem hiding this comment.
We probably want to add a test for zarr v3? Or is this codecov being silly because only pre is testing v3 arrays?
There was a problem hiding this comment.
yes. zarr 3 is only available in --pre tests
Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
There was a problem hiding this comment.
I tested locally and everything works as far as I can tell with a few simple zarr array tests -- there is no migration guide yet for v3.
I think the 1D thing doesn't really affect us, as we skip opening 1D arrays anyways.
To avoid the deprecation, should be no issue to just use create_array instead of create_dataset, because I don't think we care about h5py in our tests?
napari_builtins/_tests/test_io.py
Outdated
| def test_add_zarr_1d_array_is_ignored(): | ||
| @pytest.mark.skipif( | ||
| parse_version(version('zarr')) > parse_version('3a0'), | ||
| reason='zarr 3+ does not support setting 1d data', |
There was a problem hiding this comment.
We skip 1D here:
https://github.com/Czaki/napari/blob/a451f643065ce5ce0242bf491cd76a2dbc90d1c9/napari_builtins/io/_read.py#L232-L234
So F438 then None is returned
| @@ -362,7 +362,8 @@ filterwarnings = [ | |||
| # https://github.com/pydata/xarray/pull/8939 | |||
| "ignore:__array__ implementation doesn't accept a copy keyword, so passing copy=False failed.", | |||
| "ignore:pkg_resources is deprecated", | |||
| "ignore:Deprecated call to `pkg_resources.declare_namespace" | |||
| "ignore:Deprecated call to `pkg_resources.declare_namespace", | |||
| "ignore:Use Group.create_array instead." | |||
There was a problem hiding this comment.
I made that as a suggestion.
Unfortunately, zarr 2.x does not have |
|
Gah, sorry! I've was using a zarr v3 env and reading those docs. |
| image = da.from_zarr(data) | ||
| shape = image.shape | ||
| else: | ||
| image = [data[k] for k in sorted(data)] |
There was a problem hiding this comment.
@Czaki why do you think this works? Why is it different from the v2 version?
image = [
read_zarr_dataset(subpath)[0]
for subpath in sorted(path.iterdir())
if not subpath.name.startswith('.') and subpath.is_dir()
]Incidentally, while exploring this, I found that our current 1D skipping mechanism is completely broken, even in v2. If the first dataset is 1D, we skip the whole subdirectory. If a later dataset is 1D, we don't skip it at all.
Probably, we should merge this and fix that issue separately. Here's the diff to the test that fails, both here and in main:
@@ -356,3 +356,8 @@ def test_add_many_zarr_1d_array_is_ignored(tmp_path):
else:
assert isinstance(out[0], da.Array)
assert out[0].ndim == int(name[0])
+ out1, out2, out3 = npe2.read([zarr_dir.as_posix()], stack=False)
+ assert out1 == (None,)
+ assert out2[0].ndim == 2
+ assert out3[0].ndim == 3
But anyway, to simplify flow between versions, I suggest making this the same as v2:
| image = [data[k] for k in sorted(data)] | |
| image = [ | |
| read_zarr_dataset(subpath)[0] | |
| for subpath in sorted(path.iterdir()) | |
| if not subpath.name.startswith('.') and subpath.is_dir() | |
| ] |
There was a problem hiding this comment.
Here I use reading subarrays by zarr itself.
At least zarr v3 read properly file with multiple groups.
| @pytest.mark.xfail( | ||
| parse_version(version('zarr')) >= parse_version('3.0.0a0') | ||
| and os.name == 'nt', | ||
| reason='zarr 3 return incorrect key in windows', | ||
| strict=True, | ||
| ) |
There was a problem hiding this comment.
I need to skip this test on windows because of
zarr-developers/zarr-python#2438
described here https://napari.zulipchat.com/#narrow/channel/212875-general/topic/handling.20zarr.203.2E0.20release/near/480752197
As it is strict xfail it will fail once zarr fix the bug.
References and relevant issues
closes #7279
Description
Fix zarr 3.0 related test by used named arguments