-
Notifications
You must be signed in to change notification settings - Fork 312
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
Standalone mode set by extension authors #76
Changes from 6 commits
ab003db
fd621c4
5bc87f1
e416c74
d7b70e7
89c184c
9e4448e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1415,6 +1415,10 @@ def init_server_extensions(self): | |
func = getattr(mod, 'load_jupyter_server_extension', None) | ||
if func is not None: | ||
func(self) | ||
# Add debug log for loaded extensions. | ||
self.log.debug("%s is enabled and loaded." % modulename) | ||
else: | ||
self.log.warning("%s is enabled but no `load_jupyter_server_extension` function was found") | ||
except Exception: | ||
if self.reraise_server_extension_failures: | ||
raise | ||
|
@@ -1459,7 +1463,7 @@ def init_shutdown_no_activity(self): | |
pc.start() | ||
|
||
@catch_config_error | ||
def initialize(self, argv=None): | ||
def initialize(self, argv=None, load_extensions=True): | ||
super(ServerApp, self).initialize(argv) | ||
self.init_logging() | ||
if self._dispatching: | ||
|
@@ -1469,7 +1473,8 @@ def initialize(self, argv=None): | |
self.init_webapp() | ||
self.init_terminals() | ||
self.init_signal() | ||
self.init_server_extensions() | ||
if load_extensions: | ||
self.init_server_extensions() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be a good idea to log an informational message when Likewise, I think a debug message in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added some logging messages. |
||
self.init_mime_overrides() | ||
self.init_shutdown_no_activity() | ||
|
||
|
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.
@kevin-bates Here's the other logging message.
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 Zach. I'm wondering if the phrase "is running without loading other extensions." could get construed as a problematic symptom. That is, admins may not necessarily know that a given "primary" extension is configured for exclusive use and might wonder - why is it not loading other extensions? Just thinking that a phrase like the following might be more clear:
"{ext_name} is configured for exclusive use - no other extensions will be loaded."
This way, the intention is clear and not as ambiguous.
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.
@Zsailer - I just noticed that the polarity on the condition is not correct. This should only occur if
load_other_extensions
is False.Also, I'm thinking it might be better to move this log statement into
ServerApp.initialize()
as anelse
condition to where the other extensions are being loaded - or is that at too low a level?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.
Oh my. Good catch and sorry for the sloppiness! 🤦♂
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.
My hesitation here is that we'd need to add another argument,
extension_name
, toServerApp.initialize()
. Right now, the ServerApp is completely unaware of ExtensionApps. Adding this argument would break this pattern. Do we think that's ok?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.
Ah - that makes sense - good point. Let's keep that pattern in tact as long as possible.
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 the word "configured" might be confusing since it's not configurable by the user.
How about: