Fix zarr 3.0 compatibility by Czaki · Pull Request #7215 · napari/napari

Czaki · 2024-08-26T13:42:41Z

References and relevant issues

closes #7279

Description

Fix zarr 3.0 related test by used named arguments

codecov · 2024-08-26T14:51:59Z

Codecov Report

Attention: Patch coverage is 85.50725% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.75%. Comparing base (22a5018) to head (2fd03f1).
Report is 45 commits behind head on main.

Files with missing lines	Patch %	Lines
napari_builtins/io/_read.py	0.00%	9 Missing ⚠️
napari/layers/labels/_tests/test_labels.py	94.11%	1 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

psobolewskiPhD

One of the big changes of Zarr v3 is that everything now goes in zarr.json instead of .zarray + .zgroup + .zattrs so need to update the logic in builtins to use either or.

Czaki · 2024-08-26T20:54:44Z

It looks also like zarr.open is a problem as it does not allow creating group file.

psobolewskiPhD · 2024-08-26T21:53:41Z

Looks like need to block pydantic==2.9.0b1 for this PR.

Czaki · 2024-08-26T22:39:02Z

There is even issue for this pydantic/pydantic#10239 initially reported here https://x.com/Czaki_PL/status/1828178992332955873

# References and relevant issues closes #7193 # Description When try to fix zarr problems with zarr 3.0.0 in #7215 I have found that it is not easy with this release, so I prefer to block 3.0.0a1 release and open issue in zarr

jni · 2024-09-18T09:54:40Z

+        import zarr
+
+        image = zarr.open(store=path)
+        shape = image.shape


@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"

I'm not familiar with zarr arrays. I just follow the tests. And now waiting on new Zarr release.

jni · 2024-10-17T01:45:28Z

@@ -1,4 +1,3 @@
-matplotlib !=3.9.1 ; platform_system == 'Windows'


Is this change intentional?

yes. This release is yanked from PyPI

Ok. In general this should probably be a separate PR but let's ignore this time. 🙏

I need to edit this file to trigger --pre tests

It seems like we should have a better mechanism for that 😂 but ok!

There is, but I prefer to validate it in another PR.

jni · 2024-10-17T01:47:15Z

        ]
        assert image, 'No arrays found in zarr group'
        shape = image[0].shape
+    elif (path / 'zarr.json').exists():


We probably want to add a test for zarr v3? Or is this codecov being silly because only pre is testing v3 arrays?

yes. zarr 3 is only available in --pre tests

Co-authored-by: Juan Nunez-Iglesias <[email protected]>

stale

psobolewskiPhD

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?

psobolewskiPhD · 2024-10-22T13:28:00Z

-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',


We skip 1D here:
https://github.com/Czaki/napari/blob/a451f643065ce5ce0242bf491cd76a2dbc90d1c9/napari_builtins/io/_read.py#L232-L234

So then None is returned

psobolewskiPhD · 2024-10-22T13:30:42Z

  "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."


I made that as a suggestion.

Co-authored-by: Peter Sobolewski <[email protected]>

This reverts commit 215724f.

This reverts commit 6e2881f.

Czaki · 2024-11-04T13:30:15Z

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?

Unfortunately, zarr 2.x does not have create_array method, so we need to keep create_dataset for zarr 2.x compatybility. See https://github.com/napari/napari/actions/runs/11660697193/job/32463676718?pr=7215#step:13:576

psobolewskiPhD · 2024-11-04T14:08:38Z

Gah, sorry! I've was using a zarr v3 env and reading those docs.
Annoying!

jni · 2024-11-05T13:56:04Z

+            image = da.from_zarr(data)
+            shape = image.shape
+        else:
+            image = [data[k] for k in sorted(data)]


@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:

Suggested change

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()

]

Here I use reading subarrays by zarr itself.
At least zarr v3 read properly file with multiple groups.

Czaki · 2024-11-05T21:42:10Z

+@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,
+)


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.

fix zarr 3.0 compatybility

7914efe

Czaki added the maintenance PR with maintance changes, label Aug 26, 2024

Czaki added this to the 0.5.3 milestone Aug 26, 2024

Czaki requested review from a team, DragaDoncila, brisvag, jni, lucyleeow and psobolewskiPhD as code owners August 26, 2024 13:42

github-actions Bot added tests Something related to our tests topic:qt Relates to qt labels Aug 26, 2024

mark test as expected to fail for zarr 3

a0ed389

psobolewskiPhD previously requested changes Aug 26, 2024

View reviewed changes

part of fixes

a130130

Czaki mentioned this pull request Aug 26, 2024

Block zarr 3.0.0a1 and 3.0.0a2 in pre-release tests #7217

Merged

jni modified the milestones: 0.5.3, 0.5.4 Aug 29, 2024

Merge branch 'main' into fix_zarr_3.0

81229a6

jni changed the title ~~Fix zarr 3.0 compatybility~~ Fix zarr 3.0 compatibility Aug 31, 2024

Merge branch 'main' into fix_zarr_3.0

d3dd415

Czaki marked this pull request as draft August 31, 2024 14:02

Czaki added 3 commits September 7, 2024 08:42

Merge branch 'main' into fix_zarr_3.0

e07ee3e

remove yanked matplotlib releease

c2042e4

Merge branch 'main' into fix_zarr_3.0

12c316f

jni reviewed Sep 18, 2024

View reviewed changes

jni reviewed Oct 17, 2024

View reviewed changes

Apply suggestions from code review

267e083

Co-authored-by: Juan Nunez-Iglesias <[email protected]>

jni mentioned this pull request Oct 17, 2024

feature(group): add group setitem api zarr-developers/zarr-python#2393

Merged

6 tasks

Czaki added 2 commits October 21, 2024 14:28

Merge branch 'main' into fix_zarr_3.0

2babd1b

remove zarr from denylist

a451f64

psobolewskiPhD approved these changes Oct 22, 2024

View reviewed changes

Czaki and others added 6 commits October 22, 2024 15:49

block ipykernel

9fb4dca

Merge branch 'main' into fix_zarr_3.0

099f695

Apply suggestions from code review

6e2881f

Co-authored-by: Peter Sobolewski <[email protected]>

remove filter warning

215724f

Revert "remove filter warning"

7dc22ee

This reverts commit 215724f.

Revert "Apply suggestions from code review"

5d897ff

This reverts commit 6e2881f.

fix 1s tests

50eb333

jni reviewed Nov 5, 2024

View reviewed changes

Czaki added the ready to merge Last chance for comments! Will be merged in ~24h label Nov 5, 2024

Czaki added 5 commits November 5, 2024 15:28

trigger ci

61241cd

try use context manager

5c04d19

revert context managers

a69b55b

skip test on windows

9814c70

use xfail

2fd03f1

Czaki commented Nov 5, 2024

View reviewed changes

Czaki merged commit e0600ff into napari:main Nov 6, 2024

Czaki deleted the fix_zarr_3.0 branch November 6, 2024 15:09

github-actions Bot removed the ready to merge Last chance for comments! Will be merged in ~24h label Nov 6, 2024

jni added the highlight PR that should be mentioned in next release notes label Dec 11, 2024

psobolewskiPhD mentioned this pull request Jan 20, 2025

adding support for the zarr v3 spec #4382

Closed

		@@ -1,4 +1,3 @@
		matplotlib !=3.9.1 ; platform_system == 'Windows'

-            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()
+            ]

Uh oh!

Uh oh!

Conversation

Czaki commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References and relevant issues

Description

Uh oh!

codecov Bot commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

psobolewskiPhD left a comment

Choose a reason for hiding this comment

Uh oh!

Czaki commented Aug 26, 2024

Uh oh!

psobolewskiPhD commented Aug 26, 2024

Uh oh!

Czaki commented Aug 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psobolewskiPhD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Czaki commented Nov 4, 2024

Uh oh!

psobolewskiPhD commented Nov 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Czaki commented Aug 26, 2024 •

edited

Loading

codecov Bot commented Aug 26, 2024 •

edited

Loading