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

serverapp_config support added in #248 is not working for my ExtensionApp #536

Open
telamonian opened this issue Jun 3, 2021 · 5 comments

Comments

@telamonian
Copy link
Contributor

Following the conversation in #207, I attempted to refactor the jupyterfs.extension server extension into an ExtensionApp, with the goal that the refactor extension would set "contents_manager_class": "jupyterfs.metamanager.MetaManager" as part of the extension's init via the serverapp_config mechanism added in #248. 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?

@telamonian telamonian added the bug label Jun 3, 2021
@welcome
Copy link

welcome bot commented Jun 3, 2021

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@Zsailer
Copy link
Member

Zsailer commented Jun 4, 2021

Hi @telamonian, I took a closer look at ExtensionApp (I was surprised your example didn’t work too).

It turns out serverapp_config is only applied when the ExtensionApp is the starter application, i.e. you are launching the Jupyter server through a JupyterfsApp entrypoint (see this chunk). This was added for apps like JupyterLab/nbclassic/voila that needed to change default_url and open_browser when they launch the server through their entry-point.

So, Jupyter Server doesn’t currently support loading static config into ServerApp from extensions that aren’t starting apps, but there should be a simple work around.

ExtensionApp’s are “linked” before the ServerApp is fully configured, then “loaded”. This means you can inject ServerApp config during the linking step by overriding the link method in your ExtensionApp. e.g.

def _link_jupyter_server_extension(self, serverapp):
    ifcontents_manager_classnot in serverapp.config[”ServerApp“]:
        static_config = {
            “ServerApp": {
                contents_manager_class": "jupyterfs.metamanager.MetaManager"
             }
        }
        serverapp.update_config(static_config)
    super()._link_jupyter_server_extension(serverapp)

@Zsailer Zsailer added enhancement and removed bug labels Jun 4, 2021
@telamonian
Copy link
Contributor Author

It turns out serverapp_config is only applied when the ExtensionApp is the starter application, i.e. you are launching the Jupyter server through a JupyterfsApp entrypoint

@Zsailer That's what I figured, but thanks for doing that dive.

This means you can inject ServerApp config during the linking step by overriding the link method in your ExtensionApp

Whelp, that's a lovely hideous hack. I'd guess my only addition would be to hard-fail (or at least warn) on preexisting "contents_manager_class". Short-term, it does fix my problem, so I'll test your suggested hack out. Thanks!

Still, this hack obviously isn't the best way we could be doing these things. As discussed in the last juptyer_server meeting, the ideal solution would probably be to build the needed functionality into the code that parses the subconfigs from jupyter_server_config.d. I'll open a new issue about it

@Zsailer
Copy link
Member

Zsailer commented Jun 4, 2021

Yeah, I agree. This is a compelling case for enabling extensions to set config statically in config.d. There are a few things here we need to discuss in that new issue.

  1. We probably need a new (CLI) tool to display where config is coming from and how it’s layered.
  2. We need to present conflicts in a clear and interpretable way (i.e. no surprises for users).
  3. Mentioned by @echarles in kernel_manager_class configurable by a server extension? #207, we might need to consider a restricted list of traits that extensions can override?

@echarles
Copy link
Member

echarles commented Jun 5, 2021

Haven't tried it since a long time, but I think to remember that you can override via python config like described in #207 (comment)

So to make it clear, it is correct to say that it does not work via json, but does work via python.

Agree to try to remove any surprise, but the pressure should not be set too much on the sever extension system as by nature/design, the jupyter configuration system introduces those potential surprise (in other words, surprises may already exist even if you don't use the server extension system)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants