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.

Sign up for GitHub

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

Jump to bottom

Restore detection of missing terminado package #5465

Merged
merged 6 commits into from
May 25, 2020
Merged

Restore detection of missing terminado package #5465

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
030e879
Restore detection of missing terminado package
kevin-bates May 20, 2020
a06c79b
Properly handle terminals_enabled config
kevin-bates May 20, 2020
77aa2d9
Disambiguate terminado availability from web settings
kevin-bates May 20, 2020
1841897
Further clarity on terminal availability
kevin-bates May 21, 2020
4b36b47
Rename terminals_in_use back to terminals_available
kevin-bates May 24, 2020
2c5ccb2
Fix shutdown check
kevin-bates May 24, 2020
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
45 changes: 26 additions & 19 deletions notebook/notebookapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@
from .auth.login import LoginHandler
from .auth.logout import LogoutHandler
from .base.handlers import FileFindHandler
from .terminal import TerminalManager

from traitlets.config import Config
from traitlets.config.application import catch_config_error, boolean_flag
Expand Down Expand Up @@ -132,6 +131,13 @@
except ImportError:
async_kernel_mgmt_available = False

# Tolerate missing terminado package.
try:
from .terminal import TerminalManager
terminado_available = True
except ImportError:
terminado_available = False

#-----------------------------------------------------------------------------
# Module globals
#-----------------------------------------------------------------------------
Expand Down Expand Up @@ -172,13 +178,13 @@ class NotebookWebApplication(web.Application):
def __init__(self, jupyter_app, kernel_manager, contents_manager,
session_manager, kernel_spec_manager,
config_manager, extra_services, log,
base_url, default_url, settings_overrides, jinja_env_options):
base_url, default_url, settings_overrides, jinja_env_options, terminals_enabled):

settings = self.init_settings(
jupyter_app, kernel_manager, contents_manager,
session_manager, kernel_spec_manager, config_manager,
extra_services, log, base_url,
default_url, settings_overrides, jinja_env_options)
default_url, settings_overrides, jinja_env_options, terminals_enabled)
handlers = self.init_handlers(settings)

super(NotebookWebApplication, self).__init__(handlers, **settings)
Expand All @@ -187,7 +193,7 @@ def init_settings(self, jupyter_app, kernel_manager, contents_manager,
session_manager, kernel_spec_manager,
config_manager, extra_services,
log, base_url, default_url, settings_overrides,
jinja_env_options=None):
jinja_env_options=None, terminals_enabled=True):

_template_path = settings_overrides.get(
"template_path",
Expand Down Expand Up @@ -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=terminado_available and terminals_enabled,
kevin-bates marked this conversation as resolved.
Show resolved Hide resolved
)

# allow custom overrides for the tornado web app.
Expand Down Expand Up @@ -660,9 +666,12 @@ class NotebookApp(JupyterApp):

classes = [
KernelManager, Session, MappingKernelManager, KernelSpecManager,
ContentsManager, FileContentsManager, NotebookNotary, TerminalManager,
ContentsManager, FileContentsManager, NotebookNotary,
GatewayKernelManager, GatewayKernelSpecManager, GatewaySessionManager, GatewayClient,
]
if terminado_available: # Only necessary when terminado is available
classes.append(TerminalManager)

flags = Dict(flags)
aliases = Dict(aliases)

Expand Down Expand Up @@ -1623,7 +1632,7 @@ def init_webapp(self):
self.session_manager, self.kernel_spec_manager,
self.config_manager, self.extra_services,
self.log, self.base_url, self.default_url, self.tornado_settings,
self.jinja_environment_options,
self.jinja_environment_options, self.terminals_enabled,
)
ssl_options = self.ssl_options
if self.certfile:
Expand Down Expand Up @@ -1759,8 +1768,8 @@ def init_terminals(self):
try:
from .terminal import initialize
initialize(nb_app=self)
self.web_app.settings['terminals_available'] = True
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.

self.log.warning(_("Terminals not available (error was %s)"), e)

def init_signal(self):
Expand Down Expand Up @@ -1908,13 +1917,12 @@ def shutdown_no_activity(self):
if len(km) != 0:
return # Kernels still running

try:
term_mgr = self.web_app.settings['terminal_manager']
except KeyError:
pass # Terminals not enabled
else:
if term_mgr.terminals:
return # Terminals still running
if not self.terminals_enabled:
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. :-)


term_mgr = self.web_app.settings['terminal_manager']
if term_mgr.terminals:
return # Terminals still running

seconds_since_active = \
(utcnow() - self.web_app.last_activity()).total_seconds()
Expand Down Expand Up @@ -2000,11 +2008,10 @@ def cleanup_terminals(self):
The terminals will shutdown themselves when this process no longer exists,
but explicit shutdown allows the TerminalManager to cleanup.
"""
try:
terminal_manager = self.web_app.settings['terminal_manager']
except KeyError:
return # Terminals not enabled
if not self.terminals_enabled:
return

terminal_manager = self.web_app.settings['terminal_manager']
n_terminals = len(terminal_manager.list())
terminal_msg = trans.ngettext('Shutting down %d terminal', 'Shutting down %d terminals', n_terminals)
self.log.info(terminal_msg % n_terminals)
Expand Down
5 changes: 2 additions & 3 deletions notebook/terminal/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
import terminado
from ..utils import check_version

if not check_version(terminado.__version__, '0.8.1'):
raise ImportError("terminado >= 0.8.1 required, found %s" % terminado.__version__)
if not check_version(terminado.__version__, '0.8.3'):
raise ImportError("terminado >= 0.8.3 required, found %s" % terminado.__version__)

from ipython_genutils.py3compat import which
from tornado.log import app_log
from notebook.utils import url_path_join as ujoin
from .terminalmanager import TerminalManager
from .handlers import TerminalHandler, TermSocket
Expand Down