Allow to add datasets through wk-connect #3843

daniel-wer · 2019-02-27T20:21:09Z

This PR adds an additional tab to the "Add Dataset" view, which allows to add a Neuroglancer dataset to wK using a webknossos-connect datastore. The Neuroglancer dataset is identified using the URL, all the layer information is parsed from that.

I'm not that happy with the code, as there's quite a bit of duplication between the Upload Dataset and Add wk-connect Dataset tabs, but I couldn't find a better abstraction to combine the two, maybe that's ok for now.

Issues that need to fixed:

wk-connect should include a max segment id for segmentation layers, otherwise the "Edit Dataset" view cannot be saved (@jstriebel)
There seems to be an issue with the datasource-properties.json of the default drosophila dataset. At least the frontend validation complains. Needs to be investigated.
(both fixed in datasource-properties: add largestSegmentId, dataFormat & wkwResolutions webknossos-connect#64)
Proper testing of all dataset related functionality (does everything work with a wk-connect datastore)

URL of deployed dev instance (used for testing):

https://___.webknossos.xyz

Steps to test:

Set up an instance of webknossos-connect according to https://github.com/scalableminds/webknossos-connect#usage
Open the "Add Dataset" view and switch to the "Add wk-connect Dataset" tab
- Insert a dataset name, the datastore dropdown should be prefilled and paste the URL to a neuroglancer dataset (for example, the one I appended at the bottom)
  - You should be redirected to the import tab
- Insert a bogus URL - You should not be able to click "Add" and get a suitable error message
The Dataset Upload should still work as before
The Dataset Upload during onboarding should also still work as before

Issues:

fixes Allow to add neuroglancer dataset in wk frontend #3744

Updated changelog
Updated migration guide if applicable
~~- [ ] Updated documentation if applicable~~
~~- [ ] Needs datastore update after deployment~~
Ready for review

Example Neuroglancer dataset URL:
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

…tate handling

…-wk-connect-dataset

philippotto

Nice work and refactoring 👍 Will test now.

philippotto · 2019-03-04T09:10:28Z

frontend/javascripts/messages.js

@@ -125,6 +125,8 @@ In order to restore the current window, a reload is necessary.`,
  "task.recommended_configuration": "The author of this task suggests to use these settings:",
  "dataset.clear_cache_success": "The dataset was reloaded successfully",
  "dataset.upload_success": "The dataset was uploaded successfully",
+  "dataset.add_success": "The dataset was added successfully",
+  "dataset.add_error": "Could not reach the datastore",


No trailing fullstops here? :) Only asking because you added them a few lines below ^^

philippotto · 2019-03-04T09:13:48Z

frontend/javascripts/admin/api_flow_types.js

@@ -363,6 +364,21 @@ export type DatasetConfig = {
  +zipFile: File,
 };

+type wkConnectLayer = {


Maybe WkConnectLayer?

philippotto · 2019-03-04T09:14:15Z

frontend/javascripts/admin/api_flow_types.js

+  type: "image" | "segmentation",
+};
+
+export type wkConnectDatasetConfig = {


Also capital W maybe?

philippotto · 2019-03-04T09:14:47Z

frontend/javascripts/admin/api_flow_types.js

@@ -363,6 +364,21 @@ export type DatasetConfig = {
  +zipFile: File,
 };

+type wkConnectLayer = {
+  source: string,


It's not clear to me what source is. Is it an url? Then I'd suggest sourceUrl.

Unfortunately this is coming from Neuroglancer and wk-connect adopted the same format for simplicity's sake, but yes it is the URL.
If we really want to we could change it, but I think adding a comment will be easier

philippotto · 2019-03-04T09:17:53Z

frontend/javascripts/admin/api_flow_types.js

+export type wkConnectDatasetConfig = {
+  neuroglancer: {
+    [string]: {
+      [string]: {


Can you add a "variable name" for these two string keys? For example, [datasetName: string]. The var name only serves for documentation. For the layer name, it's clear to me, but what is the other hierarchy? Is it the setting name?

philippotto · 2019-03-04T09:58:17Z

frontend/javascripts/admin/dataset/dataset_add_wk_connect_view.js

+        await addWkConnectDataset(formValues.datastore, datasetConfig);
+
+        Toast.success(messages["dataset.add_success"]);
+        trackAction("Add remote dataset");


Why "remote dataset"? I'd use wk-connect here since it might be hard to map in the future when we analyze the data. Also, I'd move this into line 79, since we probably are interested in how often this is tried at all. Failed attempts wouldn't be tracked right now.

Good point, I renamed and moved it :)

philippotto · 2019-03-04T10:05:35Z

frontend/javascripts/admin/dataset/dataset_components.js

+          showSearch
+          placeholder="Select a Datastore"
+          optionFilterProp="children"
+          style={{ width: "100%" }}


Maybe add defaultActiveFirstOption as a prop so that the first item is autoselected.

Did the auto selection of the first item not work for you during testing?
defaultActiveFirstOption does not work if datastores is [] at first and then updated later, it only works on the initial render.
That's why I instead set the initialValue in the field decorator (couple of lines below) which worked on subsequent renders as well for me :)

Ah, sorry, you are probably right. I reviewed the code before testing :)

philippotto · 2019-03-04T10:08:05Z

frontend/javascripts/admin/dataset/dataset_add_wk_connect_view.js

+      <div style={{ padding: 5 }}>
+        <CardContainer withoutCard={withoutCard} title="Add wk-connect Dataset">
+          Currently wk-connect supports adding Neuroglancer datasets. Simply set a dataset name,
+          select the wk-connect datastore and paste the URL to the Neuroglancer dataset.


In which cases would we have more than one wkc datastore here? Is this a usecase at all? It seems a bit "complicated" to me, that the user has to deal with it at all, because most of the time they won't care.
However, it's fine for now, I think. We can always streamline this later :)

I wasn't sure, but you're probably right, there's either one or none wk-connect datastores I'd say. Do you agree @normanrz?

philippotto · 2019-03-04T10:09:26Z

frontend/javascripts/admin/dataset/dataset_add_wk_connect_view.js

+                    },
+                  },
+                ],
+                validateFirst: true,


philippotto · 2019-03-04T10:12:46Z

frontend/javascripts/admin/onboarding.js

@@ -210,11 +211,21 @@ const OrganizationForm = Form.create()(({ form, onComplete }) => {

 class OnboardingView extends React.PureComponent<Props, State> {
  state = {
-    currentStep: 0,
+    currentStep: 2,


This should probably be undone, right?

philippotto · 2019-03-04T13:39:48Z

I tested it locally and it works beautifully 👍 Didn't test in depth, though. Do we want to the testing protocol for that?

jstriebel · 2019-03-04T17:42:56Z

@daniel-wer I added a dummy max segmentation id and also the dataFormat field (open PR draft). The resolutions seem to be formatted wrong, I'm looking into that tomorrow in detail. See scalableminds/webknossos-connect#64

…-wk-connect-dataset

daniel-wer · 2019-03-05T16:06:04Z

@jstriebel Nice, thanks :) Should we add some documentation regarding wk-connect in this PR (or does some documentation exist already)?

@fm3 Could you update the migration guide? A datastore update is not required afaics, right?

I'll update the changelog and do one more testing round.

fm3 · 2019-03-05T17:09:45Z

Correct, no changes to datastore or tracingstore. I updated the migration log :)

jstriebel · 2019-03-06T10:51:21Z

There is no documentation available so far. I would do that in a separate PR, and then also consider to include wk-connect in the docker-compose setup to try it out easily.

jstriebel · 2019-03-06T10:52:58Z

see #3867

…-wk-connect-dataset

daniel-wer · 2019-03-06T15:05:30Z

I tested and the important things worked very well, editing is no longer a problem, great work :) 🎉

I encountered these issues (I am fully aware that wk-connect is not feature complete and WIP, but I think it has value that we collect and list these issues :) ):

"Refresh" causes a 500 for wk-connect (the /data/triggers/checkInboxBlocking route). On the wk-connect console there is an error like:
```
[2019-03-06 15:27:39 +0100] [1] [ERROR] Exception occurred while handling uri: 'http://localhost:8000/data/triggers/checkInboxBlocking?token=I1A62i1_TKzotZ1jVrnqRw'
...
 aiohttp.client_exceptions.ClientResponseError: 401, message='Unauthorized'
```
@jstriebel Is this simply not implemented and wk-connect should be excluded from this check or is there another issue (seems to work on demo)?
Same for "Reload" for a wk-connect dataset (/data/triggers/reload/...).
The "Find Data" route does not work yet (data/datasets/Connectomics_Department/kasthuri2011/layers/corrected_image/findData) - probably not very high prio, not blocking
4-bit data loading does not work yet. I saw that you have an assertion in place in the wk-connect backend, but we should show some kind of warning in the frontend (or disable the setting)

If we fix the first two issues and maybe take care of the fourth, this should be good to merge imo :)

jstriebel · 2019-03-06T15:56:49Z

@daniel-wer
Reload and Refresh were broken during the development of the mentioned PR, but are fixed in the meantime. It worked for me with the latest master branch version.
I documented that the find-data route is missing now here, thanks for the note!
scalableminds/webknossos-connect#22
About the 4bit: I can also look into implementing that in the next days, should not be super-complicated.

jstriebel · 2019-03-06T18:04:19Z

Re 4bit-mode: I opened a PR that implements 4bit-mode support: scalableminds/webknossos-connect#67

…-wk-connect-dataset

daniel-wer · 2019-03-12T14:19:05Z

@philippotto If you approve I would go ahead and merge this tomorrow :)

philippotto

Sorry, I forgot to approve!

daniel-wer and others added 5 commits February 27, 2019 13:30

add first version of add remote dataset view

656e944

add isConnector property to datastores

ca790e8

add dataset tab to import wk-connect datasets, refactor add dataset s…

2618efa

…tate handling

only show wk-connect tab if there are wk-connect datastores

372c749

Update dataStores.csv

149e47d

daniel-wer added backend frontend labels Feb 27, 2019

daniel-wer self-assigned this Feb 27, 2019

daniel-wer added 2 commits February 28, 2019 15:34

Merge branch 'master' of github.com:scalableminds/webknossos into add…

27c4eb8

…-wk-connect-dataset

add some comments

7885b9f

daniel-wer requested a review from philippotto February 28, 2019 15:22

update snapshots

4e93e56

philippotto reviewed Mar 4, 2019

View reviewed changes

daniel-wer added 4 commits March 5, 2019 14:18

Merge branch 'master' of github.com:scalableminds/webknossos into add…

7bd54a1

…-wk-connect-dataset

apply PR feedback

afa284a

Merge branch 'master' of github.com:scalableminds/webknossos into add…

a5e05de

…-wk-connect-dataset

apply PR feedback #2

970d167

daniel-wer and others added 2 commits March 5, 2019 17:19

update changelog

68341c9

update migration log

63d010a

Merge branch 'master' of github.com:scalableminds/webknossos into add…

55ef403

…-wk-connect-dataset

daniel-wer added 2 commits March 6, 2019 16:06

fix issues after sample datasets merge, use generic useFetch

8acfcdc

fix lint

187816c

daniel-wer marked this pull request as ready for review March 11, 2019 10:35

Merge branch 'master' of github.com:scalableminds/webknossos into add…

5d07aad

…-wk-connect-dataset

philippotto approved these changes Mar 13, 2019

View reviewed changes

Merge branch 'master' into add-wk-connect-dataset

1b645e4

daniel-wer merged commit 9f83012 into master Mar 13, 2019

daniel-wer deleted the add-wk-connect-dataset branch March 13, 2019 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to add datasets through wk-connect #3843

Allow to add datasets through wk-connect #3843

daniel-wer commented Feb 27, 2019 •

edited

Loading

philippotto left a comment

philippotto Mar 4, 2019

philippotto Mar 4, 2019

philippotto Mar 4, 2019

philippotto Mar 4, 2019

daniel-wer Mar 5, 2019

philippotto Mar 4, 2019

philippotto Mar 4, 2019

daniel-wer Mar 5, 2019

philippotto Mar 4, 2019

daniel-wer Mar 5, 2019

philippotto Mar 5, 2019

philippotto Mar 4, 2019

daniel-wer Mar 5, 2019

philippotto Mar 4, 2019

philippotto Mar 4, 2019

daniel-wer Mar 5, 2019

philippotto commented Mar 4, 2019

jstriebel commented Mar 4, 2019 •

edited

Loading

daniel-wer commented Mar 5, 2019

fm3 commented Mar 5, 2019

jstriebel commented Mar 6, 2019

jstriebel commented Mar 6, 2019

daniel-wer commented Mar 6, 2019 •

edited

Loading

jstriebel commented Mar 6, 2019

jstriebel commented Mar 6, 2019

daniel-wer commented Mar 12, 2019

philippotto left a comment

Allow to add datasets through wk-connect #3843

Allow to add datasets through wk-connect #3843

Conversation

daniel-wer commented Feb 27, 2019 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippotto commented Mar 4, 2019

jstriebel commented Mar 4, 2019 • edited Loading

daniel-wer commented Mar 5, 2019

fm3 commented Mar 5, 2019

jstriebel commented Mar 6, 2019

jstriebel commented Mar 6, 2019

daniel-wer commented Mar 6, 2019 • edited Loading

jstriebel commented Mar 6, 2019

jstriebel commented Mar 6, 2019

daniel-wer commented Mar 12, 2019

philippotto left a comment

Choose a reason for hiding this comment

daniel-wer commented Feb 27, 2019 •

edited

Loading

jstriebel commented Mar 4, 2019 •

edited

Loading

daniel-wer commented Mar 6, 2019 •

edited

Loading