Skip to content
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

zarr.open do not allow creating zarr group (regressions since zarr 3.0.0b1) #2490

Closed
Czaki opened this issue Nov 15, 2024 · 15 comments · Fixed by #2629
Closed

zarr.open do not allow creating zarr group (regressions since zarr 3.0.0b1) #2490

Czaki opened this issue Nov 15, 2024 · 15 comments · Fixed by #2629
Labels
bug Potential issues with the zarr-python library
Milestone

Comments

@Czaki
Copy link

Czaki commented Nov 15, 2024

Zarr version

3.0.0b2

Numcodecs version

0.14.0

Python Version

3.12

Operating System

all

Installation

using pip in prerelease version

Description

The latest beta release of zarr 3.0.0b2 the api of creating zarr dataset is failing as it only tries to create zarr array and fails because of lack of shape.

https://github.com/napari/napari/actions/runs/11847863761/job/33018370878

Steps to reproduce

def test_add_zarr_1d_array_is_ignored(tmp_path):
    zarr_dir = str(tmp_path / 'data.zarr')
    # For more details: https://github.com/napari/napari/issues/1471

    z = zarr.open(store=zarr_dir, mode='w')
    z.zeros(name='1d', shape=(3,), chunks=(3,), dtype='float32')

    image_path = os.path.join(zarr_dir, '1d')
    assert npe2.read([image_path], stack=False) == [(None,)]

https://github.com/napari/napari/blob/b28b55d291f8a00ce0129433ee509f645ed16804/napari_builtins/_tests/test_io.py#L313

Additional output

No response

@Czaki
Copy link
Author

Czaki commented Dec 16, 2024

Any update?

@dstansby dstansby added this to the 3.0.0 milestone Dec 16, 2024
@dstansby
Copy link
Contributor

Sorry for the slow response - I'm not sure whether this is an intended breaking change or not? @jhamman, since you're writing the migration guide, do you have any idea if this is intended?

Minimal reproducer:

import zarr
z = zarr.open(store="test.zarr", mode="w")

On zarr v2 this works fine. On current main it gives:

Traceback (most recent call last):
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/api/asynchronous.py", line 1195, in open_array
    return await AsyncArray.open(store_path, zarr_format=zarr_format)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/core/array.py", line 756, in open
    metadata_dict = await get_array_metadata(store_path, zarr_format=zarr_format)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/core/array.py", line 156, in get_array_metadata
    raise FileNotFoundError(store_path)
FileNotFoundError: file://test.zarr

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/dstansby/software/zarr/zarr-python/test.py", line 3, in <module>
    z = zarr.open(store="test.zarr", mode="w")
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/_compat.py", line 43, in inner_f
    return f(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/api/synchronous.py", line 175, in open
    obj = sync(
          ^^^^^
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/core/sync.py", line 141, in sync
    raise return_result
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/core/sync.py", line 100, in _runner
    return await coro
           ^^^^^^^^^^
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/api/asynchronous.py", line 334, in open
    return await open_array(store=store_path, zarr_format=zarr_format, mode=mode, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/software/zarr/zarr-python/src/zarr/api/asynchronous.py", line 1200, in open_array
    return await create(
                 ^^^^^^^
TypeError: create() missing 1 required positional argument: 'shape'

@jhamman
Copy link
Member

jhamman commented Jan 1, 2025

No specific update here. Would be happy to see a fix proposed for this. @agoodm, @Czaki or @jni - would you be interested in contributing a fix?

@normanrz
Copy link
Member

normanrz commented Jan 2, 2025

Fwiw, once #2463 lands, we prefer to use the zarr.{open,create}_{array,group} methods instead of zarr.{open,create,zeros}.

@jni
Copy link
Contributor

jni commented Jan 2, 2025

We'll be happy to update napari to use that API once #2463 is in and (pre)released.

@Czaki
Copy link
Author

Czaki commented Jan 2, 2025

I may try to fix it and add a proper test (as it is broke second time).

Fwiw, once #2463 lands, we prefer to use the zarr.{open,create}_{array,group} methods instead of zarr.{open,create,zeros}.

I still prefer to zarr.open work, maybe with deprecation warning and schedule for removal. This will allow for smother migration.

@jni
Copy link
Contributor

jni commented Jan 2, 2025

On our end @Czaki I think we should update to use the non-deprecated version once #2463 is pre-released. Obviously for other libraries it would be less disruptive if this issue was fixed, with or without deprecation.

@jni
Copy link
Contributor

jni commented Jan 3, 2025

... Except that means we would need to gate on the zarr version rather than have a unified interface for zarr v2 and v3. This bit about API changes gets me every time. 😂

@jni
Copy link
Contributor

jni commented Jan 3, 2025

LOL @jhamman

# TODO: the mode check below seems wrong!
if "shape" not in kwargs and mode in {"a", "r", "r+"}:

hopefully I can just add w to that list and everything will be golden 😂

jni added a commit to jni/zarr-python that referenced this issue Jan 3, 2025
@jni
Copy link
Contributor

jni commented Jan 3, 2025

Possible fix at #2629

@Czaki
Copy link
Author

Czaki commented Jan 3, 2025

On our end @Czaki I think we should update to use the non-deprecated version once #2463 is pre-released. Obviously for other libraries it would be less disruptive if this issue was fixed, with or without deprecation.

Hm, did you forget about plugins? Many plugin maintainers may realize that there is a problem after stable release.

@d-v-b
Copy link
Contributor

d-v-b commented Jan 3, 2025

I do think #2629 will fix your acute issue, but I would also recommend changing the relevant tests in napari in napari. I don't think you need to create a group for those tests -- you can create an array, with zarr.create, which should work similarly across zarr-python 2 and 3.

For background, basically all of the top level functions for creating arrays and groups in zarr 2.x are fundamentally flawed in various ways. This is really unfortunate, because it means providing a good experience to new users will require some breaking changes. But in the long term I think it's worth the pain.

@Czaki
Copy link
Author

Czaki commented Jan 3, 2025

For background, basically all of the top level functions for creating arrays and groups in zarr 2.x are fundamentally flawed in various ways. This is really unfortunate, because it means providing a good experience to new users will require some breaking changes. But in the long term I think it's worth the pain.

if you decide to change to zarr.{open,create}_{array,group} you may provide an easy migration path with keep working zarr.open that will emit warning that suggests switching to zarr.{open,create}_{array,group}

@d-v-b
Copy link
Contributor

d-v-b commented Jan 3, 2025

For background, basically all of the top level functions for creating arrays and groups in zarr 2.x are fundamentally flawed in various ways. This is really unfortunate, because it means providing a good experience to new users will require some breaking changes. But in the long term I think it's worth the pain.

if you decide to change to zarr.{open,create}_{array,group} you may provide an easy migration path with keep working zarr.open that will emit warning that suggests switching to zarr.{open,create}_{array,group}

This is the current plan -- the deprecated top-level functions like create and open will get deprecation warnings before being removed in a future release.

normanrz pushed a commit that referenced this issue Jan 3, 2025
* Add test for #2490

* Add 'w' to list of valid modes in open to create a group

---------

Co-authored-by: Davis Bennett <[email protected]>
@jhamman
Copy link
Member

jhamman commented Jan 3, 2025

Thanks folks for getting this fix together!

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 a pull request may close this issue.

6 participants