Skip to content

Add autorestart logic to the kernel managers#2873

Closed
ellisonbg wants to merge 10 commits intoipython:masterfrom
ellisonbg:autorestart
Closed

Add autorestart logic to the kernel managers#2873
ellisonbg wants to merge 10 commits intoipython:masterfrom
ellisonbg:autorestart

Conversation

@ellisonbg
Copy link
Copy Markdown
Member

@ellisonbg ellisonbg commented Feb 1, 2013

Currently, when kernels die in the notebook, the user is prompted and asked if they want to restart the kernel. This is problematic from a number of standpoints:

  • If there is client, the kernel/notebook mapping gets messed up.
  • A dead kernel should always be restarted.

In this PR, I have added a new KernelRestarter object that works alongside a KernelManager object to poll the kernel's subprocess and call KernelManager.restart_kernel if it dies. I am also changing how the user is notified of this event.

Remaining:

  • Finish logic in MultiKernelManager.
  • Update MultiKernelManager tests.
  • Add new kernel message when the kernel starts/restarts.
  • Add new kernel death logic to the notebook UI.
  • Test different combinations of frontends.
  • Decide what to do about old heartbeat code.

minrk
minrk reviewed Feb 1, 2013
View reviewed changes
Comment thread IPython/kernel/kernelrestarter.py Outdated
Copy link
Copy Markdown
Member

@minrk minrk Feb 1, 2013

Choose a reason for hiding this comment

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

this probably wants to be self.log.info, not self.log

@minrk
Copy link
Copy Markdown
Member

minrk commented Feb 1, 2013

why is the autorestart only for MKM? That doesn't make sense to me.

@ellisonbg
Copy link
Copy Markdown
Member Author

ellisonbg commented Feb 1, 2013

You mean why not put it into the KernelManager itself? Honestly not sure.
Some of the reasons:

  • MKM almost always has an event loop running, whereas KM doesn't always
  • KM's don't always own their kernel, but MKMs do.

But there are reasons to include the logic in KM itself:

  • It allows different KMs to implement autorestart in different ways - and
    MKM can still manage them all.
  • It makes it an internal detail of the KM implementation that others don't
    have to worry about.

Thoughts? It is easy to move this around at this point...

On Fri, Feb 1, 2013 at 11:20 AM, Min RK notifications@github.com wrote:

why is the autorestart only for MKM? That doesn't make sense to me.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2873#issuecomment-13009443.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Copy Markdown
Member

minrk commented Feb 1, 2013

Summary of discussion on IRC:

restart logic belongs in KM, MKM should remain strictly a multiplexer. eventloop implementation of the restart is up to subclasses (e.g. Qt signal, etc.)

@minrk
Copy link
Copy Markdown
Member

minrk commented Mar 28, 2013

this is superseded by #3011

@minrk minrk closed this Mar 28, 2013
@ellisonbg ellisonbg deleted the autorestart branch January 28, 2014 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

no action

Development

Successfully merging this pull request may close these issues.

2 participants

@ellisonbg @minrk