-
Notifications
You must be signed in to change notification settings - Fork 24
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
Allow uploading zarr datasets #7397
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff, code looks pretty good to me, and I tested a bunch, and most cases worked well. I added a couple of comments, especially about datasets with an existing datasource-properties.json file
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/UploadService.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/UploadService.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good :) Please see my question about guessed WKW vs Zarr
case UploadedDataSourceType.EXPLORED => Fox.successful(()) | ||
case UploadedDataSourceType.WKW => | ||
for { | ||
p <- tryExploringMultipleZarrLayers(unpackToDir, dataSourceId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I reading this correctly that we guessed WKW but we still try to explore Zarr? This feels counterintuitive. Maybe the guessing code could be improved instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice trick with the maxDepth ;) (could you call it by name, with maxDepth=2
though? That way we may stumble less when reading this later)
Looks like the linter also sees an unused import still
Works for me!
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)