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

Clarify that dimension_names is required in zarr.json #293

Merged
merged 8 commits into from
Jan 28, 2025

Conversation

will-moore
Copy link
Member

See discussion at #289.

cc @normanrz @xgui3783

Copy link
Contributor

github-actions bot commented Jan 22, 2025

Automated Review URLs

@xgui3783
Copy link
Contributor

xgui3783 commented Jan 22, 2025

👍🏼 (edit: I should clarify, that I am in favor of this PR, which clarifies that dimension_names is required in zarr.json )

@dstansby
Copy link
Contributor

🙏 please can the 0.5 version of the specification stop being modified, and instead changes made to a draft version? I am implementing version 0.5 of the spec over at ome-zarr-models-py, and it makes life so much harder if the specification keeps silently changing (even more so since there's no changelog...)

@joshmoore
Copy link
Member

Thanks for raising the cautionary flag, @dstansby. I went ahead and updated this PR so that the build would be green. I'll get an announcement out tomorrow and pass a heads up of this change to the community in case this causes any significant implementation issues, but generally I think getting it out as an 0.5.2 clarification worth it soon to avoid any potential confusion.

@dstansby
Copy link
Contributor

Perhaps I am misunderstanding here, but this is not just a clarification? Previously it was optional to have the dimension_names attribute, but after this change it is now mandatory. So it could potentially invalidate data that has been has already been written and is conformant with OME-Zarr 0.5.

What's the reasoning for rushing to get this out and changing version 0.5 of the spec in a breaking manner?

@normanrz
Copy link
Contributor

I think this PR qualifies as a clarification to the original statement in 0.5.0:

The "dimension_names" attribute in the `zarr.json` of the Zarr array of a multiscale level MUST match the names in the "axes" metadata.

@joshmoore
Copy link
Member

In the general case, @dstansby, I agree with your concern, however:

  • It has been "optional" since Wednesday in an untagged, unannounced version that was unintentionally a release due to changes in the build;
  • it's unlikely in that time that someone actively stripped out the writing of dimension_names (please tell me if that's not the case; I don't see a release of ome-zarr-models-python)
  • and in the RFC, the interpretation is clearer, "The dimension_names attribute in the Zarr metadata must match the axes names in the OME-Zarr metadata.", and so I consider the original state "unclarified" and the current state "contradictory" which I would like to correct as soon as possible.

@dstansby
Copy link
Contributor

dstansby commented Jan 27, 2025

My interpretation is it's been optional since the first "release" of 0.5, which said:

The "dimension_names" attribute in the zarr.json of the Zarr array of a multiscale level MUST match the names in the "axes" metadata.

There is no hard requirement (ie, a MUST) in this statement for dimension_names to be present in the zarr.json metadata. Appreciate it's ambiguous, but in that case, isn't it safer to err on the side of caution and be more lenient, tightening it up in the next major release if that was the intent? My worry is that someone could have read version 0.5 of the OME-Zarr spec when it was made public (~months ago now), written some data without including the dimension_names attribute (a reasonable interpretation of the first public release), and if this PR is merged that data suddenly becomes invalid.

If this is going to be merged, at least could the version number required to be written to the OME-Zarr metadata be incremented to 0.5.2? That way in ome-zarr-models-py we know if the version is 0.5 we don't have to check for dimension_names, but if the version is 0.5.2 we do have to check for it.

@joshmoore
Copy link
Member

My interpretation is it's been optional since the first "release" of 0.5 …

I appreciate that that is your interpretation, David, but there are enough differing opinions that, for now, we'll tend toward quick clarity.

If this is going to be merged, at least could the version number required to be written to the OME-Zarr metadata be incremented to 0.5.2?

No. We have not bumped the patch version in the files, and I don’t believe now is the time to start.

My suggestion to you would be to update ome-zarr-models-py to match the clarified behavior. If there are datasets that are found to be invalidated, though unfortunate, the fix is straight-forward and will ultimately produce less confusion in the community.

@dstansby
Copy link
Contributor

If this goes ahead, I think we will have to ignore this change to the specification in ome-zarr-models then to account for the possibility that someone made valid version 0.5 data without dimension_names.

there are enough differing opinions that, for now, we'll tend toward quick clarity.

Given there are differing opinions, why not tend towards not making a breaking change instead, and waiting for version 0.6 to make the change?

@joshmoore
Copy link
Member

I feel like we're about to loop around and repeat our conversation, @dstansby, so rather than repeat my arguments here, I'll just say your objection is noted. As I mentioned on Zulip, I'm going to move forward with the plan I outlined there and above, cognizant that we need to improve the situation for future versions.

I hope you can find a satisfactory solution for ome-zarr-models-py. Personally, again, I'd err on the side of clarity for the community rather than perfection at this point.

@joshmoore joshmoore merged commit 5cdf83b into ome:main Jan 28, 2025
10 checks passed
github-actions bot added a commit that referenced this pull request Jan 28, 2025
…es_required

SHA: 5cdf83b
Reason: push, by @joshmoore

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

5 participants