misc notebook: connection file cleanup, first heartbeat, startup flush #1187

Merged
merged 6 commits into from Jan 6, 2012

Projects

None yet

2 participants

@minrk
Member
minrk commented Dec 20, 2011

Kernels would not linger, but the KernelManagers are not garbage-collected on shutdown. This means that connection files for kernels still running at notebook shutdown would not be removed.

Now, kernels are explicitly killed at server shutdown, allowing the KernelManagers to cleanup files.

Small changes along the way:

  • disables the unnecessary (and actively detrimental) SIGINT handler inherited from the original copy/paste from the qt app.
  • put webapp initialization in init_webapp out of initialize, to preserve convention of there being no unique code in initialize().
  • don't warn about http on all interfaces if running in 100% read-only mode, because no login or execution is possible.

(Yay, my first PR for 0.13!)

minrk added some commits Dec 20, 2011
@minrk minrk cleanup connection files on notebook shutdown
Kernels would not linger, but the KernelManagers are not garbage-collected on shutdown.
This means that connection files for kernels still running at notebook shutdown would not be removed.

Also disable the unnecessary (and actively unhelpful) SIGINT handler inherited from the original
copy/paste from the qt app.
1e91756
@minrk minrk flush stdout/err after init_code
prevents startup script/file output from being attached to the first cell of a frontend
8d7b393
@minrk
Member
minrk commented Dec 21, 2011

includes stdout/err flush fix mentioned in #1191

@minrk minrk add first_beat delay to notebook heartbeats
Heartbeats start immediately, causing false heart failures on slow systems that can take a while to start kernel subprocesses.

Also adds a 'flush' to the heartbeat callback (just like in IPython.parallel), to protect against server load being detected as heart failures.
45a89f9
@minrk
Member
minrk commented Dec 23, 2011

include heartbeat delay mentioned in #1198.

I still don't like the way heartbeats are done in either the notebook or the base KernelManager, and the base KernelManager also really must be configurable. But that's for another time.

@fperez
Member
fperez commented Jan 5, 2012

Mmh, I'm gettting these messages in the starting consoles:

ERROR:root:Error in periodic callback
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/zmq/eventloop/ioloop.py", line 432, in _run
    self.callback()
  File "/home/fperez/usr/lib/python2.7/site-packages/IPython/frontend/html/notebook/handlers.py", line 451, in ping_or_dead
    self.hb_stream.flush()
  File "/usr/lib/python2.6/dist-packages/zmq/eventloop/zmqstream.py", line 282, in flush
    self._check_closed()
  File "/usr/lib/python2.6/dist-packages/zmq/eventloop/zmqstream.py", line 458, in _check_closed
    raise IOError("Stream is closed")
IOError: Stream is closed
@minrk
Member
minrk commented Jan 5, 2012

hm, any earlier info?

That's always a side effect of a previous error.

I'll look into it. PyZMQ version?

@fperez fperez referenced this pull request Jan 6, 2012
Closed

Dead kernel loop #1232

@fperez
Member
fperez commented Jan 6, 2012

No, prior to that it's all normal output. It seems to only happen on FF, but it doesn't happen always. I can't seem to get what triggers it. I'm using pyzmq 2.1.9, the system one from ubuntu 11.10 (64bit).

@minrk
Member
minrk commented Jan 6, 2012

Ah, I know what this is. Try opening a new notebook, then closing the tab within 5 seconds. You should see it again. I'll push a fix shortly.

@fperez fperez and 1 other commented on an outdated diff Jan 6, 2012
IPython/frontend/html/notebook/notebookapp.py
+
+ @catch_config_error
+ def initialize(self, argv=None):
+ super(NotebookApp, self).initialize(argv)
+ self.init_configurables()
+ self.init_webapp()
+
+ def cleanup_kernels(self):
+ """shutdown all kernels
+
+ The kernels will shutdown themselves when this process no longer exists,
+ but explicit shutdown allows the KernelManagers to cleanup the connection files.
+ """
+ self.log.info('Shutting down kernels')
+ km = self.kernel_manager
+ while km.kernel_ids:
@fperez
fperez Jan 6, 2012 Member

I'm wondering if there should be a safety exit here. While loops whose exit condition might not be triggered are dangerous. If for some reason the kernel_ids list doesn't fully empty out, this guy will hang forever.

Perhaps this logic would be cleaner as

for i in range(3): # try no more than 3 times
  for k in km.kernel_ids:
    km.kill_kernel(k)
  if not km.kernel_ids:
    break
else:
  self.log.warn('Unkillable kernels...')

What do you think?

@minrk
minrk Jan 6, 2012 Member

km.kill_kernel is the explicit method for deleting kernel ids, and cannot fail to do so without actually raising and exiting the loop. Your proposal as it is will not work because kill_kernel changes km.kernel_ids, so a copy would have to be made. But if I do make the copy, I don't think there's any need to try multiple times:

for k in list(km.kernel_ids):
    km.kill_kernel(k)

should do just fine.

@fperez
fperez Jan 6, 2012 Member

Ah, thanks for the clarification. Then yes, I think that's a cleaner-looking code; if nothing else it's obvious that it can't get stuck infinitely in a while loop even for someone who doesn't know how the kill_kernel function behaves internally.

Other than this, I think it's good to go. I checked the behavior and read the rest of the code and see no other issues. Thanks!

@minrk
minrk Jan 6, 2012 Member

Makes sense. Change above has been pushed.

@minrk minrk explicit for-loop in cleanup_kernels
makes single-iteration clearer than while loop, as reviewed by @fperez.
0f2a7ee
@fperez
fperez commented on 0f2a7ee Jan 6, 2012

I'm glad the function is called kill_kernel and not kill, otherwise that last line would read pretty darkly ;) Thanks!

Owner
minrk replied Jan 6, 2012

Indeed it would. I hadn't seen that.

@fperez fperez merged commit e73fe99 into ipython:master Jan 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment