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

ZEP0001 - Core v3.0 spec for review #149

Closed
wants to merge 5 commits into from

Conversation

alimanfoo
Copy link
Member

This pull request includes the Zarr core specification v3.0 as proposed in ZEP0001.

Reviews of this specification are now invited. If possible, please submit comments by using the normal GitHub code review functionality.

Technical note: Since the ZEP process has been adopted, the normal process for proposing new or modified Zarr specifications is to submit a ZEP and to create an associated pull request to the zarr-specs repo with the proposed specification content. However, work on the v3.0 spec began before the ZEP process was adopted, and a draft of the spec had already been merged to the main branch. To make reviewing this spec easier and most similar to the normal process going forward, this PR has been submitted against a "void" branch with no content.

@alimanfoo
Copy link
Member Author

cc @jstriebel, @MSanKeys963, @zarr-developers/steering-council

Copy link
Member Author

@alimanfoo alimanfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions regarding front matter.

docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Outdated Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Alistair! 🙏

Had a question below. Also started collecting some small errata that I can send as a separate PR

docs/core/v3.0.rst Outdated Show resolved Hide resolved
docs/core/v3.0.rst Outdated Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
@shoyer
Copy link

shoyer commented Jul 22, 2022

Is there any intention to include consolidated metadata in the spec? Or would that be an extension? I've found it's pretty important for high performance use-cases.

@alimanfoo
Copy link
Member Author

Is there any intention to include consolidated metadata in the spec? Or would that be an extension? I've found it's pretty important for high performance use-cases.

Hi @shoyer, I was thinking to define consolidated metadata as an extension. And I would put it at the top of the priority list for extensions, because it is so widely used.

@MSanKeys963
Copy link
Member

MSanKeys963 commented Jul 22, 2022

Thanks, @alimanfoo, for opening this PR. Appreciate what you've done and doing for the Zarr project and the community.

I also wanted to invite @zarr-developers/implementation-council to review the ZEP0001 and this PR. Thanks all. 🙏🏻

Copy link

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together, everyone!
I read through most of it now and overall it looks very good; I left a few minor technical comments or requests for clarification. I will be too busy to interact much here in the next 2 weeks, and then be off on vacations for 3 weeks, so feel free to resolve these comments however you see fit without waiting for feedback from my end.

From the z5 perspective: I will probably not have time to add support for v3 in the near future, but I fully support moving ahead with this to enable the progress of the format.
(And my hope would be that another, ideally xtensor based, c++ implementation emerges that I could use to deprecate the z5 c++ code-base to only keep around the z5py python bindings, which I prefer over the zarr python syntax (which is a bit bloated with convenience functions IMHO).)

docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
@jakirkham
Copy link
Member

Toggling for RTD PR preview

@jakirkham jakirkham closed this Jul 26, 2022
@jakirkham jakirkham reopened this Jul 26, 2022
joshmoore and others added 2 commits July 29, 2022 20:27
Co-authored-by: Jonathan Striebel <jstriebel@users.noreply.github.com>
@joshmoore
Copy link
Member

Is there any intention to include consolidated metadata in the spec? Or would that be an extension? I've found it's pretty important for high performance use-cases.

@alimanfoo #149 (comment) Hi @shoyer, I was thinking to define consolidated metadata as an extension. And I would put it at the top of the priority list for extensions, because it is so widely used.

@shoyer: would you be interested in being involved in the drafting of the consolidated ZEP?

@jbms
Copy link
Contributor

jbms commented Aug 5, 2022

I created #153 to add support for a list of codecs, rather than a single compressor, as was discussed in the last meeting.

Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is looking good. To me the main issues that need resolving are:

docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this discussion in #154 (comment), I think it might make sense to broaden storage transformers to act on the entire store, rather than just the array chunk data.

docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
@joshmoore
Copy link
Member

joshmoore commented Nov 3, 2022

At the ZEP meeting today, @jbms, @jstriebel and I discussed where to merge the recent PRs (into this PR or into main). For the sake of simplicity, we decided:

  • PRs should be opened against main
  • PRs which "close" a conversation here should be cross-linked to the relevant discussion
  • Where no PR yet exists, an issue will be opened to capture the state of the discussion
  • When all points above have been "Resolved", this PR can be closed without merging.
  • The merging principle for the (upcoming 🎉) active merging phase will follow the (at least) 4 eye principle*.

Onward and upwards 🚀


* "4 eye principle": There must be at least one reviewer. No self merging.

@jstriebel
Copy link
Member

Thanks everyone for the great feedback and suggestions 🚀! As @joshmoore explained above, we tried to go through all points in this PR and either address them via comments, PRs or issues. The goal is to to capture everything in this PR, but move larger discussions to separate issues, since this comment section was growing too large to stay manageable*. Also the dual-branch system (the origin of this PR and main) was getting complicated, from now on the current state can simply be found on the main branch, rendered here. There are currently three open PRs against the main branch which address feedback from this PR:

Please feel free to leave further feedback there or open different issues and PRs. I marked all discussions as resolved above in order to keep track of the progress, but many of them continue in different issues. Please let me know if you feel that some comments are not addressed appropriately (e.g. the feedback doesn't fit to the issue I linked). I fear that might have easily happened due to the large number of comments*, and apologize for such mistakes. The issues for ZEP 1 / zarr v3 are ordered in this project board. We'll post an update of the current progress and an intended timeline in the next days, I'll also link it here.

Again, 1000 thanks to all the reviewers so far ❤️

* >20m vertically on my screen 🤯

Copy link

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a quick look at this; overall looks good to me!
I left a short comment on implicit groups and race conditions, maybe that could be addressed with one more comment.

docs/core/v3.0.rst Show resolved Hide resolved
docs/core/v3.0.rst Show resolved Hide resolved
@MSanKeys963
Copy link
Member

Thanks a lot, @jstriebel, for working on this. 🙌🏻

@jstriebel
Copy link
Member

Closing this PR, since all feedback is incorporated or tracked in other issues. If you are missing something, please raise this in an issue. See also the current update for Zarr v3 and ZEP 1:
https://zarr.dev/blog/zep1-update

@jstriebel jstriebel closed this Dec 5, 2022
@jbms jbms mentioned this pull request Apr 6, 2023
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.