Allow to continue upload after browser restart for same user #7981

MichaelBuessemeyer · 2024-08-07T12:37:49Z

URL of deployed dev instance (used for testing):

https://morerobustresumableupload.webknossos.xyz/

Steps to test:

Open the upload view on the dev instance: https://morerobustresumableupload.webknossos.xyz/datasets/upload?to=66b60af101000001003deac5
Open the dev tools. In the debugger open file dataset_upload_view.tsx and set a breakpoint in line 425 (before resumableUpload.upload(); is called upon the files added action.
Keep the dev tools open, upload some small dataset (to have less traffic)
- Enter dataset name, allowed teams, target folder and drag & drop a dataset zip
Hit the upload button. The upload should be reserved and then the debugger should kick in an interrupt starting the upload process.
Close your browser and do not skip the breakpoint.
Reopen the browser and reopen the upload view.
Open the dev tools to track the network traffic
The view should now show the dataset name you just tried to upload but manually interrupted before starting the upload (stopping during the upload should also work. Click the continue upload
The UI should be autofilled with the previous values and disabled.
Drag & drop the same file as before and hit upload
The upload should happily continue and succeed :)
Now test the network traffic. There should be get requests to the /datasets route testing whether a chunk is already present. They should be returning 204 if no chunk was previously uploaded. If you tweaked the testing such that some chunks were already uploaded, some of these requests should answer 200 (signaling the chunk is present) (see below)
Test the uploaded dataset. It should be viewable as usual.

Alternatively, for the debugger interruption, choose a larger dataset to upload and interrupt the upload in time.

TODOs:

Test whether this works with multi-file upload (e.g. multiple tiffs at once)

Issues:

fixes #7931

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

Updated changelog
Needs datastore update after deployment

…e-robust-resumable-upload

…iles

reenable checking for duplicate dataset names when not continuing an upload

MichaelBuessemeyer · 2024-08-12T12:59:19Z

@philippotto could you please review the frontend and @normanrz could you please review the backend code?

philippotto

cool stuff!! didn't test yet but already left some comments :)

philippotto · 2024-08-12T14:15:34Z

package.json

@@ -76,7 +76,7 @@
    "start": "node tools/proxy/proxy.js",
    "build": "node --max-old-space-size=4096 node_modules/.bin/webpack --env production",
    "@comment build-backend": "Only check for errors in the backend code like done by the CI. This command is not needed to run WEBKNOSSOS",
-    "build-backend": "yarn build-wk-backend && yarn build-wk-datastore && yarn build-wk-tracingstore",
+    "build-backend": "yarn build-wk-backend && yarn build-wk-datastore && yarn build-wk-tracingstore && rm webknossos-tracingstore/conf/messages webknossos-datastore/conf/messages",


why are these removed?

These message files are generated by the script as the tracingstore and datastore are built. When they are not deleted and one tries to run the script again the compilation of the two components somehow fails. After running into this multiple times (me having to manually delete these files) as well as taking care of not pushing these files, I simply wanted the script to also take care of cleaning up the auto-generated message files :)

philippotto · 2024-08-12T14:17:46Z

frontend/javascripts/admin/admin_rest_api.ts

@@ -1307,6 +1307,35 @@ export function reserveDatasetUpload(
  );
 }

+export type OngoingUpload = {
+  uploadId: string;
+  dataSourceId: { name: string; organizationName: string };


usually we call this datasetId in the frontend (see APIDatasetId). can we use that term here too?

sure, thanks. missed that.

But in the backend the name is usually called dataSourceId? So we do have a naming mismatch between the frontend and backend here 🥴?

frontend/javascripts/admin/dataset/dataset_upload_view.tsx

MichaelBuessemeyer

Thanks for your feedback philipp :)

I applied / commented everything :)

MichaelBuessemeyer · 2024-08-13T09:46:16Z

frontend/javascripts/admin/admin_rest_api.ts

@@ -1307,6 +1307,35 @@ export function reserveDatasetUpload(
  );
 }

+export type OngoingUpload = {
+  uploadId: string;
+  dataSourceId: { name: string; organizationName: string };


sure, thanks. missed that.

But in the backend the name is usually called dataSourceId? So we do have a naming mismatch between the frontend and backend here 🥴?

frontend/javascripts/admin/dataset/dataset_upload_view.tsx

MichaelBuessemeyer · 2024-08-13T10:04:42Z

package.json

@@ -76,7 +76,7 @@
    "start": "node tools/proxy/proxy.js",
    "build": "node --max-old-space-size=4096 node_modules/.bin/webpack --env production",
    "@comment build-backend": "Only check for errors in the backend code like done by the CI. This command is not needed to run WEBKNOSSOS",
-    "build-backend": "yarn build-wk-backend && yarn build-wk-datastore && yarn build-wk-tracingstore",
+    "build-backend": "yarn build-wk-backend && yarn build-wk-datastore && yarn build-wk-tracingstore && rm webknossos-tracingstore/conf/messages webknossos-datastore/conf/messages",


These message files are generated by the script as the tracingstore and datastore are built. When they are not deleted and one tries to run the script again the compilation of the two components somehow fails. After running into this multiple times (me having to manually delete these files) as well as taking care of not pushing these files, I simply wanted the script to also take care of cleaning up the auto-generated message files :)

philippotto · 2024-08-15T07:44:50Z

frontend/javascripts/admin/admin_rest_api.ts

    // Rename "team" to "organization" as this is the actual used current naming.
-    return ongoingUploads.map(({ dataSourceId: { name, team }, ...rest }) => ({
+    return ongoingUploads.map(({ datasetId: { name, team }, ...rest }) => ({


just tested this, and I think this needs to be changed back to dataSourceId here because the back-end uses that. otherwise, I get:

TypeError: Cannot read properties of undefined (reading 'name')

Oh right, thanks for spotting. Should be better now hopefully. will retest on monday

tested and worked :)

…e-robust-resumable-upload

MichaelBuessemeyer

Thanks for the feedback 🙏. It should be applied now except for one comment which I do not understand and need clarification. I commented the comment in question :)

MichaelBuessemeyer · 2024-08-21T15:23:04Z

...-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala

+      _ <- runningUploadMetadataStore.insert(
+        Json.stringify(Json.toJson(DataSourceId(reserveUploadInformation.name, reserveUploadInformation.organization))),
+        reserveUploadInformation.uploadId
+      )
      _ <- runningUploadMetadataStore.insert(
        redisKeyForLinkedLayerIdentifier(reserveUploadInformation.uploadId),


I think I dont full understand your comment 🤔

With

Now we are inserting two different types of data into the same set.

you mean, that now a stringified json object is used as a key while the other inserts always use a "string only" key?

And to not have this you would want to add a new store to redis just to save the necessary mapping from datasource id back to the ongoing upload info stored in redis? This acutally seemes a little overkill. I instead could implement something like redisKeyForDatasourceId or so and this would transform a given datasource id into a unique string. Does that sound better?

MichaelBuessemeyer · 2024-08-21T16:04:05Z

...-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala

+      foundOngoingUploads = maybeOngoingUploads.filter(idBox => idBox.isDefined).flatMap(idBox => idBox.value).collect {
+        case Some(value) => value
+      }


I found a more consice version:

foundOngoingUploads = maybeOngoingUploads.collect { case Full(Some(value)) => value }

The type of maybeOngoingUploads is List[Box[Option[OngoingUpload]]]

MichaelBuessemeyer · 2024-08-21T16:30:26Z

can the voxel size also be pre-filled? (for in needsConversion case)

This information is not present in the backend as it is only passed to the worker conversion job once the conversion job is started. Storing this in the backend would need some further implementation effort :)

would it be simple to do some sanity checks before I can hit upload again? e.g. number of files to upload is equal to that of the unfinished upload?

I think that would be possible. I would need to retrieve this information from Redis and then append it to the OngoingUploads case class objects (which will be renamed soon). I might need some time but definitely doable. The question: Should this be included in the first version?

MichaelBuessemeyer · 2024-08-21T17:23:19Z

And now everything should be named consistently. At least I tried 🙈. I'll do retesting tomorrow :)

MichaelBuessemeyer · 2024-08-22T04:58:25Z

I'll do retesting tomorrow :)

Should work :)

…e-robust-resumable-upload

fm3

I replied to the thread about redis keys. Other than that, the backend looks good to me now :)

MichaelBuessemeyer · 2024-08-23T11:41:11Z

And what about

can the voxel size also be pre-filled? (for in needsConversion case)

This information is not present in the backend as it is only passed to the worker conversion job once the conversion job is started. Storing this in the backend would need some further implementation effort :)

would it be simple to do some sanity checks before I can hit upload again? e.g. number of files to upload is equal to that of the unfinished upload?

I think that would be possible. I would need to retrieve this information from Redis and then append it to the OngoingUploads case class objects (which will be renamed soon). I might need some time but definitely doable. The question: Should this be included in the first version?

?

fm3 · 2024-08-23T11:44:53Z

Ah, sorry, I missed that. Ok then let’s skip the pre-filling for voxel size. I think the little sanity check would be good to have already in this PR.

fm3

Nice, thanks for adding that! Worked for me! Backend LGTM, leaving final approval to frontend folks

…y and less guessy

…e-robust-resumable-upload

MichaelBuessemeyer · 2024-08-26T09:21:39Z

I tweaked the frontend check a little (compared to the version from Friday):

The frontend does no longer check for the exact order of files selected for the upload. It only ensures that files with the exact naming are present as from the initial upload try
The frontend now lists all files required for the continued upload in the error message. Previously, it was only guess work and thus didn't feel so good / easy to me when one file was missing and one had to guess which file might be missing for a multifile upload

frontend/javascripts/admin/dataset/dataset_upload_view.tsx

…e-robust-resumable-upload

MichaelBuessemeyer · 2024-08-27T11:30:46Z

Should be good now :)

…ds/webknossos into more-robust-resumable-upload

philippotto

🎉

last review said: "Backend LGTM, leaving final approval to frontend folks"

implement test route checking if chunk is present

daf0b18

MichaelBuessemeyer added backend frontend new feature labels Aug 7, 2024

MichaelBuessemeyer self-assigned this Aug 7, 2024

MichaelBuessemeyer added 5 commits August 7, 2024 14:38

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

81e321a

…e-robust-resumable-upload

WIP: Implement ongoing uploads listing route

74062d5

implement first version of robust resumable upload

f11e681

update build backend script command to remove autogenerated message f…

c68cb23

…iles

fix frontend typing

10b4dc1

MichaelBuessemeyer changed the title ~~implement test route checking if chunk is present~~ Allow to continue upload after browser restart Aug 8, 2024

MichaelBuessemeyer changed the title ~~Allow to continue upload after browser restart~~ Allow to continue upload after browser restart for same user Aug 8, 2024

MichaelBuessemeyer and others added 5 commits August 8, 2024 17:07

refactor code

e1b8ce3

format backend

da162dd

Merge branch 'master' into more-robust-resumable-upload

675d320

fix test file route

2eba2b9

clear uploadId after successful upload in frontend;

2cd362a

reenable checking for duplicate dataset names when not continuing an upload

MichaelBuessemeyer marked this pull request as ready for review August 9, 2024 14:25

format backend

7730af5

MichaelBuessemeyer requested review from philippotto and normanrz August 12, 2024 12:58

philippotto requested changes Aug 12, 2024

View reviewed changes

apply feedback

dad68b1

MichaelBuessemeyer commented Aug 13, 2024

View reviewed changes

philippotto reviewed Aug 15, 2024

View reviewed changes

MichaelBuessemeyer requested review from fm3 and removed request for normanrz August 16, 2024 08:25

MichaelBuessemeyer added 2 commits August 16, 2024 18:27

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

9bc5844

…e-robust-resumable-upload

fix expected format from backend when requesting ongoinguploads

9b903c6

apply feedback

a1e6d8e

MichaelBuessemeyer commented Aug 21, 2024

View reviewed changes

MichaelBuessemeyer added 2 commits August 21, 2024 18:31

remove unused import

2f2572c

consistent renaming to unfinished uploads

b127dc9

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

dc2c13e

…e-robust-resumable-upload

MichaelBuessemeyer requested a review from fm3 August 22, 2024 08:12

fm3 reviewed Aug 22, 2024

View reviewed changes

add sanity check that file names must be equal to initial upload

1b32460

fm3 reviewed Aug 26, 2024

View reviewed changes

MichaelBuessemeyer added 3 commits August 26, 2024 10:27

do not require same order of files

9fc50b0

include file names in error to make searching for these file more eas…

015e89f

…y and less guessy

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

3173885

…e-robust-resumable-upload

philippotto reviewed Aug 26, 2024

View reviewed changes

frontend/javascripts/admin/dataset/dataset_upload_view.tsx Outdated Show resolved Hide resolved

MichaelBuessemeyer added 2 commits August 27, 2024 13:09

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

57162e1

…e-robust-resumable-upload

fix "files not matching initial upload try" error message

8550faa

MichaelBuessemeyer and others added 3 commits August 27, 2024 13:30

Merge branch 'master' into more-robust-resumable-upload

8839129

add changelog entry

c42b737

Merge branch 'more-robust-resumable-upload' of github.com:scalablemin…

e958434

…ds/webknossos into more-robust-resumable-upload

philippotto approved these changes Aug 27, 2024

View reviewed changes

MichaelBuessemeyer merged commit 5ace6e2 into master Aug 27, 2024
2 checks passed

MichaelBuessemeyer deleted the more-robust-resumable-upload branch August 27, 2024 13:03

MichaelBuessemeyer mentioned this pull request Aug 30, 2024

Make filePaths for reserve upload route optional #8045

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to continue upload after browser restart for same user #7981

Allow to continue upload after browser restart for same user #7981

MichaelBuessemeyer commented Aug 7, 2024 •

edited

Loading

MichaelBuessemeyer commented Aug 12, 2024

philippotto left a comment

philippotto Aug 12, 2024

MichaelBuessemeyer Aug 13, 2024

philippotto Aug 12, 2024

MichaelBuessemeyer Aug 13, 2024

MichaelBuessemeyer left a comment

MichaelBuessemeyer Aug 13, 2024

MichaelBuessemeyer Aug 13, 2024

philippotto Aug 15, 2024

MichaelBuessemeyer Aug 16, 2024

MichaelBuessemeyer Aug 19, 2024

MichaelBuessemeyer left a comment

MichaelBuessemeyer Aug 21, 2024

MichaelBuessemeyer Aug 21, 2024

MichaelBuessemeyer commented Aug 21, 2024

MichaelBuessemeyer commented Aug 21, 2024

MichaelBuessemeyer commented Aug 22, 2024

fm3 left a comment

MichaelBuessemeyer commented Aug 23, 2024

fm3 commented Aug 23, 2024

fm3 left a comment

MichaelBuessemeyer commented Aug 26, 2024

MichaelBuessemeyer commented Aug 27, 2024

philippotto left a comment

Allow to continue upload after browser restart for same user #7981

Allow to continue upload after browser restart for same user #7981

Conversation

MichaelBuessemeyer commented Aug 7, 2024 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

TODOs:

Issues:

MichaelBuessemeyer commented Aug 12, 2024

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

MichaelBuessemeyer 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

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Aug 21, 2024

MichaelBuessemeyer commented Aug 21, 2024

MichaelBuessemeyer commented Aug 22, 2024

fm3 left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Aug 23, 2024

fm3 commented Aug 23, 2024

fm3 left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Aug 26, 2024

MichaelBuessemeyer commented Aug 27, 2024

philippotto left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Aug 7, 2024 •

edited

Loading