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

JupyterLab Server as Server Extension #79

Merged
merged 84 commits into from
Aug 4, 2020

Conversation

echarles
Copy link
Member

@echarles echarles commented Oct 26, 2019

This PR migrate the existing ServerApp and Handler as Jupyter Server Extensions. jupyterlab/jupyterlab#7416 must also be considered.

  • Migrate ServerApp and Handlers to Extensions
  • Migrate tests to use PyTest plugin pytest_jupyter_server
  • Create PyTest pytest_jupyterlab_server to use by other libraries
  • Pin to a released jupyter_server

@echarles echarles changed the title [WIP] JupyterLab Server as Server Extension JupyterLab Server as Server Extension Nov 5, 2019
@echarles
Copy link
Member Author

echarles commented Nov 5, 2019

Removed WIP from title. This PR is ready for review.

@vidartf
Copy link
Member

vidartf commented Nov 7, 2019

Just a comment from a quick read through.

setup.py Show resolved Hide resolved
@bollwyvl
Copy link
Contributor

bollwyvl commented Aug 3, 2020

I put that coverage threshold stuff in there: we're not in a great place, coverage-wise, but seems like a lot to give up. While we do have the report from that external vendor, now, having it as a real test shows we care.

Also re: installing jupyter_server: can this be done as a separate set of test excursions, where notebook is not installed? Tox can union coverage across multiple runs, I think, so i think it might be a good choice to support, while still keeping ci clean.

More broadly, this project will probably need to accept people will continue to be running notebook for a while, if they have invested in custom tooling, but will still want the newer features.

Dropping compatibility will need to be clearly signaled with warnings, potentially all the way up to the UI, well before it is officially dropped (as part of a major version bump).

Totally astronaut architecting here: it may be worth considering, in a future effort, the treating of the dependency resolutions as entire separate packages, e.g. jupyterlab_server_classic and jupyterlab_server_jserver... and adopting the manager pattern, where the logic moved into a couple Managers, while the handlers and extensions lived in the appropriate packages. This would demonstrate it's possible to run lab on top of different (async) servers, which has been expressed as being important to some downstreams that want to bring their own X for different parts of the stack. I could see this being clutch for themes and translations, where a large deployment would want to centralize these.

@echarles
Copy link
Member Author

echarles commented Aug 3, 2020

I put that coverage threshold stuff in there: we're not in a great place, coverage-wise, but seems like a lot to give up. While we do have the report from that external vendor, now, having it as a real test shows we care.

Sure, I removed that while trying to get CI green (it was not picking the def in setup.py as it was before) - Will put that back in place as soon as we have more CI stablity.

Also re: installing jupyter_server: can this be done as a separate set of test excursions, where notebook is not installed? Tox can union coverage across multiple runs, I think, so i think it might be a good choice to support, while still keeping ci clean.

I was able to install jupyter_server from github by defining it in setup.py - Now itt does not work anymore... Sorry for not mentioning that (did that the jupyter_server gitter channel) - The plan is to depend soon on a jupyter_server release that @Zsailer will do.

More broadly, this project will probably need to accept people will continue to be running notebook for a while, if they have invested in custom tooling, but will still want the newer features.

JupyterLab users will continue to be able to use notebook via the nbclassic package, see jupyterlab/jupyterlab#7416

Dropping compatibility will need to be clearly signaled with warnings, potentially all the way up to the UI, well before it is officially dropped (as part of a major version bump).

We have ensured full backward compatilbity via nbclassic (https://github.com/Zsailer/nbclassic) with log warning for traits to be moved to ServerApp and for existing server extensions.

Totally astronaut architecting here: it may be worth considering, in a future effort, the treating of the dependency resolutions as entire separate packages, e.g. jupyterlab_server_classic and jupyterlab_server_jserver... and adopting the manager pattern, where the logic moved into a couple Managers, while the handlers and extensions lived in the appropriate packages. This would demonstrate it's possible to run lab on top of different (async) servers, which has been expressed as being important to some downstreams that want to bring their own X for different parts of the stack. I could see this being clutch for themes and translations, where a large deployment would want to centralize these.

There is obviously room for evolution, (re)packaging and modularity once we can jlab as jserver extension in (which is the goal of this PR).

@echarles
Copy link
Member Author

echarles commented Aug 3, 2020

I will also have a look at https://github.com/jupyterlab/jupyterlab_server/pull/79/files#diff-f63568f03d848bfe4c05811be269b915R90 (not sure why the json response comes back with a different layout)

    # TODO(@echarles) Check this...
        first_created = rfc3339_to_timestamp(data['created'])	#    assert list_data['created'] == data['created']
        first_modified = rfc3339_to_timestamp(data['last_modified'])	#    assert list_data['last_modified'] == data['last_modified']

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2020

Codecov Report

Merging #79 into master will decrease coverage by 12.65%.
The diff coverage is 78.02%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #79       +/-   ##
===========================================
- Coverage   72.61%   59.95%   -12.66%     
===========================================
  Files          18       19        +1     
  Lines        1340     1391       +51     
===========================================
- Hits          973      834      -139     
- Misses        367      557      +190     
Impacted Files Coverage Δ
jupyterlab_server/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
jupyterlab_server/process.py 0.00% <0.00%> (ø)
jupyterlab_server/process_app.py 0.00% <0.00%> (ø)
jupyterlab_server/server.py 0.00% <0.00%> (-50.00%) ⬇️
jupyterlab_server/themes_handler.py 0.00% <ø> (-30.31%) ⬇️
jupyterlab_server/translations_handler.py 45.71% <0.00%> (-45.96%) ⬇️
jupyterlab_server/tests/utils.py 28.26% <20.68%> (-64.38%) ⬇️
jupyterlab_server/app.py 18.18% <24.00%> (-81.82%) ⬇️
jupyterlab_server/settings_handler.py 71.00% <25.00%> (-16.42%) ⬇️
jupyterlab_server/workspaces_handler.py 66.94% <25.00%> (-19.26%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4afa81...a052cfb. Read the comment docs.

@blink1073 blink1073 closed this Aug 4, 2020
@blink1073 blink1073 reopened this Aug 4, 2020
@Zsailer Zsailer merged commit db02cf4 into jupyterlab:master Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants