Skip to content

Move kernel code into IPython.kernel #2854

Merged
merged 19 commits into from Jan 31, 2013

3 participants

@minrk
IPython member
minrk commented Jan 26, 2013

inprocess Kernel in kernel.inprocess
zmq Kernel in kernel.zmq
KernelManager stuff in top-level kernel

Main functional change: allow custom kernel Popen command

reissue of #2853, because it didn't seem to want to track the right commits

  • adds KernelManager.kernel_cmd configurable for launching a custom kernel
  • splits entry_point.base_launch_kernel into two steps: making the launch cmd and launching the subprocess
  • figure out where the entry_point functions belong, if it should be anywhere else
  • move IPython.zmq.kernelmanagerabc to IPython.kernel.kernelmanagerabc
  • move IPython.lib.kernel/IPython/zmq.entry_point to IPython.kernel.launcher / connect
  • move zmq.ipkernelapp.IPKernelApp to zmq.kernelapp (I'll look at merging the classes, and see if it makes
  • move IPython.zmq to IPython.kernel.zmq
  • move IPython.inprocess to IPython.kernel.inprocess
  • move embed_kernel from zmq.ipkernelapp to zmq.embed
  • move MultiKernelManager to IPython.kernel.multikernelmanager.
  • move IPython.zmq.blockingkernelmanager to IPython.kernel.blockingkernelmanager.
  • move IPython.zmq.kernelmanager to IPython.kernel.kernelmanager.
  • move IPython.ipkernel.Kernel to IPython.kernel.kernel.

If you use my iruby branch (every other one I found had failures in the msg spec / session), you can run ruby kernels with qtconsole/console/notebook with:

ipython console --KernelManager.kernel_cmd='["ruby", "lib/kernel.rb", "{connection_file}"]' --Session.key=''

supersedes #2643

@Carreau
IPython member
Carreau commented Jan 26, 2013

Will try to get a look but need to go now.
Would you like to put the ruby kernel in its own repo somewhere on IPython Organisation ?

@Carreau
IPython member
Carreau commented Jan 26, 2013

FYI travis build is done and passes.
we can still put the function of entry point elsewhere later.

Maybe just to be annoying, make_kernel_cmd does not reflect the fact that it's only for python kernel.

@minrk
IPython member
minrk commented Jan 28, 2013

@Carreau I've renamed it make_ipkernel_cmd.

I decide to consolidate IPython.lib.kernel and IPython.zmq.entry_point into a single IPython.utils.kernel (IPython.lib.kernel imports from here with a deprecation warning, to avoid breaking apps like vim-ipython).

@minrk minrk referenced this pull request in ivanov/vim-ipython Jan 28, 2013
Merged

find_connection_file is moving #51

@ellisonbg
IPython member
@minrk
IPython member
minrk commented Jan 29, 2013

it's already done here, go ahead and have a look. It's basically just copy/paste the two files into the new location and updated imports.

@ellisonbg
IPython member

We are doing enough reorganization in this PR, some things I am wondering about...

  • Creating an IPython.kernel subpackage. This would be for the parts of the kernel at are zmq independent. This would be for the base kernel implementation, kernelmanager, kernelmanagerabc, and your kernel utilities (maybe as IPython.kernel.launcher or utils?)
  • Consolidate the zmq kernel apps into a single module, possibly combining into one class, but minimally putting them in one file.

I think this would provide a clean set of abstractions that better reflect how the code is organized. I know this expands the work for this PR a bit more, but you are much of the way there...

Cheers,

Brian

@minrk
IPython member
minrk commented Jan 29, 2013

Sure, so:

(checklist added to top)

  • move IPython.zmq.kernelmanagerabc to IPython.kernel.kernelmanagerabc
  • move IPython.utils.kernel to IPython.kernel.launcher / connect
  • move IPython.zmq.kernelmanager to IPython.kernel.kernelmanager.
  • move IPython.ipkernel.Kernel to IPython.kernel.kernel.
  • move zmq.ipkernelapp.IPKernelApp to zmq.kernelapp (I'll look at merging the classes, and see if it makes sense).

anything more than that?

@ellisonbg
IPython member

If most of the things in the kernel utils module are related to launching kernels, then I think that launcher is also a good module name.

@ellisonbg
IPython member

I have also changed a few things in the todo list...

@minrk
IPython member
minrk commented Jan 29, 2013

If most of the things in the kernel utils module are related to launching kernels, then I think that launcher is also a good module name.

It is not, but I can split it into two or three files. A minority is directly for launching kernels, the rest is for dealing with connection files (locating them, writing them, etc.), and connecting to kernels (ssh tunnels, etc.). In any case, I think the official API for these should be IPython.kernel, so the specific filename or number of files is not so important.

@minrk
IPython member
minrk commented Jan 29, 2013

I disagree with moving zmq.kernel and zmq.kernelmanager into IPython.kernel. These are 100% zmq-specific. I think only if/when we build zmq-less base classes should they go into IPython.kernel.

@ellisonbg
IPython member
@minrk
IPython member
minrk commented Jan 29, 2013

It sounds like your argument is backwards. All the stuff in IPython.zmq is specific to our kernel, and you are suggesting that we create a new module called "IPython.kernel" that has everything but code directly involved in the kernel.

Although there are a lot of other places where zmq is used (parallel, qtconsole, etc.). Does this make sense.

If we move kernelmanager to IPython.kernel, zmq.session will be the only import from IPython.zmq in either the notebook or qtconsole (aside from a couple of classes used to generate default config files).

@minrk
IPython member
minrk commented Jan 29, 2013

I should say - your argument does make sense, in that IPython.zmq contains 'our zmq-based IPython kernel', and these other things like KernelManager might belong somewhere else. I just think 'IPython.kernel' isn't the right name for it, since there is something called the "IPython kernel" and IPython.kernel is strictly code that is not that kernel.

There is one point though - if the zmq KernelManager belongs in this new package, but the zmq Kernel lives in IPython.zmq, that would suggest that the inprocess KernelManager and Kernel do not themselves belong together in the same subpackage, right?

To my mind, we have subpackages containing an implementation of a Kernel (currently zmq and inprocess), and their unique peers (associated KernelManagers) are also defined adjacently. Things that belong in IPython.kernel should be strictly reusable, which the zmq KernelManager certainly is not - it can only talk to kernels that talk the wire protocol (distinct from the message protocol in general) as implemented in zmq.Session. In this way, IPython.kernel makes sense as the generic kernel-related code, IPython.zmq is zmq-specific kernel-related code, etc. The KernelManager is 100% zmq-specific. If we do build a generic zmq-free base class, then that would belong in IPython.kernel under this scheme.

minrk added some commits Jan 28, 2013
@minrk minrk test IPython.kernel 896b92c
@minrk minrk move IPKernelApp from zmq.ipkernel to zmq.kernelapp
- merged IPKernelApp into KernelApp, they are no longer separate classes
- embed_kernel moved to its own file
- ipkernel now only contains the Kernel class
- associated imports updated
891a79b
@minrk
IPython member
minrk commented Jan 29, 2013

I'v re-read, and I just realized that you want both Kernel and KernelManager in IPython.kernel. I think that actually makes sense. If we do end up getting the zmq-dependent functionality out of the base Kernel and KernelManager classes, then we will add the zmq-specific subclasses back to IPython.zmq, and IPython.kernel can be zmq-free.

@minrk
IPython member
minrk commented Jan 29, 2013

quick question: embed_kernel was in zmq.ipkernel, but I gave it its own file. Should that file now be in IPython.kernel or IPython.zmq? I'm leaning towards IPython.kernel.

@minrk
IPython member
minrk commented Jan 29, 2013

I also presume blockingkm should come along too? I don't see a reason why blockingkm would not live adjacent to km.

@ellisonbg
IPython member
@minrk minrk never use Queue.get(timeout=None)
Queue.get(timeout=None) has stupid uninterruptible behavior,
so wait for a week instead.
d72b217
@minrk
IPython member
minrk commented Jan 29, 2013

Feel better! I'm still 50/50 on moving Kernel and KernelManger from zmq to kernel, so I will wait until we chat.

Here's the gist, as I see it:

  • it makes sense to have the base Kernel and KernelManager in IPython.kernel, and currently the ZMQ versions are our bases however
  • KernelManager, as it is, is tightly bound to the zmq wire protocol as implemented in zmq.Session
  • the same is true of Kernel
  • BlockingKernelManager should be adjacent to KernelManager
  • embed_kernel and IPKernelApp should be adjacent to Kernel
  • it does make some sense for KernelManager to not be adjacent to Kernel, but IPython.kernel is not an appropriate name for the non-kernel part of that equation.
@minrk
IPython member
minrk commented Jan 30, 2013

Okay, @ellisonbg, I think this is ready for a look.

@ellisonbg ellisonbg and 1 other commented on an outdated diff Jan 30, 2013
IPython/kernel/kernelmanager.py
@@ -677,7 +683,20 @@ def _session_default(self):
return Session(config=self.config)
# The kernel process with which the KernelManager is communicating.
- kernel = Instance(Popen)
+ # generally a Popen instance
+ kernel = Any()
+
+ kernel_cmd = List(Unicode, config=True,
+ help="""The Popen Command to launch the kernel.
+ Override this if you have a custom
+ """
+ )
+ def _kernel_cmd_changed(self, name, old, new):
+ print 'kernel cmd changed', new
@ellisonbg
IPython member
ellisonbg added a note Jan 30, 2013

Is this a debug message?

@minrk
IPython member
minrk added a note Jan 31, 2013

it certainly is. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg
IPython member

OK I have looked through the code, run the test suite and run all of the main ipython applications. Everything works perfectly. I really like how this turned out and think it is such a huge improvement. The code is much more transparent and logically organized. There may be bug that this PR is introducing, but I think the only way we will find them is by using it. Because of this, I think we should merge this unless you want to get some other eyes on this.

@ellisonbg
IPython member

I think this is ready to go - I would go ahead with the merge.

@minrk minrk merged commit 5db1672 into ipython:master Jan 31, 2013
@minrk minrk added a commit to minrk/ipython that referenced this pull request Feb 1, 2013
@minrk minrk fix payload keys
A few changes left out from PR #2854

prevented pager or set_next_input (%load) from working in the notebook.
d72667c
This was referenced Feb 1, 2013
@minrk minrk deleted the minrk:2853b branch Mar 31, 2014
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@minrk minrk fix payload keys
A few changes left out from PR #2854

prevented pager or set_next_input (%load) from working in the notebook.
4f0662e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.