Skip to content

Conversation

LucaMarconato
Copy link
Member

@LucaMarconato LucaMarconato commented Aug 18, 2025

closes #979
Resources:

@falexwolf
Copy link
Member

Thanks for opening this PR, Luca! 😄

@LucaMarconato
Copy link
Member Author

LucaMarconato commented Aug 18, 2025

Things that changed (listing also the open todos as tick items). ~250 tests were failing. Now we are down to ~120 failing.

  • ome-zarr-py now requires zarr>=v3.0.0; we inherit this depedency
  • zarr-python now has `requires-python = ">=3.11"; we also adopt this constraint
  • caught a bug with read_only and opening mode in ome-zarr-py: Zarr v3 support #969.
    • There is now a PR to fix it. Currently still using a local patch (need to switch to that PR and try it)
  • now passing SpatialDataFormat to parse_url otherwise it uses the default value of LatestFormat, which is NGFF 0.5. We want to still use NGFF 0.4 for the moment
  • ArrayNotFoundError has been removed from zarr.errors. I haven't looked for a replacement yet, we need to do it.
  • MetadataError from zarr.errors has been renamed to MetadataValidationError
  • the method .visit() has been removed from Zarr groups, I rewrote the code where we were using it
  • zarr.group() removed overwrite. I replaced zarr.group() with zarr.open_group() and now use mode in a similar way instead of overwrite.
  • similarly, I replaced zarr.group() with zarr.open_group() because zarr.open_group() has the parameter mode, that I use to open the group with the correct writing intention.
  • the argument metadata_key has been removed from zarr.consolidate_metadata(), I removed it and it should work (the behavior is covered by tests).
  • the previous way to obtain the root path of a Zarr group was using a private attribute that is no longer available, I replaced the syntax with the new correct one (in io_points.py,io_shapes.py and io_zarr.py).
  • the method zarr.open_consolidated() is not longer available, therefore I changed the way we parse data in io_zarr.py.
  • there was a bug in models/_utils.py where we were not calling a tolist() and leading to a tuple where each element had a numpy type. I caught this because it was causing a JSON serialization error, as the new zarr-python changed the types that it can serialize.
  • currently working on this: the version metadata for OME-Zarr (e.g. 0.4) was located in .zattrs["multiscales"][0]["version"], now it is located in .zattrs[0]["version"]. This seems to be a bug of ome-zarr-py, I will report it. not encountered after Wouter's work

@LucaMarconato
Copy link
Member Author

Currently blocked by ome/ome-zarr-py#477

@d-v-b
Copy link

d-v-b commented Aug 19, 2025

[ ]ArrayNotFoundError has been removed from zarr.errors. I haven't looked for a replacement yet, we need to do it.

see zarr-developers/zarr-python#3367

@LucaMarconato
Copy link
Member Author

In the latest commit I started (still wip) removing the usage of parse_url() in favor of _open_zarr_store() from @berombau (from the cloud branch).

Importantly, parse_url() returns None if the URL doesn't exist or if there is an error opening the path. Instead _open_zarr_store() never complains—we need to adjust the logic accordingly.

In particular, when calling _open_zarr_store() we should be able to intercept the following cases (for which parse_url() returns None):

  • we are trying to "open" a path that exists but that is not a Zarr store;
  • we are trying to "open", in r or r+ a path that doesn't exist, or that cannot be created.

We should not solve this with ad-hoc checks, but by using the right API from zarr-python, so that we get a None or an exception if we are trying to perform such operations.

@LucaMarconato
Copy link
Member Author

LucaMarconato commented Sep 11, 2025

  • Thanks! I will double check this.

@melonora
Copy link
Collaborator

@falexwolf @Zethson @BioinfoTongLI Everything done now, just ome/ome-zarr-py#491 is blocking at the moment.

@LucaMarconato
Copy link
Member Author

LucaMarconato commented Sep 12, 2025

@melonora reviewing the latest commit. First one comment:

  • sdata_with_invalid_zattrs_table_region_not_found() seems to be a duplicate of sdata_with_table_region_not_found_zarrv2 and does not seem to be used. Is this the case?

@LucaMarconato
Copy link
Member Author

I fixed it. That function can indeed be removed. I also sorted the fixtures in the 2 last paramatrize to match the order in which their are specified in the file. Anyway, now we are good to go!

@LucaMarconato
Copy link
Member Author

Code review finished! Great work @melonora.

The only piece missing is ome/ome-zarr-py#491, after this we can merge!

@melonora
Copy link
Collaborator

@LucaMarconato don't merge this yet, fixing an issue

@melonora
Copy link
Collaborator

I noticed that we were creating a new multiscales key outside ome. The last commit fixes that and also ensures that the ome metadata is properly overwritten. You can't just overwrite a metadata key value pair in ome. It will not write. You have to extract the ome metadata and then overwrite the whole metadata block if you want it to be persistent.

Furthermore, I have added the functionality of checking with which format the data is written so we can request the correct reader from ome-zarr.

* refactor read_zarr

* remove unneccesary checks
@melonora
Copy link
Collaborator

will reopen the PR as we can't change PR author.

@melonora melonora closed this Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

deprecations and refactors in zarr v3 work.

6 participants