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

[WIP] Voila as an ExtensionApp #270

Closed
wants to merge 22 commits into from

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Jun 25, 2019

I went ahead and started refactoring Voila to use the ExtensionApp in jupyter_server. It greatly simplifies the main application.

Using this API, Voila can be initialized, configured and launched from the command line, and you can generate configuration files using the --generate-config flag.

It can also be loaded as a Jupyter server extension, allowing you to serve Voila side-by-side with other client applications running on one server.

One thing to note is that all static files are now served behind the url namespace static/voila/ (this is automatically set up by ExtensionApp).

This should be working. Let me know if you have issues with testing it out.

@SylvainCorlay @maartenbreddels

@Zsailer Zsailer changed the title Voila as an ExtensionApp [WIP] Voila as an ExtensionApp Jun 25, 2019
@Zsailer Zsailer force-pushed the extensionapp branch 2 times, most recently from 388121a to 017ae58 Compare June 26, 2019 00:04
@Zsailer
Copy link
Member Author

Zsailer commented Jun 26, 2019

To test this out,

  1. Clone and install jupyter_server master (the latest release won't work).
  2. Checkout this PR's branch
  3. Install using pip install . (this updates the voila CLI and server extension entry points).

Start Voila by either 1) running directly:

jupyter voila

or loading as a server extension

jupyter server --ServerApp.default_url='/voila'`

@Zsailer
Copy link
Member Author

Zsailer commented Jun 26, 2019

Also, I've opened jupyter-server/jupyter_server#59 to simplify how extensions configuration gets passed to extension handlers. This should help simplify some of the changes here.

Once that PR is merged in jupyter_server, I'll update this PR.

@timkpaine
Copy link
Member

checking this now

@Zsailer
Copy link
Member Author

Zsailer commented Jul 3, 2019

Thanks, @timkpaine.

I think this will require a pretty significant refactor of Voila's tests. I'm happy to work on this if you can confirm that the ExtensionApp seems like a good solution for Voila.

@timkpaine
Copy link
Member

timkpaine commented Jul 3, 2019

@Zsailer I really would like to use my jupyterlab LoginHandler for Voila, rather than having to do any custom weirdness, so I think this makes sense to do. Im not a maintainer of voila though so its up for @SylvainCorlay to decide.

@Zsailer
Copy link
Member Author

Zsailer commented Jul 3, 2019

I really would like to use my jupyterlab LoginHandler for Voila

Definitely—though, you may have to wait for JupyterLab to adopt jupyter_server as it's core server (unless you're already using Lab without the classic notebook server).

@timkpaine
Copy link
Member

I don't need full compatibility, just the bare minimum

Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Excellent work! I like seeing all the red. Should this now work with the latest jupyter-server release?

setup.py Outdated Show resolved Hide resolved
@Zsailer
Copy link
Member Author

Zsailer commented Jul 25, 2019

@maartenbreddels Great! Glad you like it!

I just got back from holiday, so I'm catching up on emails+Github threads. I'm hoping to rebase this PR later this week. It should work with the latest jupyter_server, but I'll verify and ping you again soon.

@jtpio jtpio mentioned this pull request Jul 26, 2019
@Zsailer Zsailer force-pushed the extensionapp branch 3 times, most recently from 5358a80 to caa2b58 Compare August 3, 2019 05:43
@Zsailer
Copy link
Member Author

Zsailer commented Aug 3, 2019

Ok, once jupyter-server/jupyter_server#70 is merged, this should be working (with standalone mode).

Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Impressive work!
The test do not run on the CI because of the flake8 issues, it may be useful to run the following test locally:

py.test tests/app/execute_test.py

I also noticed that if I run jupyter-voila notebooks/basics.ipynb, it will open the wrong url (http://localhost:8891/notebooks/basics.ipynb?token=...)

voila/app.py Outdated Show resolved Hide resolved
@Zsailer
Copy link
Member Author

Zsailer commented Aug 7, 2019

The tests need some refactoring after these significant changes. I started working on that last week but didn't quite finish. I'll try to update the tests over the next day or two.

@Zsailer
Copy link
Member Author

Zsailer commented Aug 7, 2019

I also noticed that if I run jupyter-voila notebooks/basics.ipynb,

Good catch! Looking into this issue now.

@vidartf
Copy link
Contributor

vidartf commented Aug 21, 2019

I believe this would also be relevant? jupyter-server/jupyter_server#80

@timkpaine
Copy link
Member

@Zsailer anything I can do to help?

@Zsailer
Copy link
Member Author

Zsailer commented Dec 4, 2019

Closing in favor of #492. Thanks, @maartenbreddels!

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.

6 participants