Skip to content

Add cull_busy, cull_connected options #2498

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

Merged
minrk merged 4 commits into from
May 27, 2017
Merged

Add cull_busy, cull_connected options #2498

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1550a04
Add cull_busy, cull_connected options
parente May 17, 2017
d9b4c99
Do not cull kernels doing work by default
parente May 18, 2017
85ba2bd
Fix help for cull_connected and cull_busy
parente May 22, 2017
c8ba55d
Do not cull connected kernels by default
parente May 26, 2017
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: 24 additions & 3 deletions notebook/services/kernels/kernelmanager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

from jupyter_client.session import Session
from jupyter_client.multikernelmanager import MultiKernelManager
from traitlets import Dict, List, Unicode, TraitError, Integer, default, validate
from traitlets import Bool, Dict, List, Unicode, TraitError, Integer, default, validate

from notebook.utils import to_os_path
from notebook._tz import utcnow, isoformat
Expand Down Expand Up @@ -71,6 +71,16 @@ def _update_root_dir(self, proposal):
help="""The interval (in seconds) on which to check for idle kernels exceeding the cull timeout value."""
)

cull_connected = Bool(True, config=True,
Copy link
Member

@minrk minrk May 25, 2017

Choose a reason for hiding this comment

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

Maybe default to the most conservative False for both of these?

Copy link
Member

@kevin-bates kevin-bates May 25, 2017

Choose a reason for hiding this comment

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

@minrk I understand taking a conservative approach but I believe that a majority of administrators that enable culling in the first place (the feature is off by default) would want connected kernels culled - provided they've exceeded the idle duration and are not busy for that period. That said, I suppose administrators will review the four culling parameters when enabling, but still feel cull_connected = True is the appropriate default - IMHO.

help="""Whether to consider culling kernels which have one or more connections.
Only effective if cull_interval is not 0."""
Copy link
Member

@takluyver takluyver May 22, 2017

Choose a reason for hiding this comment

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

Do you mean cull_idle_timeout in both of these messages? Looking at the code, setting cull_interval to 0 produces a warning and resets it to 300.

Copy link
Member

@kevin-bates kevin-bates May 22, 2017

Choose a reason for hiding this comment

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

Good catch - yes, the descriptions of each should reference cull_idle_timeout.

)

cull_busy = Bool(False, config=True,
help="""Whether to consider culling kernels which are busy.
Only effective if cull_interval is not 0."""
)

#-------------------------------------------------------------------------
# Methods for managing kernels and sessions
#-------------------------------------------------------------------------
Expand Down Expand Up @@ -273,6 +283,10 @@ def initialize_culler(self):
self.cull_kernels, 1000*self.cull_interval, loop)
self.log.info("Culling kernels with idle durations > %s seconds at %s second intervals ...",
self.cull_idle_timeout, self.cull_interval)
if self.cull_busy:
self.log.info("Culling kernels even if busy")
if self.cull_connected:
self.log.info("Culling kernels even with connected clients")
self._culler_callback.start()

self._initialized_culler = True
Expand All @@ -294,8 +308,15 @@ def cull_kernel_if_idle(self, kernel_id):
if kernel.last_activity is not None:
dt_now = utcnow()
dt_idle = dt_now - kernel.last_activity
if dt_idle > timedelta(seconds=self.cull_idle_timeout): # exceeds timeout, can be culled
# Compute idle properties
is_idle_time = dt_idle > timedelta(seconds=self.cull_idle_timeout)
is_idle_execute = self.cull_busy or (kernel.execution_state != 'busy')
connections = self._kernel_connections.get(kernel_id, 0)
is_idle_connected = self.cull_connected or not connections
# Cull the kernel if all three criteria are met
if (is_idle_time and is_idle_execute and is_idle_connected):
idle_duration = int(dt_idle.total_seconds())
self.log.warning("Culling kernel '%s' (%s) due to %s seconds of inactivity.", kernel.kernel_name, kernel_id, idle_duration)
self.log.warning("Culling '%s' kernel '%s' (%s) with %d connections due to %s seconds of inactivity.",
kernel.execution_state, kernel.kernel_name, kernel_id, connections, idle_duration)
self.shutdown_kernel(kernel_id)