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 frontend for zarr import #6383

Merged
merged 34 commits into from
Aug 22, 2022
Merged

Conversation

Dagobert42
Copy link
Contributor

@Dagobert42 Dagobert42 commented Aug 8, 2022

URL of deployed dev instance (used for testing):

Steps to test:

Open:

  • add optionally displayable log
  • check that both datasources have same voxel size else warning when merging
  • check if dataset already exists
  • make layers unique without overriding old layers of same name

Issues:


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

@fm3 fm3 changed the base branch from master to import-zarr August 8, 2022 12:44
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

cool stuff :) I left some feedback, but nothing major.

frontend/javascripts/admin/admin_rest_api.ts Outdated Show resolved Hide resolved
frontend/javascripts/admin/admin_rest_api.ts Outdated Show resolved Hide resolved
frontend/javascripts/admin/admin_rest_api.ts Outdated Show resolved Hide resolved
@fm3
Copy link
Member

fm3 commented Aug 11, 2022

As discussed I pushed two changes to the base branch zarr-import

  • send the report as single string
  • catch exceptions for invalid uris

Feel free to merge that base branch into yours and adapt the front-end to the single-string report :)

@Dagobert42 Dagobert42 changed the title [WIP] Add frontend for zarr import Add frontend for zarr import Aug 12, 2022
@normanrz
Copy link
Member

Some notes, can be a follow-up:

  • Nobody should see the datasource json. We should show a slimmed-down nice representation of it:
    • Dataset name (editable)
    • Dataset scale (editable)
    • List of Layers
      • Name (editable)
      • Category (editable)
      • Bounding box (read-only)
      • List of mags (read-only)
      • Optional largest segment id (editable)
  • I would introduce radio buttons: No authentication, Basic auth. And only show user/pass when selecting basic auth. Same for S3 (Anonymous, With Credentials). Otherwise, it is too much information that needs to be put in.
  • Not a fan of the buttons stretching all the way from left to right. I feel they are easy to miss. Make them regular-sized and right-aligned.

@Dagobert42
Copy link
Contributor Author

Dagobert42 commented Aug 17, 2022

I opened follow-up #6409 for making the datasource representation and editing prettier. The other suggestions I've adapted like so:

Screenshot 2022-08-17 at 11 02 12

@fm3
Copy link
Member

fm3 commented Aug 17, 2022

Note that the backend currently does not support anonymous s3 access (compare #6401 ) – so maybe that should be disabled in the front-end too until that follow-up is solved

@Dagobert42
Copy link
Contributor Author

Ok the frontend will reflect this as such for now when s3:// is being used:

Screenshot 2022-08-17 at 11 22 23

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

nice 👍 code looks good and it works for me well.

@Dagobert42 Dagobert42 merged commit 957e1b3 into import-zarr Aug 22, 2022
@Dagobert42 Dagobert42 deleted the add-frontend-for-zarr-import branch August 22, 2022 12:07
@fm3 fm3 mentioned this pull request Aug 23, 2022
33 tasks
fm3 added a commit that referenced this pull request Aug 23, 2022
* [WIP] import remote zarr datasets

* read zarr header

* make (rudimentary) datasource

* WIP omeNgff import

* detect axis order, voxel size and mag from omengff

* add assertions, return Foxes

* allow multiple layers in one ngff header, return all mags

* pass name + voxel size, respect space units

* pass credentials to mags, make layer names unique, add report

* fix bbox detection, debug chunk reading errors

* relax axis order assertion

* allow inputting uris directly pointing to .zattrs,.zarray,.zgroup. experiment with array order

* ZYX

* error messages for everything that can go wrong

* f2

* lint

* Store AxisOrder with Mag, fix NonLocalReturn, fix fortran-order flip

* fix no-partial-copying-needed case, fix unit axes, read element class independent of endianness

* clean up

* match error

* add put route for datasource

* add provider for http (not https)

* increase https file system provider timeouts

* pretty

* add explaining comment for nested list

* makedirs for new datasource

* send correct error

* clean up chunk loading, catch exceptions

* send report as single string, catch exception for invalid uris

* Detect Segmentation Layers Heuristic, Assert valid element class

* include channel in AxisOrder for forward-compatibility

* allow dots in dataset names (if they are not at first or last position)

* Add frontend for zarr import (#6383)

* basic zarr import

* merge data layers of multiple sources

* use credentials in exploreRemote improve frontend

* [suggestion] minor wording improvement while totally not stealing awesome Toast UX idea

* add rudimentary error for wrong credentials

* add referrer modal onAdded

* remove multiline URL exploration until UX concept is discussed

* return and display exploration report

* updated (unreleased) changelog

* check voxel sizes and potentially warn when merging

* give unique layer names to duplicates

* ensure largest segment id exists

* Edit s3 credentials label

Co-authored-by: Norman Rzepka <[email protected]>

* add radio buttons for authentication

* make s3 always require credentials

Co-authored-by: Florian M <[email protected]>
Co-authored-by: Norman Rzepka <[email protected]>

* update changelog pr number

Co-authored-by: Jonathan Striebel <[email protected]>

Co-authored-by: Arthur Hilbert <[email protected]>
Co-authored-by: Norman Rzepka <[email protected]>
Co-authored-by: Jonathan Striebel <[email protected]>
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.

Allow import of remote Zarr datasets
4 participants