kernel_manager_class configurable by a server extension?

[davidbrochart]

Does it make sense to have the ServerApp.kernel_manager_class trait configurable by a server extension? Today it doesn't seem to be possible as the extensions are loaded after init_configurables(), where the kernel manager is created.

[kevin-bates]

Thanks for opening this discussion David.

This is tricky since this is a singleton. I suppose this becomes a question for how we support "core" extensions - should these be done via server extensions or the only via the subclassing/mixin approach mentioned in the service composition issue?

Because of the singularity, I'm inclined to say these should not be configurable by server extensions but am open to discussing further. That said because these are configurable anyway, why should a server extension be prevented from doing something that can be done from the command line? (Hmm, one answer might be because singularity is still preserved via command line settings, while multiple server extensions could compete with each other - last setter wins.)

[davidbrochart]

Thanks for the explanation Kevin.
We could also allow for a kernel manager class per extension, but I guess this would be a major refactoring.

[kevin-bates]

Hmm. This is the multi kernel manager so I think we have to limit to one instance. I hate that the name is so generic. We probably can't change the trait kernel_manager_class (due to our attempt to retain as much configurables as possible), but I think it would be helpful to change the attribute kernel_manager to multi_kernel_manager or mapping_kernel_manager (I prefer multi).

KernelProviders enable the ability to "bring your own kernel manager", but those will still be invoked via the single "multi" kernel manager instance. In this sense, one could view a provider as a special kind of server extension.

[Zsailer]

Today it doesn't seem to be possible as the extensions are loaded after init_configurables()

Actually, this changed in #180. Jupyter server extensions configs are gathered before init_configurables(). See this logic:

https://github.com/jupyter/jupyter_server/blob/b9b5c7ddbf94114886bc0dd396610a1aa485ef73/jupyter_server/serverapp.py#L1775-L1781

init_server_extension, which reads in extension config, occurs before init_configurables. Then, the extensions are "loaded", meaning their handlers are added, after the configurables are made. The sequence of steps goes as follows:

1. init_server_extensions()
2. init_configurables()
3. load_server_extensions()

Does it make sense to have the ServerApp.kernel_manager_class trait configurable by a server extension?

In short, I think this should already be possible. Let me know if you are experiencing something different (I might have misunderstood your question).

[davidbrochart]

Thanks Zach, that was very helpful.
I think it doesn't work in my case because my extension is an extension module, not an ExtensionApp.

[Zsailer]

Ah, I see. You're right—your extension's _load_jupyter_server_extension function will be called after the configurables are initialized.

Just an aside here, you could use the ExtensionApp for your extension since it already offers a way to configure the server's traits—and just ignore the pieces of the app that you don't need (like adding an entrypoints, loading templates + other static assets, etc.)

[Zsailer]

We could also add a "config" entry to the metadata returned by the _jupyter_server_extension_paths() function. This entry would point to a function that returns (or loads) config.

For your use-case, this would look something like:

# myextension/config.py

def _get_server_config():
    return {
        "ServerApp": {
            "kernel_manager_class" : ...,
        }
    }

# myextension/__init__.py

def _jupyter_server_extension_paths():
    """
    Returns a list of dictionaries with metadata describing
    where to find the `_load_jupyter_server_extension` function.
    """
    return [
        {
            "module": "myextension",
            "config": "myextension.config"
        }
    ]

If this seems like a reasonable feature, we would need to add logic to the server to look for this "config" key in the init_server_extension() function, which gets called before initializing the server.

[davidbrochart]

Yes it looks like a reasonable feature to have.

[kevin-bates]

Given that the trait enforces the override be a class of MappingKernelManager means that we know that implementation would be in play - so I'm ok with allowing this. (Sorry, I had gotten caught up with instance vs. class a bit previously.)

Btw, I would love to rename this trait to something more like multi_kernel_manager_class and came up with another Pro point in the arguments of the No branch in April 23 meeting minutes.

• Freedom to diverge configuration!

[Zsailer]

As @kevin-bates mentioned—this becomes a little messy if multiple extensions try to configure the same trait. I'm not sure how to reconcile that, because there is no clear order to which extensions should be loaded first vs. last. The only solution I can come up with is that extension authors should require their users to manually set the shared trait directly in their ServerApp config, rather than allow all extensions to affect any ServerApp config automatically.

[echarles]

I believe we should avoid (and even enforce) extension authors to make any change to the core serverapp. This should be IMHO something only accessible to the Administrator of the server.

[kevin-bates]

@davidbrochart - is there a particular use case for allowing an extension to bring its own MappingKernelManager (subclass) implementation? Just wondering if it makes sense to bake that into server (i.e., MappingKernelManager) in the first place.

To Eric's point, I think any Singleton is in jeopardy in the server extension discussion w/o saying "last setter wins". There was a way to say that a given extension is exclusive and will not co-exist with other extensions - is that right? (Sorry, I should know this and haven't spent enough time here.) If so, would it be possible to enforce that only those "exclusive" extensions can set traits corresponding to singletons? I suppose you could set up an @observe and then only allow the first change, denying all others.

[davidbrochart]

It is for Voila, I'd like to have an AsyncMappingKernelManager. But it's not a big deal, when we use the ExtensionApp we will be able to do it.

[kevin-bates]

Actually, we should switch jupyter server to always be async now. This will be the case when moving to providers anyway. The non async version is purely to reduce risk, but in server, we are free to live on the edge! 😄 FWIW, I've been using Notebook's AsyncMappingKernelManager in EG for a while - just waiting on the next NB release to merge it. Its really nice from the throughput perspective!

[kevin-bates]

Shoot, I went to flip the switch here and immediately ran into this block: https://github.com/jupyter/jupyter_server/blob/master/jupyter_server/serverapp.py#L1262-L1269

Since Lab still needs to support 3.5, I don't think switching at this time is the right step. Perhaps we could make the change through Voila and let this use case be a litmus test for server extension overrides?

[blink1073]

I think we can make the case that Jupyterlab 3.0 does not need to support Python 3.5. I'll bring it up at our next meeting.

[echarles]

We could also add a "config" entry to the metadata returned by the _jupyter_server_extension_paths() function. This entry would point to a function that returns (or loads) config.

I like that idea to expose to extension authors ways to interact with the core server settings. I would put on this feature two requirements:

• Have a restricted list a traits that can be overridden by the extension, the other traits should not be allowed (server should even fail if one of those disallowed is attempted to be updated by the extension.
• Configuration defined by config file (and CLI) override the config defined by the extension.

[Zsailer]

This issue popped up in jupyter-fs. This extension needs to automatically configure the Server's contents manager class when enabled.

This convinces me that we need a way to load server config from extensions.

I agree with @echarles. We should have a list of traits that extensions can override, and config+CLI supersedes the extension's config (though it should throw a warning).

We also need a way to check for conflicting config if multiple extensions touch the same trait. My initial thought is that we don't load conflicting extensions and throw warnings saying that one of the extensions must be disabled.

[echarles]

That would be a great feature to bring flexibility while ensuring stability and debugability.

To start, the limited list of trait could be limited to just two (kernel_manager and content_manager). We could add more on request.

I would be tempted to fail fast on conflicting extensions. I am OK also with a more user-friendly warning but would love to have a real visible multi-line warning to avoid the log message to be lost in the other log messages.

[Zsailer]

#248 offers a simple way to configure the underlying Jupyter Server from an ExtensionApp.

It exposes a serverapp_config class property that can be overridden by a subclass. This property is then used when creating an instance of ServerApp running underneath the extension.

To address the issue mentioned here, you'll want to write something like:

class MyApp(ExtensionApp):
    ...

    serverapp_config = {
        "kernel_manager_class" : ...,
        ...
    }

[kevin-bates]

As Zach mentions, #248 enables the ability for an ExtensionApp to adjust server config and given that AsyncMappingKernelManager is now the default via #399, I suspect this issue can be closed.

@davidbrochart - would you agree?

[davidbrochart]

I agree, thanks!

[telamonian]

@Zsailer @kevin-bates Wait wait. I just tried rewriting the jupyterfs.extension server extension as an ExtensionApp, but it seems like serverapp_config just gets ignored in my case:

import warnings

from jupyter_server.extension.application import ExtensionApp
from jupyter_server.utils import url_path_join

from ._version import __version__  # noqa: F401
from .metamanager import MetaManager, MetaManagerHandler

_mm_config_warning_msg = """Misconfiguration of MetaManager. Please add:

"ServerApp": {
  "contents_manager_class": "jupyterfs.metamanager.MetaManager"
}

to your Notebook Server config."""


def _jupyter_server_extension_paths():
    return [{
        "module": "jupyterfs.extension",
        "app": JupyterfsApp
    }]

class JupyterfsApp(ExtensionApp):
    # name = "jupyterfs.extension"
    serverapp_config = {
        "contents_manager_class": "jupyterfs.metamanager.MetaManager"
    }

    # def initialize_handlers(self):
    @classmethod
    def _load_jupyter_server_extension(cls, serverapp):
        """
        Called when the extension is loaded.

        Args:
            nb_server_app (NotebookWebApplication): handle to the Notebook webserver instance.
        """
        super()._load_jupyter_server_extension(serverapp)

        web_app = serverapp.web_app
        base_url = web_app.settings["base_url"]
        host_pattern = ".*$"

        if not isinstance(serverapp.contents_manager, MetaManager):
            warnings.warn(_mm_config_warning_msg)
            return

        resources_url = "jupyterfs/resources"
        print("Installing jupyter-fs resources handler on path %s" % url_path_join(base_url, resources_url))
        web_app.add_handlers(host_pattern, [(url_path_join(base_url, resources_url), MetaManagerHandler)])

What I'm seeing is that the isinstance check fails and the _mm_config_warning_msg warning prints, meaning that the "contents_manager_class" is never actually set. Is this a valid use of ExtensionApp and serverapp_config? Am I doing something wrong here?

[kevin-bates]

Hi @telamonian - thanks for raising this issue.

Since David's original use-case was actually resolved by the switch to using async kernel management by default (although that doesn't necessarily match the title of this issue) and your issue is relative to the general ability to introduce this functionality via #248, would you mind opening a new issue specific to your attempt to set the ContentsManager? I'd rather keep this issue closed and party on a new issue that is more of a bug issue than a feature request issue such as was originally raised here.

[telamonian]

k