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

simple extension example #117

Merged
merged 38 commits into from
Mar 2, 2020
Merged

Conversation

echarles
Copy link
Member

This is a simple example of a server extension.

@Zsailer
Copy link
Member

Zsailer commented Oct 10, 2019

Wow! Thanks, @echarles. This is awesome!

I'll review this more closely after the meeting today.


## TO FIX

+ The token created in `browser-open.html` is `None` - http://localhost:8888/?token=None
Copy link
Member

Choose a reason for hiding this comment

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

This should be fixed in #115

@Zsailer
Copy link
Member

Zsailer commented Oct 10, 2019

Overall, this looks really good. Thanks, @echarles!

I think it would be interesting to add a simple Javascript file that lives behind the /static/server_simple endpoint.

This repo by @markellekelly is another great example. This repo includes an example of how to leverage the static_url method added in the ExtensionHandler to access the extension's static files. It even shows how to build a Typescript frontend, bundle with webpack, and load in the browser behind the static_url.

@echarles
Copy link
Member Author

@Zsailer Thx for your review. I have a few ideas to refactor a bit. Also, if ok with @markellekelly, I will inject here the work mark has done to showcase the static_url and typescript example.

@echarles
Copy link
Member Author

@Zsailer just pushed the typescript example. This is still WIP but you can always scan and give me feedback.

One question that was asked yesterday during the server meeting and one info you gave is related to a server which would have multiple extensions (eg. jlab, voila, notebook combined in one server) - I don't see how this is possible: this would mean creating a application with would combine the different handlers, but what about the static content place? We only have a single place for content...

@Zsailer
Copy link
Member

Zsailer commented Oct 11, 2019

but what about the static content place? We only have a single place for content...

Each extension should serve its static files behind /static/<extension_name.

In practice, it looks like this:

  • /static/notebook goes to Jupyter notebook's static content,
  • /static/simple_ext goes to your example extension's static content,
  • /static/voila goes to Voila's static content
  • etc.

@echarles
Copy link
Member Author

echarles commented Oct 11, 2019

I get that, but a server is launched with SimpleServer(ExtensionApp), so we can only have one extension, and one static path per tornado listening port? I don't see how to have multiple extensions per server port.

Could you write the updates to this example to get multiple extensions (maybe in init:py_ )?

@Zsailer
Copy link
Member

Zsailer commented Oct 11, 2019

Ah, yes! Remember that ExtensionApps are server extensions and use the old load_jupyter_server_extension mechanism. That means you can load multiple extensions if they are enabled.

One way to test this directly is by calling:

jupyter server --ServerApp.jpserver_extensions="{'my_ext': True, 'notebook': True}"

Or you can enable them automatically in the jupyter_server_config.d folder.

@Zsailer
Copy link
Member

Zsailer commented Oct 11, 2019

An ExtensionApp starts a ServerApp, append its own handlers, and loads other extensions if they are enabled in jupyter_server_config.d. (Note, you can block other extensions by setting ExtensionApp.load_other_extensions = False if you want a standalone extensions without other extensions).

A key point here is that the ExtensionApp itself is not a server. Rather, it creates an instance of a Jupyter Server and extends it (along with other extensions if they are enabled).

Does that make sense?

@echarles
Copy link
Member Author

Thx Zack. The example has now 2 apps and brings to me more clarity on how the extensions are working. I may bring more fine-tuning but this PR can already be reviewed.

examples/simple/README.md Outdated Show resolved Hide resolved
examples/simple/README.md Outdated Show resolved Hide resolved
Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

This is looking really good, @echarles. Left a few comments inline. Let me know when you reach the point for final review.

@echarles
Copy link
Member Author

@Zsailer Just pushed changes that implements your comments - I was also considering moving Makefile to README which is must didactic for an example case.

I can take additional comments. To me, this is ready for review/merge.

examples/simple/setup.py Outdated Show resolved Hide resolved
@echarles echarles added this to the 0.3.0 Release milestone Jan 4, 2020
@echarles
Copy link
Member Author

echarles commented Jan 9, 2020

Current issues:

  • Configuration is not taken into account jupyter simple-ext1 --SimpleApp1.configA "ConfigA from command line" does not show up in the config.
  • ExtensionAppJinjaMixin does not call both _prepare_templates method with consequence that templates are not available in the settings - PR removes the Mixin, TBD maybe in a separated PR?
  • jupyter simple-ext11 --generate-config fails with "The ExtensionApp has not ServerApp ".

Waiting a bit on @Zsailer feedback before opening separated issues for these.

@Zsailer
Copy link
Member

Zsailer commented Jan 14, 2020

Thanks, @echarles. Apologies for the delay.

  • Configuration is not taken into account jupyter simple-ext1 --SimpleApp1.configA "ConfigA from command line" does not show up in the config.

Hm, try adding = between your config name and value, i.e.:
jupyter simple-ext1 --SimpleApp1.configA="ConfigA from command line"
works for me. I think the = has always been required by traitlets CLI for trait values.

ExtensionAppJinjaMixin does not call both _prepare_templates method with consequence that
templates are not available in the settings - PR removes the Mixin, TBD maybe in a separated PR?

This looks like the issue is in the ExtensionApp itself, not the ExtensionAppJinjaMixin. The _prepare_templates method in ExtensionApp should have an initialize_templates() call at the end.

Feel free to open an issue/PR to fix this if you'd like.

jupyter simple-ext11 --generate-config fails with "The ExtensionApp has not ServerApp ".

This is a bug. The following lines need to be removed from the ExtensionApp.

https://github.com/jupyter/jupyter_server/blob/750ff3f03fe83cdff12b1a1c989d220f42e512c8/jupyter_server/extension/application.py#L215-L223

Feel free to open an issue/PR to fix this if you'd like.

@echarles
Copy link
Member Author

Thx @Zsailer Adding an = fixes the params via CLI. I will open 2 PRs (my) tomorrow (hopefully with tests) for the other 2 issues.

@echarles
Copy link
Member Author

#172 is created to fix the initialize_templates call,

@echarles
Copy link
Member Author

jupyter simple-ext11 --generate-config fails with "The ExtensionApp has not ServerApp ".

This is a bug. The following lines need to be removed from the ExtensionApp.

@Zsailer I have removed from jupyter_server/jupyter_server/extension/application.py Lines 215 to 223 in 750ff3f, but then the tests fail on test_extensionapp_load_config_file

@echarles
Copy link
Member Author

@Zsailer Would be great to merge this. It can be very useful to test simple extensions and show users how to develop. It will not hurt anything as the example folder is in is own island.

@Zsailer Zsailer merged commit 33cf8e6 into jupyter-server:master Mar 2, 2020
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
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.

2 participants