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

Add zarr3 streaming v0 #7941

Merged
merged 32 commits into from
Aug 14, 2024
Merged

Add zarr3 streaming v0 #7941

merged 32 commits into from
Aug 14, 2024

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Jul 24, 2024

URL of deployed dev instance (used for testing):

Steps to test:

Issues:


(Please delete unneeded items, merge only when none are left open)

- Added mag/zarr.json & mag/coords route for datasets (not annotations)
@joshmoore
Copy link

This is great! 🙌🏽 When there's a URL that can be tested, please let me know.

@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review August 1, 2024 11:03
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Looking pretty good already :) I added a couple comments. Will do some testing next

@@ -57,6 +58,22 @@ object NgffMultiscalesItem {
implicit val jsonFormat: OFormat[NgffMultiscalesItem] = Json.format[NgffMultiscalesItem]
}

case class NgffMultiscalesItemV2(
Copy link
Member

Choose a reason for hiding this comment

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

I’d say the v2 stuff should be in separate file. If some code is shared between v1 and v2, maybe add a third file for the shared stuff.

Also, is v2 the right version number? I thought they were like at 0.5?

Also, please add some links here to the spec documents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put the shared classes into SharedNgffMetadataAttributes.scala and named the new NgffMetadata version "NgffMetadataV0_5". Should I rename the old version to NgffMetadataV0_4 accordingly?

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

@normanrz do you think we should also already add a datasource-properties.json route and directory listing routes? Will some clients need that?

@normanrz
Copy link
Member

normanrz commented Aug 1, 2024

@normanrz do you think we should also already add a datasource-properties.json route and directory listing routes? Will some clients need that?

Yes, I think that would be useful.

@MichaelBuessemeyer
Copy link
Contributor Author

This is the how the view of a existing color layer looks like when listing an annotation. It says that the layer belongs to a organization and certain dataset. It this alright or should the title be changed? (Bc it kinda leeks the organization & dataset name (although this is already likely leaked via some properties sent to the client)

image

@MichaelBuessemeyer
Copy link
Contributor Author

Yes, I think that would be useful.

It should be done. See the added routes in the testing guide.

@normanrz
Copy link
Member

I think the metadata is not correct yet. For example, https://addzarrstreamingv.webknossos.xyz/data/zarr3_experimental/sample_organization/l4_sample/color/zarr.json should be similar to this:

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

You can validate with https://deploy-preview-36--ome-ngff-validator.netlify.app/?source=https://addzarrstreamingv.webknossos.xyz/data/zarr3_experimental/sample_organization/l4_sample/color

@MichaelBuessemeyer
Copy link
Contributor Author

Thanks for poiting this out @normanrz
the newest version (not yet deployed) should incorporate the schema fix.

But the validator seems broken to me. It shows some warnings about not being able to find schema definitions.
image

A auick look showed that the referenced schema json for the version is no longer available under the link linked in the schema definition of https://raw.githubusercontent.com/normanrz/ngff/spec-rfc2/latest/schemas/image.schema

Thus, I just used a different json validator and with the assumption that "0.5-dev2" is the correct version string, the schema of the https://addzarrstreamingv.webknossos.xyz/data/zarr3_experimental/sample_organization/l4_sample/color/zarr.json should now be valid :)

@MichaelBuessemeyer MichaelBuessemeyer requested review from normanrz and removed request for normanrz August 13, 2024 11:37
@normanrz
Copy link
Member

Now it is green!
Screenshot 2024-08-13 at 16 33 53

@MichaelBuessemeyer
Copy link
Contributor Author

Now it is green!

Awesome 🎉 🚀

Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

This looks very nice! Thanks.

I just pinged you again on this one comment by @fm3.

@normanrz
Copy link
Member

:shipit:

@MichaelBuessemeyer MichaelBuessemeyer enabled auto-merge (squash) August 14, 2024 14:04
@MichaelBuessemeyer MichaelBuessemeyer merged commit cb1331b into master Aug 14, 2024
2 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the add-zarr3-streaming-v0 branch August 14, 2024 14:23
MichaelBuessemeyer added a commit that referenced this pull request Aug 14, 2024
MichaelBuessemeyer added a commit that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add zarr3 streaming
4 participants