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

Standalone mode set by extension authors #76

Merged
merged 7 commits into from
Aug 26, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
27 changes: 18 additions & 9 deletions jupyter_server/extension/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ class ExtensionApp(JupyterApp):
class method. This method can be set as a entry_point in
the extensions setup.py
"""
load_other_extensions = True

# Name of the extension
extension_name = Unicode(
"",
Expand Down Expand Up @@ -152,8 +154,8 @@ def _prepare_config(self):
the object to the webapp's settings as `<extension_name>_config`.
"""
traits = self.class_own_traits().keys()
self.config = Config({t: getattr(self, t) for t in traits})
self.settings['{}_config'.format(self.extension_name)] = self.config
self.extension_config = Config({t: getattr(self, t) for t in traits})
self.settings['{}_config'.format(self.extension_name)] = self.extension_config

def _prepare_settings(self):
# Make webapp settings accessible to initialize_settings method
Expand Down Expand Up @@ -220,14 +222,14 @@ def _prepare_templates(self):
self.initialize_templates()

@staticmethod
def initialize_server(argv=[], **kwargs):
def initialize_server(argv=[], load_other_extensions=True, **kwargs):
"""Get an instance of the Jupyter Server."""
# Get a jupyter server instance
serverapp = ServerApp(**kwargs)
# Initialize ServerApp config.
# Parses the command line looking for
# ServerApp configuration.
serverapp.initialize(argv=argv)
serverapp.initialize(argv=argv, load_extensions=load_other_extensions)
return serverapp

def initialize(self, serverapp, argv=[]):
Expand Down Expand Up @@ -279,7 +281,6 @@ def load_jupyter_server_extension(cls, serverapp, argv=[], **kwargs):
# Configure and initialize extension.
extension = cls()
extension.initialize(serverapp, argv=argv)

return extension

@classmethod
Expand All @@ -290,7 +291,6 @@ def _prepare_launch(cls, serverapp, argv=[], **kwargs):
"""
# Load the extension
extension = cls.load_jupyter_server_extension(serverapp, argv=argv, **kwargs)

# Start the browser at this extensions default_url, unless user
# configures ServerApp.default_url on command line.
try:
Expand All @@ -313,13 +313,22 @@ def launch_instance(cls, argv=None, **kwargs):
# arguments trigger actions from the extension not the server.
_preparse_command_line(cls)
# Handle arguments.
if argv is not None:
if argv is None:
args = sys.argv[1:] # slice out extension config.
else:
args = []

# Get a jupyter server instance.
serverapp = cls.initialize_server(argv=args)
serverapp = cls.initialize_server(
argv=args,
load_other_extensions=cls.load_other_extensions
)
# Log if extension is blocking other extensions from loading.
if cls.load_other_extensions:
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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 an else condition to where the other extensions are being loaded - or is that at too low a level?

Copy link
Member Author

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! 🤦‍♂

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I'm thinking it might be better to move this log statement into ServerApp.initialize() as an else condition to where the other extensions are being loaded - or is that at too low a level?

My hesitation here is that we'd need to add another argument, extension_name, to ServerApp.initialize(). Right now, the ServerApp is completely unaware of ExtensionApps. Adding this argument would break this pattern. Do we think that's ok?

Copy link
Member

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.

Copy link
Member Author

@Zsailer Zsailer Aug 16, 2019

Choose a reason for hiding this comment

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

"{ext_name} is configured for exclusive use - no other extensions will be loaded."

I think the word "configured" might be confusing since it's not configurable by the user.

How about:

"jupyter {ext_name}" runs without loading other extensions. If you would like to run {ext_name} with other extensions, try runnig "jupyter server" with {ext_name} enabled.

serverapp.log.info(
"{ext_name} is running without loading "
"other extensions.".format(ext_name=cls.extension_name)
)

extension = cls._prepare_launch(serverapp, argv=args, **kwargs)
# Start the ioloop.
extension.start_server()
Expand Down
4 changes: 4 additions & 0 deletions jupyter_server/extension/handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ def initialize(self, extension_name):
def config(self):
return self.settings["{}_config".format(self.extension_name)]

@property
def server_config(self):
return self.settings["config"]

@property
def static_url_prefix(self):
return "/static/{extension_name}/".format(
Expand Down
9 changes: 7 additions & 2 deletions jupyter_server/serverapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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()
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 it might be a good idea to log an informational message when load_extensions is False stating that "<extension_name> is running in exclusive mode" (or whatever terminology is appropriate). This way any confusion as to why other extensions aren't appearing is more clear.

Likewise, I think a debug message in init_server_extensions() stating which extension is being loaded would also be helpful information. This way admins can confirm their configurations are behaving as expected.

Copy link
Member Author

Choose a reason for hiding this comment

The 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()

Expand Down