Open Zsailer opened 2 months ago
@fcollonval @davidbrochart
cc @vidartf and @afshin, since we discussed this a bit on the server call today.
I've added documentation and made it possible to write one of these hooks for "classic" server extensions, i.e. extensions that don't inherit from the ExtensionApp
.
The function must be named _start_jupyter_server_extension
and should be found in the same location as the _load_jupyter_server_extension
.
Downstream test failures appear to be unrelated.
Hey @fcollonval 👋
Thanks for looking at this. First, let me acknowledge the obvious... naming is hard.
- why not using
start_extension
to get an easier to understand APIstart/stop_extension
?
I originally used this naming pattern, but in the last Jupyter Server meeting, folks asked if we could make this API available for "simple" server extension, i.e. one's that don't inherit from ExtensionApp. The pattern set for classic extension is to use _<verb>_jupyter_server_extension
.
I'm fine with have start_extension
in the Extension App, though, and just pointing a _start_jupyter_server_extension
method at it.
- if not renaming to
start_extension
, we should not prefix the class method with_
(akaExtensionApp._start_jupyter_server_extension
) as the convention implies that those have visibility protected and that does not fit the need for them to be public (to be accessed by the extension manager).
Hm, I understand what you mean, but this is a slightly odd/edge case. The _start_jupyter_server_extension
function lives in an extension, and the extension has two types of consumers that represent the "public" in this case: 1) the extension manager (as you mentioned) and 2) people extending the extension. It's critically important that we make this protected from group (2), i.e. people who are likely to break things if they override this method.
The extension manager is a special case. Yes, technically it's breaking the "public visibility within a class" convention, but it's a special type of "public" here. It must change if the _start_jupyter_server_extension
signature changes; this method and the extension manager must evolve together and thus, their relationship needs to be protected.
We followed this same logic when renaming the _load_jupyter_server_extension
and _link_jupyter_server_extension
APIs.
- I understand why you would add support for the classic extension. But in that case, I feel that we should also allow the
stop_extension
hook as it will surely be needed to clean started resources.
Sure. I don't think it's strictly necessary for this PR to be reviewed/merged. I'm happy to add it here, but there are many other cases where you need an async start action without leaving lingering resources around that need cleanup.
This is enabled in the current API, and is used by jupyter-lsp, using the function-based mechanism:
def load_jupyter_server_extension(nbapp):
if hasattr(nbapp, "io_loop"):
io_loop = nbapp.io_loop
else:
# handle jupyter_server 1.x
io_loop = ioloop.IOLoop.current()
io_loop.call_later(0, initialize, nbapp, virtual_documents_uri)
async def initialize(nbapp, virtual_documents_uri):
await #... yadda yadda
The shortcoming is this spews a lot of output (especially with --debug
) and obscures the URL.
We added this because people complained about how long a server took to start up in a JupyterHub setting.
Blocking the URL print would feel perceptually worse, but in the end would feel better.
Thanks @Zsailer for your detailed answer.
The arguments with the naming make sense. Indeed naming is always a high source of debates 😅
Regarding adding stop_extension
in classic extension, don't hesitate to open an issue to have that point tackled in a follow-up PR.
Fixes #1203 and #1329