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.

Sign up for GitHub

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

Jump to bottom

surface config.d nbserver_extensions to the NotebookApp config object #4376

Merged
merged 1 commit into from
Feb 25, 2019
Merged

surface config.d nbserver_extensions to the NotebookApp config object #4376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions notebook/notebookapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1539,15 +1539,13 @@ def init_components(self):
# TODO: this should still check, but now we use bower, not git submodule
pass

def init_server_extensions(self):
"""Load any extensions specified by config.
def init_server_extension_config(self):
"""Consolidate server extensions specified by all configs.
Copy link
Member

Choose a reason for hiding this comment

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

This wording is much clearer to me. 👍


Import the module, then call the load_jupyter_server_extension function,
if one exists.
The resulting list is stored on self.nbserver_extensions and updates config object.

The extension API is experimental, and may change in future releases.
"""

# TODO: Remove me in notebook 5.0
for modulename in self.server_extensions:
# Don't override disable state of the extension if it already exist
Expand All @@ -1566,15 +1564,23 @@ def init_server_extensions(self):
manager = ConfigManager(read_config_path=config_path)
section = manager.get(self.config_file_name)
extensions = section.get('NotebookApp', {}).get('nbserver_extensions', {})

for modulename, enabled in sorted(extensions.items()):
if modulename not in self.nbserver_extensions:
Copy link
Member

Choose a reason for hiding this comment

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

I think this inverts priority when config is defined at both conf.d and top-level. What we had before, was:

if in nbserver_extensions and not in extensions, load it (conf.d extensions has higher priority)

What we have here is:

if in extensions and not in self.nbserver_extensions (conf.d extensions has lower priority)

This resolves differently when a given config value is defined at both levels.

If you'd like to add a test that verifies this (would be super helpful!), try loading config where jupyter_notebook_conifg.py enables an extension and jupyter_notebook_config.d/test.json disables it. The result should be a disabled extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

when you say priority do you mean literal priority in terms of the Dict itself or in terms of overriding?

I had thought that user settings should override the defaults.

If self.nbserver_extensions already has this modulename then we defer to the user's configuration loaded from the app. Otherwise we adhere to what we find in the jupyter_notebook_config.d/ directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you'd like to add a test that verifies this (would be super helpful!), try loading config where jupyter_notebook_conifg.py enables an extension and jupyter_notebook_config.d/test.json disables it.

That seems backwards to me, that means that as long as that library is installed I can never override its values.

Specifically in the case I'm imagining

jupyter_notebook_conifg.py disables an extension and jupyter_notebook_config.d/test.json enables the extension… If we always enable the extension there is no way for a user to override that library's default to enabling when it is installed.

Copy link
Member

Choose a reason for hiding this comment

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

Where I think this gets murky is if a user's config ends up making their notebook servers not launchable by orchestrators like JupyterHub. As an operator I like to know that a deployment will reliably come up and if there are issues they get surfaced so that we can resolve them. In the current state of affairs like the inconsistency that's being addressed in this PR, the lack of config either doesn't surface (a jupyter server extension doesn't hard stop the server if improperly configured) or ends up buried away in the notebook server logs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where I think this gets murky is if a user's config ends up making their notebook servers not launchable by orchestrators like JupyterHub.

That feels like a different case in my mind than what is happening in the jupyter_notebook_config.d which is often not defined by deployers but by library maintainers as part of their data_files to their setuptools.setup call.

I would agree that there should be a case for some way to override user configuration available to deployers. But that shouldn’t be the same convention that libraries are using to ensure their extensions are loaded.

Consider that if the person were to upgrade a library that autoenabled an extension by default (an extension that had been explicitly disabled by the deployed). The user would then overwrite the configuration placed as part of deployment in the jupyer_notebook_config.d, and that would mean a user could inadvertently change deployment config without having any clue that it was happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

However I didn’t intend for this to be a change in behaviour so I’ll switch it back to the previous logic even if I disagree with that logic’s appropriateness to the purpose of this mechanism.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for thinking through it. I think it's tricky to say whether conf.d vs top-level is 'user config' vs not. The priority was based on other conf.d-type systems (profile.d, etc.) where the top-level config is loaded first, followed by .d/*, thus giving .d higher priority. That said, in practice today, conf.d is typically written by installers, while config.{py|json} is written by user actions. It does indeed make sense for that to have highest priority in that case.

I'll go ahead and merge this one.

self.config.NotebookApp.nbserver_extensions.update({modulename: enabled})
Copy link
Member

Choose a reason for hiding this comment

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

This makes it very likely that self.config.NotebookApp.nbserver_extensions will be a LazyConfigValue, which is not meant to be interrogated directly, so you will still have trouble if you ever try to talk to the config object directly.

self.nbserver_extensions.update({modulename: enabled})
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@minrk minrk Feb 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

👍 This is the most important change, I think. self.nbserver_extensions is the thing to check, and that wasn't properly populated before this PR. It should not be assumed that self.config.NotebookApp.nbserver_extensions contains the final value of the extensions to be loaded.


for modulename, enabled in self.nbserver_extensions.items():
if modulename not in extensions:
# not present in `extensions` means it comes from Python config,
# so we need to add it.
# Otherwise, trust ConfigManager to have loaded it.
extensions[modulename] = enabled
def init_server_extensions(self):
"""Load any extensions specified by config.

for modulename, enabled in sorted(extensions.items()):
Import the module, then call the load_jupyter_server_extension function,
if one exists.

The extension API is experimental, and may change in future releases.
"""


for modulename, enabled in sorted(self.nbserver_extensions.items()):
if enabled:
try:
mod = importlib.import_module(modulename)
Expand Down Expand Up @@ -1632,6 +1638,7 @@ def initialize(self, argv=None):
if self._dispatching:
return
self.init_configurables()
self.init_server_extension_config()
self.init_components()
self.init_webapp()
self.init_terminals()
Expand Down
1 change: 1 addition & 0 deletions notebook/tests/test_serverextensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ def test_merge_config(self):
toggle_serverextension_python('mockext_both', enabled=False, user=True)

app = NotebookApp(nbserver_extensions={'mockext_py': True})
app.init_server_extension_config()
app.init_server_extensions()

assert mock_user.loaded
Expand Down