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

Restore detection of missing terminado package #5465

Merged
merged 6 commits into from May 25, 2020

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented May 20, 2020

Because the change to add culling of terminals derived from terminado's terminal manager in a local class, it inadvertently broke Notebook server's functionality to tolerate a missing terminado package. These changes restore that behavior.

@kevin-bates kevin-bates changed the title Restore detection of missing terminado package [WIP] Restore detection of missing terminado package May 20, 2020
@kevin-bates
Copy link
Member Author

Support for the command line disablement still isn't right (sigh). Will remove [WIP] once resolved.

@@ -300,7 +306,7 @@ def init_settings(self, jupyter_app, kernel_manager, contents_manager,
allow_password_change=jupyter_app.allow_password_change,
server_root_dir=root_dir,
jinja2_env=env,
terminals_available=False, # Set later if terminals are available
terminals_available=terminals_available,
Copy link
Member

Choose a reason for hiding this comment

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

There are three related things, which I think might be getting a bit confused here:

a. The terminals_enabled config option, which is True by default.
b. Whether terminado can be imported
c. Whether we are exposing terminals (i.e. enabled and importable)

Currently (before this PR), we use terminals_enabled for a and terminals_available for c (i.e. available to the user). In this PR,terminals_available refers to b, and terminals_enabled to c.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - see latest commit.

@kevin-bates kevin-bates changed the title [WIP] Restore detection of missing terminado package Restore detection of missing terminado package May 20, 2020
notebook/notebookapp.py Outdated Show resolved Hide resolved
except ImportError as e:
self.terminals_enabled = False
Copy link
Member

Choose a reason for hiding this comment

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

Modifying this still seems like it's inviting confusion, because what .terminals_enabled means depends on the point at which you use it: before this, it means a config option, and after this it means whether the terminal machinery is used.

Can I suggest that we keep clearly separate terms for the three things we might want to know, e.g. terminals_available, terminals_enabled and terminals_in_use?

Copy link
Member Author

Choose a reason for hiding this comment

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

+1, although I believe terminado_available as an indication of the package's presence is more clear than terminals_available if that's okay.

I also think it's important to issue the warning message when terminals are enabled but the terminado is not present even though we know that answer prior to init_terminals. I've retained that try-catch in case there might be other initialization failures down the road.

Copy link
Member

Choose a reason for hiding this comment

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

No problem - I wasn't tied to those specific names, just keen to distinguish the concepts.

Now terminals_available on the webapp is the equivalent of terminals_in_use on the Jupyter app class. It's possibly worth aligning those, but I don't think it's a big deal either.

if term_mgr.terminals:
return # Terminals still running
if not self.terminals_in_use:
return
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look quite right. This method is to shut down the server after a certain time with no kernels/terminals. Terminals not in use means no terminals running, but this early return prevents it from ever shutting down.

Copy link
Member Author

Choose a reason for hiding this comment

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

😄 Looks like we could do with another pass at naming, as you alluded to in another comment. I was coming to same conclusion about terminals_in_use yesterday (while doing yard work). I think a switch (alignment) to terminals_available might just be the best name:
a. terminado_available = Terminado package is installed
b. terminals_enabled = User has condoned the use of terminals (independent of terminado's presence)
c. terminals_available = User has condoned the use of terminals and terminado 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.

To be clear, I think there's an actual logic bug here, regardless of which name we're using. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely correct. Thank you for spotting that.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I think this is in good shape now. :-)

@takluyver
Copy link
Member

Other than my latest comment about shutting down on a timeout, I think this looks good now.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 added this to the 6.1 milestone May 25, 2020
@blink1073 blink1073 merged commit 4da22ce into jupyter:master May 25, 2020
@kevin-bates kevin-bates deleted the detect-missing-terminado branch May 26, 2020 21:50
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants