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

Add async start hook to ExtensionApp API #1417

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Apr 24, 2024

Fixes #1203 and #1329

@Zsailer
Copy link
Member Author

Zsailer commented Apr 24, 2024

@fcollonval @davidbrochart

@Zsailer
Copy link
Member Author

Zsailer commented Apr 25, 2024

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.

@Zsailer
Copy link
Member Author

Zsailer commented Apr 26, 2024

Downstream test failures appear to be unrelated.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @Zsailer

I have some questions about this:

  • why not using start_extension to get an easier to understand API start/stop_extension ?
  • if not renaming to start_extension, we should not prefix the class method with _ (aka ExtensionApp._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).
  • 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.

@Zsailer
Copy link
Member Author

Zsailer commented Apr 29, 2024

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 API start/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 _ (aka ExtensionApp._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.

@bollwyvl
Copy link
Contributor

bollwyvl commented May 2, 2024

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.

@fcollonval
Copy link
Member

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.

@Zsailer Zsailer closed this Jul 18, 2024
@Zsailer Zsailer reopened this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add async post_start method to ExtensionApp
3 participants