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

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Aug 15, 2019

My alternative solution to #70 and #75.

This makes standalone mode an option only set by extension authors, not a configurable trait that users can set...

@vidartf
Copy link
Member

vidartf commented Aug 15, 2019

I thought that adding load_extensions=True as a keyword arg to ServerApp.initialize was pretty elegant (as this is the only place it is currently used). It also avoids the smell of setting _ variables on another object.

-    def initialize(self, argv=None):
+    def initialize(self, argv=None, load_extensions=True):
        super(ServerApp, self).initialize(argv)
        self.init_logging()
...
        self.init_signal()
-       self.init_server_extensions()
+       if load_extensions is True:
+           self.init_server_extensions()

@Zsailer
Copy link
Member Author

Zsailer commented Aug 15, 2019

That is pretty elegant 😃

Is there any danger to changing initialize's signature? That's what made me hesitate using this solution in the original PR.

@kevin-bates
Copy link
Member

kevin-bates commented Aug 15, 2019

I guess I still prefer an empty subclass StandaloneApp ...

class StandaloneApp(ExtensionApp):
    pass

then use...

if not isinstance(self, StandaloneApp):
     self.init_server_extensions()

Then no _ variables and self is always available wherever these checks are necessary and it provides a location for future divergence/overrides. For example, StandaloneApp could simply define init_server_extensions() as a no-op (allowing the conditional to be removed).

@SylvainCorlay
Copy link
Contributor

In the case of two serverapp extensions (voila and lab)

  • when launching jupyter voila --standalone (or without standalone if voila is a standalone app), I expect jupyterlab to not be served.
  • when launching jupyter lab, I expect to be able to use voila (especially for the voila-preview jupyterlab extension).

@kevin-bates
Copy link
Member

Thanks @SylvainCorlay - that's interesting and great example.

It seems like in the latter case, voila is merely a server extension and not an application and it would be nice that that be expressed in a class hierarchy somehow. Some StandaloneApps are also extensions and some are not. All ExtensionApps are also extensions by default. Is there a way to express that?

I apologize for dragging this on. Perhaps this is a matter of clarifying the jupyter server command itself. Perhaps we should require jupyter server be the use-case for a headless server so that it not load any extension apps or it not be a valid command at all - producing output that an extensionApp is required.

We could then remove --standalone by having a StandaloneApp(ExtensionApp) class, but allow StandaloneApps to be loaded as extensions - assuming there's a way to distinguish between a "primary" extensionApp (i.e., the one specified on the CLI) vs. a "secondary" extensionApp (loaded by the primary). I suppose this raises the question - how does an author state that they don't want their StandaloneApp loaded as an extension?

Can an "app" distinguish between it being primary vs. secondary - both at load and runtime? I think that would be useful.

@SylvainCorlay
Copy link
Contributor

It seems like in the latter case, voila is merely a server extension and not an application and it would be nice that that be expressed in a class hierarchy somehow. Some StandaloneApps are also extensions and some are not. All ExtensionApps are also extensions by default. Is there a way to express that?

It does not seem to me that there is really a distinction. I was merely assuming the voila command would be an alias for jupyter voila --standalone.

@vidartf
Copy link
Member

vidartf commented Aug 15, 2019

Given that we could possibly want all 4 combinations of "autoload me/not" × "autoload others/not", I believe the server app having a flag "load other extensions/not" should be sufficient (#76). Apps that do not want to be used as extensions should then not expose the server extension "magics" in the module.

@Zsailer
Copy link
Member Author

Zsailer commented Aug 15, 2019

@vidartf Did you mean to link #70 (instead of #76)?

@vidartf
Copy link
Member

vidartf commented Aug 15, 2019

No, I was confused about where I was posting, not where I was linking 😀

Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

If we can pass flag instead of writing private var (without breaking anything), that would be better, but I think this concept is currently the best one.

@Zsailer
Copy link
Member Author

Zsailer commented Aug 15, 2019

@vidartf Updated this PR to use #75's way of allowing extensions to prevent other extensions from being loaded.

@@ -1469,7 +1469,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.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Another comment for now. (Not sure if duplicates were created - on a bouncy bus at the moment - I apologize if that was the case.)

@@ -1415,6 +1415,8 @@ 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))
Copy link
Member

Choose a reason for hiding this comment

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

These should be indented.

We should probably add an else: clause that logs a warning stating that the extension, while enabled, doesn't have a load_jupyter_server_extension() method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

👍 Warning added.

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.

@rolweber
Copy link
Contributor

rolweber commented Aug 16, 2019

I'm in favor of flags over inheritance. This allows for extensions that can act standalone as well as cooperative. There's nothing wrong with having a convenience class that sets the flag by default. But the behavior should be driven by the flag, not by the class hierarchy.

@kevin-bates
Copy link
Member

kevin-bates commented Aug 16, 2019

@rolweber - good point. The dual-mode behavior of Voila that @Sylvain stated put a damper on a class-based approach since StandaloneApps could then also be cooperative - which makes sense.

I think we've boiled this down to two primary ways to start the server, each with a couple semantic differences: jupyter <extension_app> and jupyter server and no --standalone. 🎉

Here are some concrete examples that should illustrate the differences ... (I've also used "footnotes" for questions.)

  • jupyter voila - Voila extends ExtensionApp and because it sets load_other_extensions = False, no other server extensions [1] will be loaded.
  • jupyter lab - Lab extends ExtensionApp and leaves load_other_extensions = True. As a result, any configured server extensions will be loaded - including Voila.[2,3]
  • jupyter server - Looks in the configuration files for ServerApp.jpserver_extensions and loads any enabled extensions. [4]
  • jupyter server --jpserver_extensions={...} - This is really just a form of the previous where the CLI is going to override anything found in the config files. Just stating it here for completeness. It would be cool - if possible - if use of the launch in this manner implies that all listed extensions are enabled. I guess that goes back to making the enabled boolean default to True.

[1]: By "server extension" are we (now) always talking about applications that extend ExtensionApp? That is, Voila could define its own configurable containing a list of server extensions (that do not derive from ExtensionApp and only apply to Voila) that are loaded by Voila directly - correct?

[2]: Is there a way for an ExtensionApp to say that it doesn't want to be 'cooperative'? I.e., extension authors require "complete exclusivity". Is allowing such a thing necessary?

[3]: Can a given ExtensionApp determine in what context it was loaded? Is that necessary? This probably only matters in the cases where load_other_extensions = False since those apps may want to change some behavior depending on how they're being used. This would also be a way for an author to enforce the "complete exclusivity" requirement and say "I don't want to participate" - converting its load function into a no-op in that case.

[4]: What should the behavior be if no extension apps are configured and enabled? Currently, it runs but I'm not sure how useful that is. However, I suppose this could be a way to validate a minimum configuration in times of troubleshooting, adding server extensions at each step - so I guess I'd be inclined to leave this alone.

@vidartf
Copy link
Member

vidartf commented Aug 17, 2019

Is there a way for an ExtensionApp to say that it doesn't want to be 'cooperative'? I.e., extension authors require "complete exclusivity". Is allowing such a thing necessary?

One clear step would be to not define the server extension interfaces in the package (server extension path function, server extension load function), so that it cannot be used as an extension.

Another clear step would be to name all the app classes with a leading underscore to hint that "this class is not meant for reuse".

I don't think we should do anything from this package's side.

@SylvainCorlay
Copy link
Contributor

[1]: By "server extension" are we (now) always talking about applications that extend ExtensionApp? That is, Voila could define its own configurable containing a list of server extensions (that do not derive from ExtensionApp and only apply to Voila) that are loaded by Voila directly - correct?

It was my understanding that by server extension we don't only talk about ExtensionApps. Basically we start creating server extensions as soon as we need new tornado entry points.

ExtensionApps are the subset of server extensions that has a default URL that we may want to start with a python entry point.

[2]: Is there a way for an ExtensionApp to say that it doesn't want to be 'cooperative'? I.e., extension authors require "complete exclusivity". Is allowing such a thing necessary?

Good question. I don't see a usecase for that at the moment though.

The reason why I brought up the standalone option for voilà is that it is sometimes important that we don't serve the filesystem or that we don't allow arbitrary code execution by giving a notebook interface. I expect the voila command-line to become an alias on jupyter voila --standalone, also specifying the allowed jupyter messages to disallow execution requests.

[3]: Can a given ExtensionApp determine in what context it was loaded? Is that necessary? This probably only matters in the cases where load_other_extensions = False since those apps may want to change some behavior depending on how they're being used. This would also be a way for an author to enforce the "complete exclusivity" requirement and say "I don't want to participate" - converting its load function into a no-op in that case.

It may be useful to e.g. disallow execution requests in the case of voilà, although since we should have the previously described alias, it is probably fine as-is.

[4]: What should the behavior be if no extension apps are configured and enabled?

I presume that the default start URL should be some jupyter equivalent to Apache's "it works" and some documentation about how to install extension apps.

When extension apps are enabled, they could be listed on that page.

@kevin-bates
Copy link
Member

Thank you @SylvainCorlay - that's helpful.

@Zsailer
Copy link
Member Author

Zsailer commented Aug 23, 2019

Any reason not to merge this?

Copy link
Contributor

@rolweber rolweber left a comment

Choose a reason for hiding this comment

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

lgtm

@rolweber rolweber merged commit 37afd57 into jupyter-server:master Aug 26, 2019
@Zsailer Zsailer deleted the standalone2 branch January 10, 2020 17:35
Zsailer pushed a commit to Zsailer/jupyter_server that referenced this pull request Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants