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

Remove the notebook 5.1 check for large uploads #9616

Merged
merged 2 commits into from Jan 14, 2021

Conversation

jasongrout
Copy link
Contributor

References

Fixes #9601

Code changes

In JLab 3, we are using Jupyter Server, which unfortunately advertises its own version as the notebook version (1.2.1, for example). This trips up this notebook version test.

Since we do not support the notebook server for JupyterLab 3, we can eliminate the version check entirely.

I also normalized the spelling of 'canceled' in the error message to what we are using elsewhere in jlab.

User-facing changes

Large uploads (>15MB) are supported in JLab 3.

Backwards-incompatible changes

In JLab 3, we are using Jupyter Server, which unfortunately advertises its own version as the notebook version (1.2.1, for example). This trips up this notebook version test.

Since we do not support the notebook server for JupyterLab 3, we can eliminate the version check entirely.

Fixes jupyterlab#9601
@jasongrout jasongrout added this to the 3.0 milestone Jan 13, 2021
@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073
Copy link
Member

Ah, there is one failing test that is relevant: https://github.com/jupyterlab/jupyterlab/pull/9616/checks?check_run_id=1697725231#step:9:145

@jasongrout jasongrout force-pushed the largeupload branch 2 times, most recently from eb27f13 to ea19a45 Compare January 14, 2021 07:54
@jasongrout
Copy link
Contributor Author

Ah, there is one failing test that is relevant: https://github.com/jupyterlab/jupyterlab/pull/9616/checks?check_run_id=1697725231#step:9:145

All green now except for the ever-failing services test.

@jasongrout
Copy link
Contributor Author

@blink1073 - if you could re-review after the test changes above, that would be great. Thanks!

@blink1073
Copy link
Member

Looks good now, thanks!

@blink1073 blink1073 merged commit 3200010 into jupyterlab:master Jan 14, 2021
1 check passed
@krassowski
Copy link
Member

It seems that lab installations using JupyterHub are still running on notebook rather than on jupyter_server, a thing that causes problem to the lsp extension users as when I ported it I assumed I do not need notebook integration anymore (which JupyterLab itself kept). Would this affect the notebook version check and this workaround for legacy installations?

jupyterhub/jupyterhub@f69ef9f

@jasongrout
Copy link
Contributor Author

I was under the assumption that jlab 3 only ran on Jupyter server, not on notebook. Am I wrong?

@jasongrout
Copy link
Contributor Author

If I am wrong, then yes indeed this check would need to be added back, if we supported notebook < 5.1

@krassowski
Copy link
Member

krassowski commented Jan 15, 2021

My very likely wrong impression is that it runs under whatever is available. I came to think that after the users reports from JupyterHub users (running JupyterLab 3.0 for whom our server extension for jupyter_server is not picked up, but the older one for notebook is), seeing the commit mentioned above, and based on that JupyterLab ships with JSON for both notebook and jupyter_server: https://www.github.com/jupyterlab/jupyterlab/tree/master/setup.py

@blink1073
Copy link
Member

JupyterHub uses the classic notebook by default, but can be configured to use Jupyter server using an environment variable. JupyterLab can still also run on the classic notebook.

@krassowski
Copy link
Member

So the lack of hard breakage seems to be biting the hub users when they try to use an extension with a server component (of note while lab ships both jupyter_notebook_config.d and jupyter_server_config.d, extensions only ship jupyter_server_config.d - see the cookiecutter). This is probably going to bite jupyterlab-git an others too.

Would it be wise to remove jupyter_notebook_config.d and break early to prevent some weird configurations? The current situation will lead to bug reports accumulating on extensions repositories I think and they may not have resources to provide hub support (let alone some exotic deployments).

Or, is it intentional to support notebook and should the cookicutter and extensions and this PR be updated?

@jasongrout
Copy link
Contributor Author

JupyterLab can still also run on the classic notebook.

Uh-oh. I assumed otherwise. I hope this isn't the only place where I made that assumption.

We'll need to put this 5.1 check in if JupyterLab still runs on notebook 5.0. Do we support notebook 5.0, or do we at least assume notebook>=5.1?

jasongrout added a commit to jasongrout/jupyterlab that referenced this pull request Jan 16, 2021
In jupyterlab#9616, I did not realize that JupyterLab still supports the Jupyter Notebook server. This fix supports Jupyter Notebook as a server.

Jupyter Server advertises itself as notebook version 1.2 or so at the time of this commit. JupyterLab 2 supports Jupyter Notebook >=4.3.1. Thus we assume that if we get a version number less than 4, we are actually talking to Jupyter Server, and chunked uploading is supported.

Eventually this code should be removed when JupyterLab no longer supports Jupyter Notebook server, and hopefully that is before Jupyter Server 4.0.
jasongrout added a commit to jasongrout/jupyterlab that referenced this pull request Jan 16, 2021
In jupyterlab#9616, I did not realize that JupyterLab still supports the Jupyter Notebook server. This fix supports Jupyter Notebook as a server.

Jupyter Server advertises itself as notebook version 1.2 or so at the time of this commit. JupyterLab 2 supports Jupyter Notebook >=4.3.1. Thus we assume that if we get a version number less than 4, we are actually talking to Jupyter Server, and chunked uploading is supported.

Eventually this code should be removed when JupyterLab no longer supports Jupyter Notebook server, and hopefully that is before Jupyter Server 4.0.
@jasongrout
Copy link
Contributor Author

Another fix is in #9628 that re-enables the block on notebook < 5.1.

jasongrout added a commit to jasongrout/jupyterlab that referenced this pull request Jan 16, 2021
In jupyterlab#9616, I did not realize that JupyterLab still supports the Jupyter Notebook server. This fix supports Jupyter Notebook as a server.

Jupyter Server advertises itself as notebook version 1.2 or so at the time of this commit. JupyterLab 2 supports Jupyter Notebook >=4.3.1. Thus we assume that if we get a version number less than 4, we are actually talking to Jupyter Server, and chunked uploading is supported.

Eventually this code should be removed when JupyterLab no longer supports Jupyter Notebook server, and hopefully that is before Jupyter Server 4.0.

Fixes jupyterlab#9626
jasongrout added a commit to jasongrout/jupyterlab that referenced this pull request Jan 16, 2021
In jupyterlab#9616, I did not realize that JupyterLab still supports the Jupyter Notebook server. This fix supports Jupyter Notebook as a server.

Jupyter Server advertises itself as notebook version 1.2 or so at the time of this commit. JupyterLab 2 supports Jupyter Notebook >=4.3.1. Thus we assume that if we get a version number less than 4, we are actually talking to Jupyter Server, and chunked uploading is supported.

Eventually this code should be removed when JupyterLab no longer supports Jupyter Notebook server, and hopefully that is before Jupyter Server 4.0.

Fixes jupyterlab#9626
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jul 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2021
@jupyterlab jupyterlab unlocked this conversation Aug 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:filebrowser status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove 15MB upload limit
3 participants