add ability to bind to a ruby kernel #2643

Closed
wants to merge 5 commits into
from

Projects

None yet

4 participants

@Carreau
IPython member

This need the iruby kernel of my repo, (https://github.com/Carreau/iruby)
And to be modified as ruby interpreter and zmq-kernel path in IPython/zmq/rubykernel.py

Launch with (for example)
ipython notebook
--MappingKernelManager.kernel_manager_class='IPython.zmq.rubykernel.RubyKernelManager'

need zmq, uuid and hmac gem, and ruby 1.9.3

I would love to change the ruby interpreter path to ruby1.9.3 but Even with latest ruby installed, it is not in my path, so I don't know how what the ruby way is.

Also any idea of how we want to managed external kernel ? Do we put them in main IPython repo ? keep them separate ?

@Carreau Carreau add ability to bind to a ruby kernel
this (for now) need the iruby patch of my branch,
and to modify a hardcoded path in IPython/zmq/rubykernel.py

Launch with
ipython notebook
--MappingKernelManager.kernel_manager_class='IPython.zmq.rubykernel.RubyKernelManager'

need zmq, uuid and hmac gem, and ruby 1.9.3
26aac1d
@ellisonbg
IPython member

The other kernels should be separate repos/projects under the ipython org. I think we should try to make these "packages" in whatever language they are written. So for the ruby kernel, we should be able to easily create a gem and install it like any other ruby package.

@ellisonbg ellisonbg and 1 other commented on an outdated diff Dec 3, 2012
IPython/frontend/html/notebook/kernelmanager.py
@@ -28,6 +28,7 @@
from IPython.config.configurable import LoggingConfigurable
from IPython.utils.importstring import import_item
+from IPython.zmq.rubykernel import RubyKernelManager
@ellisonbg
ellisonbg Dec 3, 2012

Does this really need to be imported here?

@Carreau
Carreau Dec 3, 2012

No, you're right.

@ellisonbg ellisonbg commented on an outdated diff Dec 3, 2012
IPython/zmq/rubykernel.py
@@ -0,0 +1,16 @@
+from subprocess import Popen
+
+from IPython.zmq.kernelmanager import KernelManager
+
+def launch_ruby_kernel(fname='cf.json', **kw):
+ cmd = ['iruby_ruby', '~/iruby/lib/kernel.rb', fname]
@ellisonbg
ellisonbg Dec 3, 2012

Obviously, we will need to make these cmd lines general before we can merge this. But I think it will be helped if we can create the ruby package and use ruby to find the location of kernel.rb. I guess we need to learn more about ruby packaging and command line installation...

@Carreau
IPython member

So, we need to build a 'PopenKernelManager' I guess with a configurable command line....

This should handle pretty much every basic things that are not python...
Do we make some kind of "template" for the command line ? Like %(cfile) replaced by connexion file ?

@Carreau
IPython member

does this look better ?

I just have to add this in my config to run the ruby kernel :

[.ipython/profile_ruby/ipython_notebook_config.py]

c = get_config()

c.SubprocessKernelManager.kernel_launch_program = 
         ['/.../ruby','~/.../kernel.rb','{connexion_file_name}']
c.MappingKernelManager.kernel_manager_class=
         'IPython.zmq.subprocesskernel.SubprocessKernelManager'

and start notebook with --profile=ruby

I think it should be able to run pretty much anything...

@tkf

Is as expanduser needed?

IPython member

probably not....

@tkf

I guess people normally use _ only for variable you throw away. Just mentioning it.

IPython member

That the kind of variable I consider throwing away as it has no use outside of list comprehension. But I understand what you mean.
I've never find a good way to write those, my preference would have been to write it using map and lambda function, but you also need a useless variable to do so.

it would be great to have a way of telling 'this expression is a lambda function of implicite first ... nth variable called...'

map(expanduser(str.format( λ,foo='bar')), self.kernel_launch_program)

where f(λ,µ,...) is automatically understand as lambda λ,µ: f(λ,µ,...) with a convention for the order/name of parameters.

Variable leaks from list comprehension but this is Python 2.x's fraw, not feature. It is fixed in Python 3. So, if you write Python 3 compatible code then always you don't use the variable outside of the comprehension, meaning that it is always "throw away variable" by your criteria. I feel it is natural to consider the variable in list comprehension as used variable (if it is used).

I agree it would be nice if Python's anonymous function is shorter :)

@Carreau Carreau referenced this pull request in isotope11/iruby Dec 4, 2012
Open

iruby kernel <-> ipython frontend. #1

@Carreau Carreau referenced this pull request in JuliaLang/julia Dec 4, 2012
Closed

zeromq/IPython notebook integration. #1675

@minrk
IPython member

The existing kernel manager already starts a subprocess, and the launch command could already be customized (via passing launch). For instance, the iruby project which you reference didn't need to touch IPython at all to work. It seems this subclass just adds one configurable as an easier way to do what was already possible. Why not just add this configurable to the base KernelManager class?

@Carreau
IPython member

I didn't looked deep enough in kernelmanager and missed the configurability.

And actually the original iruby added another kernel launcher to launch the ruby kernel.

I'll try to see if I can do what you suggest.

@minrk
IPython member

And actually the original iruby added another kernel launcher to launch the ruby kernel.

Yes, I wrote it :) - but the important thing is that IPython itself did not need to be modified - the kernel launch args were not configurable, but the launch function was, so it needed a subclass assigned via the KMClass configurable. But that subclass is totally unnecessary if the launch command is configurable in the base class. There is a benefit to adding the configurable to the base class rather than a subclass, because that also means it is inherited by other subclasses (Qt, Blocking, etc.).

The reason it isn't there is probably just that the base KM is not a Configurable, but that's just a historical artifact since it was written before the Config stuff - it absolutely should be a Configurable.

@Carreau
IPython member

Question to @minrk,

Making kernelmanager.launch_kernel configurable will still have people be obliged to write their own launcher function in python to run a custom kernel, Am I wrong ?

Would it make more sens to make entry_point.base_launch_kernel configurable, and/or to provide a way to have launching process so that you don't have to write any python ?

The reason would be that then, the different kernel wouldn't have to deal with writing/installing any python files/module.

I might be missing something in the way this works though...

@minrk
IPython member

Ah, I forgot about the remnants still in entry_point. That means a little bit more needs to change than I thought.

But no, don't make launch_kernel configurable, make the Popen list itself configurable,
and make appropriate changes to base_launch_kernel to allow that.

@minrk
IPython member

a bit more:

base_launch_kernel does two things:

  1. turn some functional arguments into a Popen command
  2. add some wrapper stuff for pipes, etc.

It should be split so those are separate actions, and a custom Popen command skips the whole command assembly step.

@ellisonbg
IPython member

I just had a look at the following modules in IPython.zmq:

  • entry_point.py
  • ipkernel.py
  • kernelapp.py
  • kernelmanager.py

At least from a first glance, they are all sort of a rats nest of functions and classes that call each other. It is tempting to start refactoring this in a clean way, but I worry that it is actually quite subtle because of the different ways that kernels are used. But I also remember that this code grew in a very organic way and hasn't been cleaned up since the beginning. @minrk do you have a sense of why things are such a mess - is that needed? Is part of this the complexity of IPython.parallel also using the same kernel?

@minrk
IPython member
  • I think kernelmanager.py should be fairly well contained. What's wrong with it? Maybe you want to split the channels into their own file? I don't see any particular reason to do that.

  • entry_point shouldn't exist, as it's a vestige of the pre-Application way of launching things. Almost all of it is gone now, with just two utility functions left. They can go somewhere else, if there is a better name for it. I never did move them, unlike the rest of its original contents which I did move various other more appropriate places, because I couldn't come up with a better one.

  • kernelapp and ipkernel are the way they are because there used to be two kinds of kernels - they should definitely be cleaned up, ipkernel especially. Probably move IPKernelApp to its own ipkernelapp or add it to kernelapp. And the extra launch functions at the bottom probably belong wherever the contents of entry_point end up.

It is totally appropriate to clean this up, but do note that from IPython.zmq.ipkernel import IPKernelApp is currently in use as a public API. Be aware of PR #2724, because it should probably result in a zmq-free bases of Kernel / KernelManager, from with the zmq ones will inherit. Any attempt at cleanup will conflict severely with that work.

@minrk
IPython member

Forgot to mention: No, there shouldn't be anything related to IPython.parallel in the complexity here. The last major step of merging the two kernels is to make IPEngineApp a minimal subclass of IPKernelApp.

@ellisonbg
IPython member

Thanks Min that really helps. I guess with #2724 I will just wait. I can accomplish everything I need to with the current design, it is just a bit messy.

  • I think the only problem with kernelmanager.py is how the launch_kernel function is customized in a wonky way by passing a launcher kwarg to start_kernel. It should probably be a configurable attribute. I have thought about separating the channels into separate code, but that is a minor point.
  • I would separate the files into three clean ones: put the actual kernel implementation in one file, put the applications in another, and put the launching code in another.
  • Is there any need to have separate KernelApp and IPKernelApp classes. It is not at all clear why the two are needed and it means that IPKernelApp has multiple inheritance.

In what way is from IPython.zmq.ipkernel import IPKernelApp a public API? If it makes sense I would love to combine the two applications and put the result into its own file, to leave ipkernel with just the kernel.

@minrk
IPython member

In what way is from IPython.zmq.ipkernel import IPKernelApp a public API?

If you want to start a kernel and customize it in any way whatsoever, you must make this import. It is equivalent to the embedded classes - IPython.embed is fine if you don't want to change anything, but you have to import the class itself to do any customization. People are doing this (maybe not many).

I would separate the files into three clean ones: put the actual kernel implementation in one file, put the applications in another, and put the launching code in another.

Sensible

Is there any need to have separate KernelApp and IPKernelApp classes. It is not at all clear why the two are needed and it means that IPKernelApp has multiple inheritance.

The main reason was to separate IPython-shell-specific startup from just 'generic interpreter startup'. Again, the reason being that we used to have a pure Python Kernel. I think the main argument against merging them is that we may again, but that's not a particularly strong one. I would maybe just put IPKernelApp as-is after KernelApp in zmq.kernelapp, but merging the two is fine as well.

@ellisonbg
IPython member
@ellisonbg
IPython member

I cleaned up the kernel manager code a lot in #2775 but didn't really change how the custom launcher is specified. Bu tin either case, I don't think the approach in this branch is the way to do. In the long run, I think we will want to build an additional argument into MultiKernelManager.start_kernel, namely, the name of the kernel to start: ipython, ruby, etc. This list should be where the configuration happens, Each kernel name should be associated with the required launcher. For now can we just get by with what we have until we really get to thinking about multi-kernel support an what that will look like?

@minrk
IPython member

@ellisonbg that approach doesn't get users anywhere without updates to IPython itself. I think the point here is that we should have a configurable for launching custom kernels, either with

A) a custom launch_kernel function, as a DottedObjectName
or B) a custom Popen command-line list.

Now, maybe this should be a mapping by kernel-name, but in any case, we should expose the fact that the KernelManager as it already is can launch custom kernels. It's just not exposed to config.

It should be straightforward to add one or both of these, particularly now that KernelManager itself is Configurable, as it always should have been.

@ellisonbg
IPython member
@Carreau
IPython member
@ellisonbg
IPython member

@minrk and @Carreau I never heard back from you if we can close this PR and open an issue for the broader kernel launcher work.

@minrk
IPython member

Actually you did, just not here. I was going to submit a new PR with the changes discussed here (split base_launch_kernel into its two separate actions, and expose Popen command as KM config, avoiding the need for subclass), and suggest closing this when I submit that, rather than closing without having a replacement to point to.

@ellisonbg
IPython member
@minrk
IPython member

replaced by #2854

@minrk minrk closed this Jan 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment