-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changes from 1 commit
030e879
a06c79b
77aa2d9
1841897
4b36b47
2c5ccb2
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 |
---|---|---|
|
@@ -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 | ||
|
@@ -132,6 +131,13 @@ | |
except ImportError: | ||
async_kernel_mgmt_available = False | ||
|
||
# Tolerate missing terminado package. | ||
try: | ||
from .terminal import TerminalManager | ||
terminals_available = True | ||
except ImportError: | ||
terminals_available = False | ||
|
||
#----------------------------------------------------------------------------- | ||
# Module globals | ||
#----------------------------------------------------------------------------- | ||
|
@@ -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, | ||
) | ||
|
||
# allow custom overrides for the tornado web app. | ||
|
@@ -660,9 +666,12 @@ class NotebookApp(JupyterApp): | |
|
||
classes = [ | ||
KernelManager, Session, MappingKernelManager, KernelSpecManager, | ||
ContentsManager, FileContentsManager, NotebookNotary, TerminalManager, | ||
ContentsManager, FileContentsManager, NotebookNotary, | ||
GatewayKernelManager, GatewayKernelSpecManager, GatewaySessionManager, GatewayClient, | ||
] | ||
if terminals_available: # Only necessary when terminals are available | ||
classes.append(TerminalManager) | ||
|
||
flags = Dict(flags) | ||
aliases = Dict(aliases) | ||
|
||
|
@@ -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 | ||
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. Modifying this still seems like it's inviting confusion, because what Can I suggest that we keep clearly separate terms for the three things we might want to know, e.g. 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. +1, although I believe 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 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. No problem - I wasn't tied to those specific names, just keen to distinguish the concepts. Now |
||
self.log.warning(_("Terminals not available (error was %s)"), e) | ||
|
||
def init_signal(self): | ||
|
@@ -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 | ||
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. 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. 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. 😄 Looks like we could do with another pass at naming, as you alluded to in another comment. I was coming to same conclusion about 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. To be clear, I think there's an actual logic bug here, regardless of which name we're using. ;-) 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. You're absolutely correct. Thank you for spotting that. 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. 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() | ||
|
@@ -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) | ||
|
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.
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 andterminals_available
for c (i.e. available to the user). In this PR,terminals_available
refers to b, andterminals_enabled
to c.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.
Agreed - see latest commit.