Enable ServerApp to discover ExtensionApps (and their config).

[Zsailer]

Summary

This PR enables the ServerApp to discover ExtensionApp's before the server's Tornado Web Application is configured.

Other smaller changes:

1. Extensions' default_url changed to extension_url.
2. The order of paths for searching enabled extensions was reversed from user > env > sys-prefix to sys-prefix > env > user. This was a lingering fix in accordance with JEP #29
3. Extensions' loading function, load_jupyter_server_extension, changed to _load_jupyter_server_extension (a private function prefixed with _).
4. ServerApp now uses extensions' _jupyter_server_extension_paths function to discover server extensions. Previously, this was only used to toggle extensions.
5. Added an optional "app" key to the metadata returned by _jupyter_server_extension_paths.
6. ServerApp now initializes extensions first in it's initialize method. Then, constructs a WebApplication.

tl;dr Why?

There's a challenge with the order in which the Server and Extensions are loaded. Previously, the order is:

1. Server is initialized
2. Server config is loaded
3. Tornado WebApplication is created
4. Extensions are initialized through load_jupyter_server_extension
5. Extension config is loaded

The issue is that, during a transition to jupyter server, extension apps that try configuring serverapp do not load their config early enough (step 5). The serverapp will already have created it's WebApplication and HTTPServer objects before the extension can change their attributes. E.g. if you want to change the port of the server through a CLI like jupyter lab --LabApp.port=8890, you cannot currently do this. The port value will be loaded too late. The server will run on 8888, and the trait will change after this server starts.

This PR adds a step after step 2, where the ServerApp can discover ExtensionApp subclasses listed in jpserver_extensions. The ServerApp can then call its load_config_file() method and discover server-specific config from the ExtensionApp.

1. Server is initialized
2. Server config is loaded
3. Extension are found+stored in new attribute and ExtensionApp are initialized (thus, config loaded).
4. Tornado WebApplication is created
5. Extensions are loaded through _load_jupyter_server_extension function.

The added benefit is that it allows me to intercept the ServerApp config and shim traits using the shim layer added in Zsailer/nbclassic#7.

How

This adds an optional key:value pair to the extension metadata dict, app: <ExtensionApp subclass. If an ExtensionApp is listed in jpserver_extension, this key will be found in the extension metadata and discoverable by ServerApp. Then, the ServerApp's initialize method can pick up these extensions and load their config before creating a webapp.

My apologies for the auto-formatting diffs! 😦 If the diff is too difficult to look at, I can highlight the chunks that matter

[Zsailer]

@vidartf and @echarles, I labeled you as reviewers since you've been involved most with the discussion... hopefully you don't hate me too much 😉

[echarles]

Hi Zach, I have done a few tests with this branch.

My jlab work is completed since yesterday and works fine with master. With this branch, I face 2 issues

[1] Running jupyter lab does not return exception, but a 404 in the browser.
[2] The examples (in jlab examples/ folder) do not start anymore and return in _get_server_extension_metadata raise KeyError(u'The Python module {} does not include any valid server extensions'.format(module))

I have then used the example branch and the extensions examples work fine with this branch.

I would suggest:

[1] merge #117 so it is easier to have a debuggable baseline upon the tests.
[2] gimme a few days to debug what is going on with the jlab port.

[echarles]

@Zsailer This is the exception I receive when I run an example. Would it be related to the new method introduced here for the metadata or the _jupyter_server_extension_paths? If such, any tip to solve it?

Traceback (most recent call last):
  File "main.py", line 37, in <module>
    ExampleApp.launch_instance()
  File "/Users/echar4/datalayer/repos/jupyter-server/jupyter_server/extension/application.py", line 450, in launch_instance
    load_other_extensions=cls.load_other_extensions
  File "/Users/echar4/datalayer/repos/jupyter-server/jupyter_server/extension/application.py", line 366, in initialize_server
    serverapp.initialize(argv=argv, load_extensions=load_other_extensions)
  File "<decorator-gen-7>", line 2, in initialize
  File "/opt/datalayer/opt/miniconda3/envs/jlab-server/lib/python3.7/site-packages/traitlets/config/application.py", line 87, in catch_config_error
    return method(app, *args, **kwargs)
  File "/Users/echar4/datalayer/repos/jupyter-server/jupyter_server/serverapp.py", line 1684, in initialize
    self.init_server_extension_config()
  File "/Users/echar4/datalayer/repos/jupyter-server/jupyter_server/serverapp.py", line 1492, in init_server_extension_config
    _, metadata = _get_server_extension_metadata(modulename)
  File "/Users/echar4/datalayer/repos/jupyter-server/jupyter_server/extension/serverextension.py", line 381, in _get_server_extension_metadata
    raise KeyError(u'The Python module {} does not include any valid server extensions'.format(module))
KeyError: 'The Python module __main__ does not include any valid server extensions'

[echarles]

To reproduce, you need those 2 branches jupyterlab/jupyterlab#7416) and jupyterlab/jupyterlab_server#79.

Then go to jupyterlab/examples/app and run python main.py

[echarles]

I have added this in the jupyterlab_server handler and th example work back. Still, I receive a 404 with the full jupyter lab command even adding _jupyter_server_extension_paths on the lab handler. The good think is that jupyter lab -h works now, it was failing before.

    @staticmethod
    def _jupyter_server_extension_paths():
        return [{
            'module': 'jupyterlab_server'
        }]

[echarles]

All good now - Lab is working again . I have added a TODO to discuss later on the impact
datalayer-externals/jupyterlab@86b89ec#diff-f7b3ec31a3cdaf9ff4b443cf6ff0d38eR563

[vidartf]

I'm coming from this less updated to recent changes to the codebase, so take my questions below with a grain of salt :)

[Zsailer]

Checkout #188. There are some significant inconsistencies in our extension loading framework. I'd like to revisit them, fix it here, and rewrite our documention in this PR. Adding a [WIP] tag until I've finished.

[Zsailer]

@echarles Thanks a bunch for testing out this PR. I've been making some significant changes to the extension loading API as it has many inconsistencies. I'm hoping we smooth some of these issues.

[Zsailer]

I think this is ready for final review.

[echarles]

LGTM. Also tested with jlab extension

[vidartf]

Thanks! I had a look, and added some comments. Some of them are simply on readability, as I'm not 100% fresh on what is the current logic. Some of them might just end up as informational.

[vidartf]

I'm confused by the property names used here: serverapp is a property that maps onto settings. One implies a ServerApp instance, the other a settings value (not typically run-time objects?).

[Zsailer]

Hm, the weird thing is that we pass all kinds of run-time objects to the webapp settings (kernel manager, contents manager, session manager, etc.) I agree, this probably isn't best practice but would take an extension refactor.

[vidartf]

So wait, what is type(self.settings[key]) here? ServerApp ?

[vidartf]

Should we not keep this exception guard? Or is its implementation moved? I.e. shouldn't the actual calling of the load_jupyter_server_extension function be guarded?

[Zsailer]

Depends on what you mean. It is guarded in that, if it no load_jupyter_server_extension function is found, the serverapp doesn't crash but throws (more helpful) console log warnings. This PR improves the overall messaging around server extension failures.

[vidartf]

I meant: Previously, this code had a try: load_jupyter_server_extension(...) except Exception self.log.warning("Error loading server extension" ...). Now it has:

            try:
                # This function was prefixed with an underscore in in v1.0
                # because this shouldn't be a public API for most extensions.
                func = getattr(extension, '_load_jupyter_server_extension')
                func(self)
                self.log.debug(log_msg)
            except AttributeError:
                # For backwards compatibility, we will still look for non
                # underscored loading functions.
                func = getattr(extension, 'load_jupyter_server_extension')
                func(self)
                self.log.debug(log_msg)
            except:
                warn_msg = _(
                    "{extkey} is enabled but no "
                    "`_load_jupyter_server_extension` function "
                    "was found.".format(extkey=extkey)
                )
                self.log.warning(warn_msg)

So if the load_jupyter_server_extension throws an error (AttributeError or not), the error message will be less informative.

[vidartf]

I read this logic as "if an ExtensionApp, use new initialization path, if not, use old one". It is unclear to me if we should initialize all ExtensionApps this way, or only the one being initialized as the main app?

[Zsailer]

I think all ExtensionApps should be initialized the same way by the server... currently that's by creating and instance of the app and calling link_to_serverapp.

I can see that it's a bit weird that I'm checking extloc here too. My intention was that extloc should always be found in metadata. If it's not found, the extension should not be initialized and a warning thrown below.

[echarles]

@Zsailer With a6096ed, the default_url does not work anymore (was working before). Is this a reegression?

[Zsailer]

The failing test is unrelated to this PR. This test has been flaky since merging #201 and #191.

This PR has approved and open for a little while, so I'm going merge. Thanks all for the review!

[kevin-bates]

I fired off a restart on this, just to see if py3.7 terminal test succeeds. I suspect the timeout on that particular test may need to be increased. It's too bad there isn't a way to restart a single job with GH actions - at least that I could find.

[vidartf]

This is the line I meant should be guarded with an "Error loading server extension" warning.

[vidartf]

And this one will swallow Ctrl+C kills.