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 Neuroglancer Dataset with Authentication #4037

Merged
merged 5 commits into from
Apr 30, 2019

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Apr 23, 2019

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Add a wk-connect instance locally
  • Open the "Add Dataset" view and switch to the "Add Neuroglancer Dataset" tab
    • Add a dataset which is saved on Google Cloud Storage (I'll let you know how to get credentials)
    • The dataset should be added successfully and you should be able to see data when viewing it
  • Add a Neuroglancer dataset without authentication, for example: https://neuroglancer-demo.appspot.com/#!%7B%22layers%22:%5B%7B%22source%22:%22precomputed://gs://neuroglancer-public-data/kasthuri2011/image%22%2C%22type%22:%22image%22%2C%22name%22:%22original-image%22%2C%22visible%22:false%7D%2C%7B%22source%22:%22precomputed://gs://neuroglancer-public-data/kasthuri2011/image_color_corrected%22%2C%22type%22:%22image%22%2C%22name%22:%22corrected-image%22%7D%2C%7B%22source%22:%22precomputed://gs://neuroglancer-public-data/kasthuri2011/ground_truth%22%2C%22type%22:%22segmentation%22%2C%22selectedAlpha%22:0.63%2C%22notSelectedAlpha%22:0.14%2C%22segments%22:%5B%2213%22%2C%2215%22%2C%222282%22%2C%223189%22%2C%223207%22%2C%223208%22%2C%223224%22%2C%223228%22%2C%223710%22%2C%223758%22%2C%224027%22%2C%22444%22%2C%224651%22%2C%224901%22%2C%224965%22%5D%2C%22name%22:%22ground_truth%22%7D%5D%2C%22navigation%22:%7B%22pose%22:%7B%22position%22:%7B%22voxelSize%22:%5B6%2C6%2C30%5D%2C%22voxelCoordinates%22:%5B5519.10400390625%2C8531.5380859375%2C1196.4942626953125%5D%7D%7D%2C%22zoomFactor%22:22.573112129999547%7D%2C%22perspectiveOrientation%22:%5B0.3849591314792633%2C0.6616792678833008%2C-0.6423912644386292%2C-0.0363389253616333%5D%2C%22perspectiveZoom%22:340.35867907175077%2C%22layout%22:%224panel%22%7D

Issues:


  • Ready for review

@daniel-wer daniel-wer requested a review from philippotto April 23, 2019 17:08
@daniel-wer daniel-wer self-assigned this Apr 23, 2019
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.

Awesome 👍 Code looks good and it works well, too. I only have two remarks:

  • the upload area for the auth file is quite prominent which makes it less obvious that it is optional. maybe add "optional" to the form item label and/or into the drag area description text.
  • if I try to add a dataset (which requires authentication) without authentication, I get a hard error message (see below). Can we change this to say something like "You need to add an auth file"? Would this need to happen in wk-connect?
HTTP Forbidden (403) in webknossos-connect.
Show debug information
Traceback (most recent call last):
  File "/root/.local/share/virtualenvs/app-4PlAip0Q/lib/python3.7/site-packages/sanic/app.py", line 750, in handle_request
    response = await response
  File "/app/wkconnect/webknossos/access.py", line 59, in decorated_function
    return await f(request, *args, **kwargs)
  File "/app/wkconnect/routes/datasets/upload.py", line 28, in add_dataset
    for dataset_args in iterate_datasets(request.json)
  File "/app/wkconnect/__main__.py", line 50, in add_dataset
    organization_name, dataset_name, deepcopy(dataset_config)
  File "/app/wkconnect/backends/neuroglancer/backend.py", line 98, in handle_new_dataset
    for layer_name, layer in dataset_info["layers"].items()
  File "/app/wkconnect/backends/neuroglancer/backend.py", line 74, in __handle_layer
    info_url, headers=await get_header(token)
  File "/root/.local/share/virtualenvs/app-4PlAip0Q/lib/python3.7/site-packages/aiohttp/client.py", line 581, in _request
    resp.raise_for_status()
  File "/root/.local/share/virtualenvs/app-4PlAip0Q/lib/python3.7/site-packages/aiohttp/client_reqrep.py", line 942, in raise_for_status
    headers=self.headers)
aiohttp.client_exceptions.ClientResponseError: 403, message='Forbidden'

@daniel-wer
Copy link
Member Author

Thanks for your feedback, I adjusted the caption and text to explain this is optional and when it would be needed:

non-public

Regarding the error messages, yes that should be done as part of wk-connect in my opinion (otherwise wK would need to parse the error message), do you agree @jstriebel?

@jstriebel
Copy link
Contributor

@philippotto What do you mean with a "hard" error? The message should not be in the response, I agree. I guess the response is a 500?

@jstriebel
Copy link
Contributor

I reproduced it, I will fix this in wk-connect.

@philippotto
Copy link
Member

@philippotto What do you mean with a "hard" error? The message should not be in the response, I agree. I guess the response is a 500?

Mainly, I meant the stack trace. The wk backend usually sends a JSON which includes details about the error message (e.g., something like [{error: "Authentication failed"}]). However, I don't have a strong opinion about whether wk connect should do that, too.

@@ -201,7 +201,7 @@ services:

# webKnossos-connect
webknossos-connect:
image: scalableminds/webknossos-connect:master__190
image: scalableminds/webknossos-connect:master__205
Copy link
Contributor

Choose a reason for hiding this comment

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

I just squeezed this in here, this is the upgrade to include the nicer error message.

Copy link
Member

Choose a reason for hiding this comment

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

Nice, thank you!

@daniel-wer daniel-wer merged commit 9736ad5 into add-boss-db-dataset Apr 30, 2019
@daniel-wer daniel-wer deleted the neuroglancer-auth branch April 30, 2019 11:38
daniel-wer added a commit that referenced this pull request Apr 30, 2019
* add tab to add bossdb dataset

* upgrade antd to newest version

* use Input.Password antd component which allows to show password

* fix tests by not polluting global namespace

* fix window mocking in non-test env

* update changelog

* fix pretty

* Add Neuroglancer Dataset with Authentication (#4037)

* add authentication upload to neuroglancer add dataset view

* fix linting

* indicate that credentials are optional and when they are needed

* upgrade wk-connect in docker-compose

* pretty
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.

3 participants