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

Allow larger files uploads #4224

Merged
merged 14 commits into from Jun 6, 2018
Merged

Allow larger files uploads #4224

merged 14 commits into from Jun 6, 2018

Conversation

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Mar 22, 2018

This fixes #1735 by allowing files larger than 15 MB to be uploaded, when you are running notebook version >= 5.1. If you are running older notebook versions, the old behavior is intact of denying the large upload. On newer versions, it ads a warning if you are over the 15 MB limit, which copies the behavior of Jupyter Notebook.

  • add notebook version information to the client
  • disallow large uploads on old version
@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Mar 22, 2018

Ah, just realized I will actually have to modify the _upload function as well to send it chunked as well, so it doesn't freeze the browser.

But we should keep the existing _upload for smaller smaller files and notebook older than 5.1, so I will make another _uploadChunked function.

I will also need to figure out where the progress bar should live in the UI.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Mar 23, 2018

The notebook server will save the chunks as they come in. So I think it will show up in the file browser before it is finished uploading. If we could mark it as somehow "not finished" we could not let anything open it till its done and put some progress icon on it.

@jasongrout jasongrout added this to the Beta 2 milestone Mar 28, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 28, 2018

Do you think we should bump this to beta 3? It looks a bit bigger than we thought originally.

@saulshanabrook saulshanabrook removed this from the Beta 2 milestone Mar 28, 2018
@saulshanabrook saulshanabrook added this to the Beta 3 milestone Mar 28, 2018
@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Mar 28, 2018

@jasongrout Yep, I think that makes sense. I have bumped it

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Mar 28, 2018

@gnestor Do you have thoughts on how these chunked file uploads should be handled in the UI? There could be multiple files uploading at once and it would also be nice to have some way for users to cancel these uploads halfway through.

@gnestor
Copy link
Contributor

@gnestor gnestor commented Mar 30, 2018

@saulshanabrook I'm on vacation at the moment so I haven't had a time to sit down and look at this, but I'd be happy to help out early next week 👍

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Mar 30, 2018

Sounds good. Another thing to think about is maybe we should warn the user if they try to close the page while file chunks are uploading. If they do this, they will end up with half uploaded files.

@gnestor
Copy link
Contributor

@gnestor gnestor commented Mar 31, 2018

I agree. I'm not sure if there is a JupyterLab-specific way of doing this, but the docs for doing it the traditional way: https://developer.mozilla.org/en-US/docs/Web/Events/beforeunload

@saulshanabrook saulshanabrook mentioned this pull request Apr 4, 2018
@gnestor
Copy link
Contributor

@gnestor gnestor commented Apr 5, 2018

I tried to test this with a 45mb file and I'm still getting the 15mb limit error. Is that expected?

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 5, 2018

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 5, 2018

screen shot 2018-04-05 at 3 01 33 pm

@gnestor
Copy link
Contributor

@gnestor gnestor commented Apr 5, 2018

I tried again and bumped into a new error:

dialog.ts:54 SyntaxError: Unexpected token & in JSON at position 10
    at JSON.parse (<anonymous>)
    at Object.getNotebookVersion (pageconfig.ts:168)
    at FileBrowserModel.<anonymous> (model.ts:326)
    at step (model.ts:2)
    at Object.next (model.ts:2)
    at model.ts:2
    at new Promise (<anonymous>)
    at webpackJsonp.0fw8.__awaiter (model.ts:2)
    at FileBrowserModel.webpackJsonp.0fw8.FileBrowserModel.upload (model.ts:325)
    at upload.ts:76

I'm running master and notebookVersion = "[5, 5, 0, &#34;.dev0&#34;]" for me (which decoded = '[5, 5, 0, ".dev0"]'.

A simple fix would be:

return JSON.parse(notebookVersion.replace(/, &#34;.+&#34;/, ''));

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 6, 2018

@gnestor Good catch! I just pushed a change that now properly escapes the version number so it can be parsed easily on the frontend. It now show up like "notebookVersion": "[5, 5, 0, \".dev0\"]" in the HTML.

That way it will work with whatever characters are in the version string.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 6, 2018

I am trying to figure out where to store the file progress events. In some ways, it would make the most sense as part of the contents model, so that every file could have a progress like attribute. However, the backend doesn't store any information about if an upload is only partial. And we are just returning the parsed model that the backend uses for each file.

So now I am thinking this needs to be it's own structure on the model. For the list of files, there is fileChanged and items, which make up the observable. So I could create a parallel one for file upload progress.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 10, 2018

I added a beforeunload event listener that warns if a file is still uploading.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 23, 2018

@don-lab-dc
Copy link

@don-lab-dc don-lab-dc commented May 4, 2018

Very much looking forward to this, as uploads are now the only reason I switch from lab to tree.

@saulshanabrook saulshanabrook changed the title WIP: Allow larger files uploads Allow larger files uploads May 4, 2018
@saulshanabrook saulshanabrook requested a review from gnestor May 4, 2018
@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 4, 2018

I am still not sure how the UI should look to show upload progress. Maybe we could merge this as-is if that will take a bit longer to figure out?

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented May 15, 2018

I know we've been talking about having a 'status bar' of sorts a la VS Code's bottom status bar. This seems like a prime candidate do exist in that location.

Copy link
Contributor

@jzf2101 jzf2101 left a comment

LGTM. File w/ upload problems won't work in original directory. Other files I tested work fine.

Copy link
Contributor

@jzf2101 jzf2101 left a comment

Works for me

@jzf2101
Copy link
Contributor

@jzf2101 jzf2101 commented May 24, 2018

@ian-r-rose @tgeorgeux @gnestor does anyone have any thoughts before we merge?

@afshin afshin merged commit 7422d93 into jupyterlab:master Jun 6, 2018
2 checks passed
@Abhijit-2592
Copy link

@Abhijit-2592 Abhijit-2592 commented Jul 27, 2018

I am still getting the error 15mb limit.

Jupyter lab 0.32.1
Jupyter notebook 5.6.0

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 27, 2018

Hi @Abhijit-2592, this feature is available in 0.33.0, which was just released yesterday.

@datalee
Copy link

@datalee datalee commented Feb 1, 2019

ok,nice

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.