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

Support zarr v3 #249

Closed
wants to merge 7 commits into from
Closed

Support zarr v3 #249

wants to merge 7 commits into from

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jun 12, 2024

removes the zarr 2 specific language from the spec, and adds a section recommending against mixing zarr 2 and zarr 3 hierarchies

Copy link
Contributor

github-actions bot commented Jun 12, 2024

Automated Review URLs

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 12, 2024

this is a lighter version of #227, without any metadata changes.

@will-moore
Copy link
Member

Would a group zarr.json containing multiscales then look like this?

{
  "attributes": {
    "multiscales": [
      {
        "version": "0.5",
        ...
      }
    ]
  },
  "zarr_format": 3,
  "node_type": "group"
}

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 12, 2024

@will-moore yes, everything that was previously in .zattrs would be stored instead under the attributes key in zarr.json. But zarr implementations should make this change invisible, since the basic model of a group has not changed.

@d-v-b d-v-b requested review from joshmoore and sbesson and removed request for joshmoore June 13, 2024 11:30
@joshmoore
Copy link
Member

@d-v-b: would you be up for moving this change from latest to a new copy under 0.5-dev1?

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 13, 2024

@joshmoore I guess that's fine, but then don't I have to manually track changes to latest?

@joshmoore
Copy link
Member

I was thinking these would become the fast moving location, e.g. from the meetings yesterday:

  • dev1: v2 -> v3
  • dev2: RFC-2
  • dev3: ro-crate (RFC-4, whatever)

The problem I'm concerned about when using latest for everything is that there won't be a way to track the intermediate stages in the (more or less) ephemeral data.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 13, 2024

* dev1: v2 -> v3

* dev2: RFC-2

* dev3: ro-crate (RFC-4, whatever)

In this scheme, doesn't dev2 depend on dev1, and similarly for dev3 / dev2? Which means separate folders might get clunky. Or is the idea that these are all independent changes?

@joshmoore
Copy link
Member

They may depend on each but there may also be roll backs in the changes we need. So it definitely might get clunky, but it will let us move quickly for a period of time.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 13, 2024

@joshmoore I made the requested changes, the diff is now unreadable, not sure if there is a way to avoid that as long as we are doing copy + paste

@joshmoore
Copy link
Member

Thanks, @d-v-b.

not sure if there is a way to avoid that as long as we are doing copy + paste

It's a good question, and one I've wondered about independently of the dev challenge. The only fairly wacky idea I had was to have the version be a branch rather than directory. 🤷🏽

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 13, 2024

Alternatively, the repo only ever has 1 version, the latest one, and old versions are accessible via git history (and as github releases), and proposed versions are branches which get merged via PRs. It seems like the only reason to keep old versions like 0.3 around would be if we are planning on changing them, but I don't think that's the case?

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 13, 2024

cf conventions does a normal github release workflow. seems a bit simpler than keeping old versions around as folders

@will-moore
Copy link
Member

I'm using this branch schemas in updated ome-ngff-validator at ome/ome-ngff-validator#35 (with sample data - see link on PR).
All looking good. The only trivial issue I had was due to the new directories containing schemas etc are under 0.5-dev1 whereas the version in the schemas is 0.5-dev.

@normanrz
Copy link
Contributor

I like the idea of maintaining the versions on individual branches.

I think this PR is a useful base for #242 but I don't think it is worth having on its own. If we are fine with using non-finalized versions, why not use the current (or next) iteration of RFC-2? Why introduce yet another way for supporting Zarr v3?

Because it came up in the meeting. I definitely think that this is a breaking change, because all 0.4-conforming OME-Zarr impls would suddenly be non-conforming anymore. While loosening restrictions in libraries or applications can be considered non-breaking changes the same logic does not apply to file format specifications.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 14, 2024

Because it came up in the meeting. I definitely think that this is a breaking change, because all 0.4-conforming OME-Zarr impls would suddenly be non-conforming anymore. While loosening restrictions in libraries or applications can be considered non-breaking changes the same logic does not apply to file format specifications.

Where does the spec state what an implementation must do in order to be considered conformant? I don't think we have ever been strict about this.

@joshmoore
Copy link
Member

It's definitely a breaking change as all of the devs will be. As I mentioned by email, in mind, what you're referring to is dev2.

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/ome2024-ngff-challenge/97363/10

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 27, 2024

@will-moore all the versions should be correct, let me know if I missed anything

@will-moore
Copy link
Member

@d-v-b Great, thanks. Updated ome/ome-ngff-validator#35 accordingly and looks good 👍

@normanrz
Copy link
Contributor

normanrz commented Jul 2, 2024

I mentioned this PR in the RFC-2 revision as an alternative approach.

@yarikoptic
Copy link
Contributor

should this PR be closed or have I misunderstood and it could still be accepted eventually?

@joshmoore
Copy link
Member

Thanks for the ping, @yarikoptic. Closing this now that #261 is merged. I'll either find a way to implement the version publication solutions for 0.5 or file an issue capturing the thoughts above.

Thanks all.

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.

6 participants