-
Notifications
You must be signed in to change notification settings - Fork 505
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
refactor: use jupyter_server ExtensionApp #492
refactor: use jupyter_server ExtensionApp #492
Conversation
This is great! Thank you, @maartenbreddels. |
We're rolling back jupyter_server to support Python 3.5 for a new release (0.2) with the ExtensionApp. This should be done by Friday. Then, we'll likely drop Python 3.5 in a jupyter_server 0.3 release. The new Kernel Provider work from @kevin-bates that we're hoping to land in master this next month requires Python 3.6 and above. |
Just to be clear, we can easily fix the 3.5 support issue in kernel provider stuff. There was a recent change that unexpectedly introduced a syntax error only on 3.5 - and we had cut the 0.5.0 release just prior to that discovery (thus the hesitancy to fix it for something that we thought was more near to its EOL). Of course, by the time jupyter-server 0.3.0 is developed, 3.5 may be deprecated, but we shouldn't let this hold up use of JKM. It's relatively easy to produce a patch release (0.5.1) that restores 3.5 support in JKM if necessary. |
Note: we have a constraint to support Python 3.5 for a little bit more in Voilà. Having jupyter_server be compatible with python 3.5 would be great! |
Ok, we've reverted jupyter_server to support 3.5 in jupyter-server/jupyter_server#150. |
Thanks @kevin-bates , @Zsailer and @echarles for correcting this before I even had time to reply 👏 Related: |
2d3cb87
to
4262d57
Compare
4262d57
to
13b3818
Compare
Just release Jupyter Server 0.2.0. Still supports Python 3.5 and includes all the ExtensionApp work (with tests, finally). Let us know if you run into any issues. I'm happy to help with this PR too, if you'd like. |
voila_app.initialize(voila_args + ['--no-browser']) | ||
voila_config(voila_app) | ||
voila_app.start() | ||
def voila_app(server_config, voila_config, voila_args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I released jupyter_server
0.2.1 (see changelog) today, which includes a pytest-plugin for jupyter_server.
Inside this plugin, we've added a serverapp
fixture and a configurable_serverapp
fixture that handle the creation + teardown of a jupyter server instance between each test so you don't have to.
You can see an example using this plugin here in nbclassic.
@@ -14,8 +14,8 @@ | |||
|
|||
|
|||
@pytest.mark.gen_test | |||
def test_hello_world(http_client, base_url): | |||
response = yield http_client.fetch(base_url) | |||
def test_hello_world(http_client, default_url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jupyter server pytest-plugin we added in jupyter_server 0.2.1 drastically improves this syntax.
- You can now use
async
/await
syntax. - we over a
fetch
fixture—a function for making requests to the jupyter server API and extension handlers.
To see an example, check out this simple unit test in Jupyter Server.
I mentioned in my comments, I released Jupyter Server 0.2.1 today. This includes a pytest-plugin for jupyter server extensions (like future Voila). This significantly improves the syntax when writing extension tests:
To start using today, |
This looks great! Just tried locally with |
Opened jupyter-server/jupyter_server#175 for the |
I think we also need this change: jupyter-server/jupyter_server#172 |
Also, this issue seems to prevent using Voila as a server extension at the moment: jupyter-server/jupyter_server#74. |
self.serverapp.log.debug("using template: %s", self.template) | ||
self.serverapp.log.debug("nbconvert template paths:\n\t%s", "\n\t".join(self.nbconvert_template_paths)) | ||
self.serverapp.log.debug("template paths:\n\t%s", "\n\t".join(self.template_paths)) | ||
self.serverapp.log.debug("static paths:\n\t%s", "\n\t".join(self.static_paths)) | ||
|
||
jenv_opt = {"autoescape": True} # we might want extra options via cmd line like notebook server | ||
env = jinja2.Environment(loader=jinja2.FileSystemLoader(self.template_paths), extensions=['jinja2.ext.i18n'], **jenv_opt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could make use of ExtensionAppJinjaMixin
here: https://github.com/jupyter/jupyter_server/blob/5f99e53f7206c1272494a977e3246aa870d74fdf/jupyter_server/extension/application.py#L89
I'm trying to sync this PR with master, I opened a PR in @maartenbreddels's |
Closing as the proposed changes are not applicable anymore to the next release of Voila |
A 'reboot' of #270 with a single clean commit to avoid merge conflicts with other PR's.
@Zsailer I took out many code reformatting changes, since they confused the diff-er, and me, and with large outstanding PR's it will lead to many more merge conflicts and a polluted history. I hope you don't hold it against me :)
Replaces #270