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

Download Sample Datasets #3725

Merged
merged 29 commits into from
Mar 6, 2019
Merged

Download Sample Datasets #3725

merged 29 commits into from
Mar 6, 2019

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Feb 4, 2019

URL of deployed dev instance (used for testing):

Steps to test:

  • new routes GET http://localhost:9000/data/datasets/sample/Connectomics_Department?token=secretScmBoyToken and POST http://localhost:9000/data/datasets/sample/Connectomics_Department/Sample_Cremi_dsA_plus/download?token=secretScmBoyToken
  • corresponding view can be found next to upload dataset (“or add sample dataset”)

Issues:


@fm3 fm3 self-assigned this Feb 4, 2019
@fm3 fm3 changed the title [WIP] Download Demo dataset [WIP] Download Sample datasets Feb 12, 2019
@daniel-wer
Copy link
Member

daniel-wer commented Feb 14, 2019

@fm3 I added a first version of a sample datasets modal that shows the available sample datasets and allows to add them to webKnossos. To make it easy to test, I added a button in the dataset view.

I noticed that it seems like the downloading status is never reported. When triggering the download, the status remains "available" for some seconds and then switches to "present" directly. Or maybe the downloading state is only there very briefly (I'm polling every 500ms), ideally it would be set directly after triggering the download :)

The remaining functionality works very well! 🎉

Threre's still stuff missing in the frontend:

  • Decide where exactly the option to add sample datasets should be available
  • Choose the correct datastore (or datastores?) to download from (hardcoded as localhost:9000 for now).

@fm3
Copy link
Member Author

fm3 commented Feb 18, 2019

Thanks @daniel-wer !
The modal looks good :)
I found the issue causing the “downloading” state to not be reported. That should be fixed now.
Another thing we should consider: if the download fails in the back-end, the status goes back to available (not sure if setting it to failed would really work well, raising the question when that should be resetted). Could the frontend interpret repeated “available”s after the download request was started as “failed”, until the page is reloaded? I’m not sure what behavior I’d prefer here, maybe we can talk about that.

Also, if the first request fails, the download wasn’t started (there are a couple of checks first), which is also not reflected in the modal’s behavior right now, it will be polling indefinitely.

@daniel-wer
Copy link
Member

Another thing we should consider: if the download fails in the back-end, the status goes back to available (not sure if setting it to failed would really work well, raising the question when that should be resetted). Could the frontend interpret repeated “available”s after the download request was started as “failed”, until the page is reloaded? I’m not sure what behavior I’d prefer here, maybe we can talk about that.

Is it guaranteed, that when requesting the sample dataset status after triggering the download of dataset X (and having waited for the response), that the status for X === "available" if and only if the download request failed or could it be that the status for X is STILL available, because the internal data structures were not updated yet?
If the former is guaranteed, then this should be pretty easy, otherwise it'll probably get a bit dirty.

Also, if the first request fails, the download wasn’t started (there are a couple of checks first), which is also not reflected in the modal’s behavior right now, it will be polling indefinitely.

I fixed that :)

@fm3
Copy link
Member Author

fm3 commented Feb 18, 2019

Is it guaranteed, that when requesting the sample dataset status after triggering the download of dataset X (and having waited for the response), that the status for X === "available" if and only if the download request failed

I shoud think so, yes.

I fixed that :)

Cool, thanks!

@fm3
Copy link
Member Author

fm3 commented Feb 18, 2019

note to self, todo: add deterministic sorting to dataStoreDAO.findAll done

@daniel-wer
Copy link
Member

I added the option to add Sample Datasets to a couple of spots (as we discussed):

  • A link at the bottom of the Add Dataset view, which allows to add sample datasets even when there are existing datasets already.
  • As a placeholder for the dataset list if there are no datasets, see below:

no-datasets

  • As part of the onboarding flow - Add Dataset step. I've reworked this step quite a bit to incorporate the sample dataset option (which is the most prominent option by design as it is the easiest), see below:

onboarding-datasets

I think this is ready for a first review round. Any feedback regarding the design, copy and functionality appreciated :)

@daniel-wer
Copy link
Member

Note that the current backend version expects http://localhost/cremi.zip to be a zipped dataset for testing.
We'll have to think about where we want to host the sample datasets (and which).

@fm3
Copy link
Member Author

fm3 commented Feb 20, 2019

the docs link to https://static.webknossos.org/data/e2006_wkw.zip. I think, the cremi one actually also makes sense (small, includes segmentation)

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 work! I'll approve once the proper hosting is done :)

conf/messages Outdated Show resolved Hide resolved

const SampleDatasetsModal = ({ destroy, onOk, organizationName }: Props) => {
const [pendingDatasets, setPendingDatasets] = useState([]);
const [datastores] = useDatastores();
Copy link
Member

Choose a reason for hiding this comment

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

Could useDatastores directly return datastores without the array wrapper or do hooks dictate/encourage the current way?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it absolutely could (and probably should). At first I wanted to maintain the hooks pattern of returning [value, valueSetter], but that didn't really work out ^^


useInterval(fetchDatasets, pendingDatasets.length ? 1000 : null);

const handleSampleDatasetDownload = async (name: string) => {
Copy link
Member

@philippotto philippotto Feb 21, 2019

Choose a reason for hiding this comment

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

I'm wondering whether it makes sense to move handleSampleDatasetDownload, the pendingDatasets definition and the useDatastores call into the useSampleDatasets hook. At the moment, it feels a bit weird that failedDatasets are provided by the useSampleDatasets hook, but the pendingDatasets have to be maintained at an higher level.
handleSampleDatasetDownload would be another return argument of useSampleDatasets then.

This would clean up the view logic here a bit, but obviously the useSampleDatasets hook would get longer. I'd argue, that the abstraction level is more "even", though. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I struggled with that as well, thinking along the same lines ^^ I'll give this option a try and see what I can do to clean this up a little bit :)

organizationName: string,
};

function useDatastores(): [Array<APIDataStore>] {
Copy link
Member

Choose a reason for hiding this comment

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

I imagine that these ten lines of code will be useful quite a lot in the future (independent of entity type, of course). We don't have to do it now, but I'd think that something like const datastores = useAsyncGet(getDataStores, []) would be a good abstraction (naming is debatable).

Copy link
Member

Choose a reason for hiding this comment

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

I created a generic useFetch method and included a third parameter for the dependencies (for useEffect) :)

@normanrz
Copy link
Member

@fm3
Copy link
Member Author

fm3 commented Feb 27, 2019

@daniel-wer I added the three datasets from above. Could you display the description in the front-end? I added it as description in the json. It contains line-breaks, not sure how that is best handled.

@daniel-wer
Copy link
Member

@fm3 I took care of the descriptions :)
During testing of the sample datasets, I noticed that the Sample_FD0144_wkw dataset cannot be openend, because no resolutions are sent as part of the dataset request. The datasource config (in the Edit view) does contain resolutions, though, so I'm not sure what the problem is. Maybe you could have a look at that?

@fm3
Copy link
Member Author

fm3 commented Feb 28, 2019

There’s two layers named “color_2” in the datasource-properties.json in FD0144_wkw.zip. @normanrz could you update it? I suppose one of them should be “color_3”. Error handling for this is not great, but I don’t know a quick way to fix that. Created #3845 to track.

backend: 2019-02-28 13:33:59,110 [ERROR] models.binary.DataSetDataLayerDAO - SQL Error: org.postgresql.util.PSQLException: ERROR: duplicate key value violates unique constraint "dataset_layers_pkey"
backend:   Detail: Key (_dataset, name)=(5c77d537640100b601f3b0b5, color_2) already exists.

@fm3 fm3 changed the title [WIP] Download Sample datasets Download Sample Datasets Mar 4, 2019
@fm3
Copy link
Member Author

fm3 commented Mar 4, 2019

Thanks for fixing that @normanrz! one last thing: the real bounding box for that dataset (FD0144_wkw) seems to be smaller (I don’t see data for z>=164) so to optimize the user experience we might want to add that to the datasource-properties.json. Shouldn’t block the PR, though

@normanrz
Copy link
Member

normanrz commented Mar 4, 2019

fixed

@fm3
Copy link
Member Author

fm3 commented Mar 5, 2019

Cool, thanks! I updated the docs. If you have no objections @philippotto @normanrz I suggest we can merge this today

Identify a dataset that your interested in and click on `Start Skeleton Tracing` to create a new skeleton annotation.
webKnossos will launch the main annotation screen allowing you to navigate your dataset and place markers to reconstruct skeletons.
To get started with your first annotation, navigate to the `Datasets` tab on your [dashboard](./dashboard.md).
Identify a dataset that your interested in and click on `Start Skeleton Tracing` to create a new skeleton annotation.
Copy link
Member

Choose a reason for hiding this comment

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

I know that you only changed the whitespace here, but it should be you are 🙈

@philippotto
Copy link
Member

Docs look good to me 👍

@fm3 fm3 merged commit 5e7016f into master Mar 6, 2019
@fm3 fm3 deleted the demo-dataset branch March 6, 2019 10:17
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.

Pre-package WK with demo datasets
4 participants