-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
surface config.d nbserver_extensions to the NotebookApp config object #4376
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turned out to be much cleaner than I expected. Well done, M. 🥇
def init_server_extensions(self): | ||
"""Load any extensions specified by config. | ||
def init_server_extension_config(self): | ||
"""Consolidate server extensions specified by all configs. |
There was a problem hiding this comment.
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. 👍
for modulename, enabled in sorted(extensions.items()): | ||
if modulename not in self.nbserver_extensions: | ||
self.config.NotebookApp.nbserver_extensions.update({modulename: enabled}) | ||
self.nbserver_extensions.update({modulename: enabled}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
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.
- separates nbserver_extension config loading into new init_server_extension_config method - adds init_server_extension_config to the initialize funciton before the init_webapp call - adds nbserver_extension configuration found in config.d files (by the BaseJSONConfigLoader) to both the underlying NotebookApp config object and the self.nbserver_extensions value - makes self.nbserver_extensions the canonical location for identifying all nbserver_extensions rather than temporary extensions variable
5d1a595
to
134c8b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to ensure priority is preserved, this change can be smaller:
- don't change the iteration over extensions at all, but
- add
self.nbserver_extensions = extensions
at the end of iteration to make sure the value is correct
I don't necessarily object to updating the config object to match as well, but what's most important to me is that it shouldn't matter that the config object is updated after doing this. Nothing should be consulting the config object after it's been loaded, only the configured instance attribute (app.nbserver_extensions
), which this PR does indeed fix. It is important to always work from the perspective that the config object is only one piece of configuring the value, and can only ever give a partial response for what the final value will be.
|
||
for modulename, enabled in sorted(extensions.items()): | ||
if modulename not in self.nbserver_extensions: | ||
self.config.NotebookApp.nbserver_extensions.update({modulename: enabled}) |
There was a problem hiding this comment.
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.
@@ -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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 andjupyter_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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
for modulename, enabled in sorted(extensions.items()): | ||
if modulename not in self.nbserver_extensions: | ||
self.config.NotebookApp.nbserver_extensions.update({modulename: enabled}) | ||
self.nbserver_extensions.update({modulename: enabled}) |
There was a problem hiding this comment.
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.
What do you think now @minrk? |
This PR comes from a discussion that arose around surfacing information about whether a server_extension is enabled from within another server_extension when the server_extension has been automatically enabled through the use of a
jupyter_notebook_config.d/
directory.Basically if you autoenable a server_extension, it's never discovered by the core JupyterApp, and so while the server_extension is enabled, this information doesn't appear in the resulting config object surfaced within
config.NotebookApp.nbserver_extensions
.This arose when trying to surface whether bookstore(a s3 publishing service) is enabled in nteract (see discussion in nteract/nteract#4144).
init_server_extension_config method
the init_webapp call
BaseJSONConfigLoader) to both the underlying NotebookApp config object
and the self.nbserver_extensions value
all nbserver_extensions rather than temporary extensions variable