add IPython.embed_kernel() #1357

scottt · 2012-01-31T00:36:20Z

This is the second submission of this patch, revised to address concerns raised in:
#1351 (comment)

Allow IPython.zmq import to fail since IPython does not require zmq to exist.
This disables the feature.
Pass the namespace of the function calling embed_kernel() down as user_ns

This patch adds IPython.embed_kernel() as a public API.
Embedding an IPython kernel in an application is useful when you want to
use IPython.embed() but don't have a terminal attached on stdin and stdout.

My use case is a modern gdb with Python API support
#!/usr/bin/gdb --python
import IPython; IPython.embed_kernel()
this way I get to use ipython to explore the GDB API without
the readline librarry in gdb and ipython fighting over the terminal settings.

A Google search revealed that other people were interetsted in this use
case as well:
http://mail.scipy.org/pipermail/ipython-dev/2011-July/007928.html

This patch adds IPython.embed_kernel() as a public API. Embedding an IPython kernel in an application is useful when you want to use IPython.embed() but don't have a terminal attached on stdin and stdout. My use case is a modern gdb with Python API support #!/usr/bin/gdb --python import IPython; IPython.embed_kernel() this way I get to use ipython to explore the GDB API without the readline librarry in gdb and ipython fighting over the terminal settings. A Google search revealed that other people were interetsted in this use case as well: http://mail.scipy.org/pipermail/ipython-dev/2011-July/007928.html

minrk · 2012-01-31T00:58:52Z

Now the default behavior is to embed the Kernel into the calling application scope, which is incorrect for the basic IPython kernel. This is why the embed entry point and the regular entry point in terminal IPython are not the same. One is not a subset of the other.

scottt · 2012-01-31T01:18:40Z

I'm reading up on traitlets.

scottt · 2012-01-31T01:41:05Z

@minrk, I think I addressed the concerns raised so far.

minrk · 2012-01-31T02:20:42Z

nice! Thanks. Pinging @fperez, who was asking about exactly this feature last week.

One thing you might change from a user perspective is slightly different handling of the failed import. Right now, if pyzmq is not installed, trying IPython.embed_kernel() will raise an AttributeError. Perhaps it would be better to fallback on something with a more informative error (ImportError("IPython.zmq requires pyzmq ≥ 2.1.4") being the most logical error to raise).

fperez · 2012-01-31T02:22:05Z

Awesome, @scottt, many thanks! I have a long trip tomorrow so I'll be offline for a day or two, but if nobody beats me to it I'll definitely play with this and review it before the end of the week.

scottt · 2012-01-31T02:38:09Z

@minrk, I tweaked the error message to be ImportError("IPython.embed_kernel requires pyzmq >= 2.14") as refering to IPython.embed_kernel instead of IPython.zmq seems clearer to me.
Thanks a bunch for the fast responses and pointers.

@fperez, Looking forward for the review :)
My last patch to IPython was 6+ years ago, thanks for all your work on IPython. I love this software!

minrk · 2012-01-31T02:47:12Z

IPython/__init__.py

+    from .zmq.ipkernel import embed_kernel
+except ImportError:
+    def embed_kernel(*args, **kwargs):
+        raise ImportError("IPython.embed_kernel requires pyzmq >= 2.14")


This should be 2.1.4, not 2.14

I'm not sure about catching ImportError here. It could mask a real problem with another import this triggers. Also, having this import here will load the ZMQ code even if it's not needed (i.e. in the plain terminal client). I think we can work round both issues by doing something like this:

def embed_kernel(*args, **kwargs): # Lazy import from .zmq.ipkernel import embed_kernel as real_embed_kernel real_embed_kernel(*args, **kwargs)

@takluyver, I'm sympathetic to the two issues you raised:

Catching all ImportError's could mask the module really causing it.

Always importing ZMQ is unnecessary for normal IPython use.

For functions that examine the local variables of their callers you can't just add another layer that wraps them though. i.e.real_embed_kernel() would end up inspecting the locals and module globals of embed_kernel() instead of the caller of embed_kernel() in your snippet. I ended up changing the code like: scottt@087e9c3

minrk · 2012-01-31T03:16:05Z

IPython/zmq/ipkernel.py

+    if local_ns is None:
+        local_ns = caller_locals
+    app = IPKernelApp.instance(user_module=module, user_ns=local_ns)
+    app.initialize()


You should pass initialize an empty list here (app.initialize([])), so that it doesn't try to parse sys.argv, which could be problematic if called from an environment that used its own command-line args.

https://github.com/ipython/ipython/pull/1357/files#r400153 Stop embed_kernel() from parsing sys.argv since to support being called from a environments with their own command line args.

scottt · 2012-03-11T03:07:58Z

After reading #1357 (comment), I changed the code slightly in scottt@087e9c3

embed_kernel() now only imports IPython.zmq.ipkernel when called. So as not to cause unnecessary I/O and slow down IPython startup for an infrequently used feature.

I also stopped catching ImportError for IPython.zmq.ipkernel. I found that IPython/zmq/init.py would catch and replace the ImportError message with "IPython.zmq requires pyzmq >= 2.1.4" which seems clear enough. Not doing the import check twice also means the minimum ZMQ version required can stay written only in one place.

takluyver · 2012-03-12T14:13:28Z

IPython/utils/frame.py

@@ -85,3 +85,10 @@ def debugx(expr,pre_msg=''):
 # deactivate it by uncommenting the following line, which makes it a no-op
 #def debugx(expr,pre_msg=''): pass

+def caller_module_and_locals():
+    """Returns (module, locals) of the caller"""


We should clarify this a bit - 'the caller' would normally mean the frame in which this is called, whereas this actually looks at the frame above that.

Possibly the function should take an optional depth argument similar to extract_vars.

@takluyver,I didn't want to add a depth argument to ``caller_module_and_locals()` without renaming the function. See if you like scottt@a61f445

Thanks, that looks good.

takluyver · 2012-03-12T16:52:06Z

IPython/__init__.py

+def embed_kernel(module=None, local_ns=None):
+    """Call this to embed an IPython kernel at the current point in your program. """
+    (caller_module, caller_locals) = extract_module_locals_above()
+    if module is None:


I think this logic to get the calling scope should be inside the embed_kernel function in ipkernel, so that it has exactly the same API if it's imported from there.

I agree with @takluyver here.

fperez · 2012-03-13T22:24:09Z

@scottt, this is great! One thing I'd like to have before we merge it, is at least one or two tests. Because of the kernel embedding, they might need to go into a new test group, so let us know if you need a hand. But I don't want to build too much more technical debt without new tests.

In this case the test should be pretty simple to write: just start a new process with an embedded kernel that has a namespace configured with one or two variables, check those values from the test, run a piece of code in there, check the result, done.

Let us know if you need a hand with the test, shouldn't be hard.

fperez · 2012-03-13T22:25:01Z

Finally, this new capability should also be mentioned in the manual, in the embedding section.

bfroehle · 2012-03-14T16:52:35Z

IPython/utils/frame.py

+    module = sys.modules[global_ns['__name__']]
+    return (module, f.f_locals)
+
+def extract_module_locals_above():


This function seems overly specific. Why not just replace its one use with extract_module_locals(1)? I see that there is an extract_vars_above function, but I don't think it's ever actually used.

Maybe I just bristle at the word above 'above'. extract_caller_module_locals, similar to a previous patch of yours, makes more sense to me.

I don't think caller properly conveys what this is doing: that would imply it looks at the frame which calls this function, whereas its looking at the frame above that. above seems less ambiguous.

I agree that the more general function is probably sufficient for the limited use of this, though.

minrk · 2012-04-15T01:36:18Z

This currently breaks all regular usage of the KernelApp, with:

AttributeError: 'IPKernelApp' object has no attribute 'user_module'

Which should be fixed by adding the user_module/user_ns to the IPKernelApp as traitlets.

fperez · 2012-04-16T22:22:32Z

@scottt, what's your take on this one? This is kind of a high-priority/value PR for us, so if you're swamped and don't think you'll be able to work on it, let us know and we'll do our best to pitch in and give you a hand. It seems there's a bit of fixing still needed before we can merge it...

minrk · 2012-04-18T21:07:39Z

This is super close, so I'm happy to take it over the line, if you want.

The only changes requested by review seem to be:

add user_module/ns to IPKernelApp
remove unnecessarily specific extract_module_locals_above()
add a docstring and basic test and docs

Those seem quite straightforward, and I'm happy to do them.

One thing we might want to add is passing config to the embedded Kernel, changing the sig to:

def embed_kernel(module=None, local_ns=None, **kwargs):
    KernelApp = IPKernelApp.instance(**kwargs) 
    ...

This better matches the existing IPython.embed()

minrk · 2012-04-18T21:41:17Z

Actually, come to think of it, user_module/_ns should not be part of IPKernelApp. The embedding should just set them directly on the kernel.

fperez · 2012-04-18T23:08:16Z

@minrk, are you sure? I seem to recall that we need to 'prepare' a namespace that will be used by the kernel by adding a few names to it (like __file__, get_ipython, etc). In that scenario, it's better to get the dict the user wants in the constructor, so we can manipulate it before the interactive loop starts.

minrk · 2012-04-19T02:09:57Z

@minrk, are you sure? I seem to recall that we need to 'prepare' a namespace that will be used by the kernel by adding a few names to it (like __file__, get_ipython, etc). In that scenario, it's better to get the dict the user wants in the constructor, so we can manipulate it before the interactive loop starts.

That's not how it works in the current embedded terminal, and there's no reason embed_kernel should be different.

minrk · 2012-04-19T03:22:30Z

@fperez I've got a working version here, where the user_module/ns are attached only to the Kernel, and behave essentially the same as EmbeddedShell (changing user_ns triggers init_user_ns() which does the setup).

fperez · 2012-04-19T05:07:09Z

I certainly agree that the two should match, I just wasn't sure the embedded terminal was fully correct :) Is the init_user_ns() step triggered by traitlets event handling?

minrk · 2012-04-19T17:42:54Z

it is not in EmbeddedShell, it is in the Kernel

zmq.ipkernel: don't call embed_kernel() in main()

8f53ec7

zmq.Kernel: turn user_{module,ns} into traitlets

a92b812

embed_kernel: give a clear error message on pyzmq ImportError

a96b676

minrk reviewed Jan 31, 2012
View reviewed changes

embed_kernel: fix pyzmq version requirement error message

a2ec8bd

minrk reviewed Jan 31, 2012
View reviewed changes

scottt added 2 commits January 31, 2012 21:49

embed_kernel: pass [] to IPKernelApp.initialize to prevent argv parsing

75b852d

https://github.com/ipython/ipython/pull/1357/files#r400153 Stop embed_kernel() from parsing sys.argv since to support being called from a environments with their own command line args.

embed_kernel: only import zmq when needed

087e9c3

Move caller_module_and_locals() to IPython.util.frame

030fc46

takluyver reviewed Mar 12, 2012
View reviewed changes

Refactor caller_module_locals() into extract_module_locals()

a61f445

takluyver reviewed Mar 12, 2012
View reviewed changes

bfroehle reviewed Mar 14, 2012
View reviewed changes

ghost assigned fperez Apr 15, 2012

minrk mentioned this pull request Apr 21, 2012

Finish up embed_kernel #1640

Merged

fperez merged commit a61f445 into ipython:master Apr 23, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add IPython.embed_kernel() #1357

add IPython.embed_kernel() #1357

scottt commented Jan 31, 2012

minrk commented Jan 31, 2012

scottt commented Jan 31, 2012

scottt commented Jan 31, 2012

minrk commented Jan 31, 2012

fperez commented Jan 31, 2012

scottt commented Jan 31, 2012

minrk Jan 31, 2012

takluyver Mar 11, 2012

scottt Mar 11, 2012

minrk Jan 31, 2012

scottt commented Mar 11, 2012

takluyver Mar 12, 2012

scottt Mar 12, 2012

takluyver Mar 12, 2012

takluyver Mar 12, 2012

fperez Mar 13, 2012

fperez commented Mar 13, 2012

fperez commented Mar 13, 2012

bfroehle Mar 14, 2012

takluyver Mar 14, 2012

minrk commented Apr 15, 2012

fperez commented Apr 16, 2012

minrk commented Apr 18, 2012

minrk commented Apr 18, 2012

fperez commented Apr 18, 2012

minrk commented Apr 19, 2012

minrk commented Apr 19, 2012

fperez commented Apr 19, 2012

minrk commented Apr 19, 2012

add IPython.embed_kernel() #1357

add IPython.embed_kernel() #1357

Conversation

scottt commented Jan 31, 2012

minrk commented Jan 31, 2012

scottt commented Jan 31, 2012

scottt commented Jan 31, 2012

minrk commented Jan 31, 2012

fperez commented Jan 31, 2012

scottt commented Jan 31, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottt commented Mar 11, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fperez commented Mar 13, 2012

fperez commented Mar 13, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minrk commented Apr 15, 2012

fperez commented Apr 16, 2012

minrk commented Apr 18, 2012

minrk commented Apr 18, 2012

fperez commented Apr 18, 2012

minrk commented Apr 19, 2012

minrk commented Apr 19, 2012

fperez commented Apr 19, 2012

minrk commented Apr 19, 2012