Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

In-process kernel support (take 3) #2724

Merged
merged 17 commits into from Jan 16, 2013

Conversation

Projects
None yet
Contributor

pberkes commented Dec 27, 2012

This PR continues PR #2397 now that @epatters is offline. The previous PR has been closed and this one will take its place.

I fixed the Python3 issues that got PR #2397 stuck.

Contributor

pberkes commented Dec 27, 2012

As I mentioned before, this PR covers a frequent use case for me and I think it would be great to have it in IPython. Based on comments on the previous PR by @fperez , it also sounds like it would be useful for IPython in general. I have some time to spend on it at the moment if there is any other issue to be fixed...

BTW, I tried merging the PR to master and all tests stil pass.

Owner

takluyver commented Dec 28, 2012

Thanks for tackling this. It might be a few days before anyone looks at it in detail, but we're definitely interested in supporting this case.

Contributor

jdmarch commented Jan 10, 2013

Who would be plausible reviewers for this PR?

Owner

takluyver commented Jan 11, 2013

Maybe @epatters and @minrk ?

Contributor

epatters commented Jan 15, 2013

@takluyver, I have reviewed the Python 3 compatibility fixes. The fixes look good (thanks, @pberkes!).

At the time of my departure, these were the only outstanding issues of which I was aware. I hope that this PR can now move forward.

Owner

takluyver commented Jan 15, 2013

Thanks @epatters .

@minrk : are you happy for this to be merged?

Contributor

jdmarch commented Jan 15, 2013

Thanks, @epatters.
@ellisonbg, in #2643, also expressed willingness to help review this ticket, if more review is needed.

Owner

minrk commented Jan 15, 2013

I think this looks pretty solid now. My only thought is that I would put these new base classes in IPython.kernel instead of IPython.inprocess.

@ellisonbg ellisonbg commented on the diff Jan 15, 2013

IPython/inprocess/blockingkernelmanager.py
+
+# Standard library imports.
+import Queue
+from threading import Event
+
+# Local imports.
+from IPython.utils.io import raw_print
+from IPython.utils.traitlets import Type
+from kernelmanager import InProcessKernelManager, ShellInProcessChannel, \
+ SubInProcessChannel, StdInInProcessChannel
+
+#-----------------------------------------------------------------------------
+# Utility classes
+#-----------------------------------------------------------------------------
+
+class BlockingChannelMixin(object):
@ellisonbg

ellisonbg Jan 15, 2013

Owner

I am not sure it makes sense for these base classes to in in IPython.inprocess, I view the main kernel code to be more fundamental.

@epatters

epatters Jan 15, 2013

Contributor

This seems reasonable and @minrk evidently shares this view. Are BlockingChannelMixin and SocketABC the only classes that need to be moved?

@ellisonbg ellisonbg commented on the diff Jan 15, 2013

IPython/zmq/ipkernel.py
@@ -739,36 +753,6 @@ def _complete(self, msg):
cpos = len(c['line'])
return self.shell.complete(c['text'], c['line'], cpos)
- def _object_info(self, context):
@ellisonbg

ellisonbg Jan 15, 2013

Owner

Where did the implementation of _object_info go?

@epatters

epatters Jan 15, 2013

Contributor

This was dead code, left over from the very early days of the kernel. I took the liberty of removing it.

@ellisonbg ellisonbg commented on the diff Jan 15, 2013

IPython/inprocess/kernelmanager.py
+ self._pause = True
+
+ def unpause(self):
+ """ Unpause the heartbeat. """
+ self._pause = False
+
+ def is_beating(self):
+ """ Is the heartbeat running and responsive (and not paused). """
+ return not self._pause
+
+
+#-----------------------------------------------------------------------------
+# Main kernel manager class
+#-----------------------------------------------------------------------------
+
+class InProcessKernelManager(HasTraits):
@ellisonbg

ellisonbg Jan 15, 2013

Owner

After this PR is merged, I am going to do some cleanup work on the zmq.KernelManager. I am going to make it a Configurable. Do you want to use that base class here? Does this class have the same public API as zmq.KernelManager. Would it make sense to share any of the implementation with a common base class? Also, do the channel classes above have the same API?

@epatters

epatters Jan 15, 2013

Contributor

I have tried to make the in-process kernel manager API-compatible with the standard kernel manager. Under ideal circumstances, they should be drop-in replacements for each other. Certain methods, like signal_kernel, do not make sense for the in-process kernel manager and therefore raise exceptions; similarly, certain attributes, like the ZMQ context, are not present on the in-process channel objects. Apart from this, the interfaces should be identical, including for the channel classes. Ultimately, I would like to see both kernel managers inherit from some abstract base class to clarify which subset of the interface is guaranteed to be common.

Sharing implementation was not useful for the kernel managers, though I did share implementation as appropriate in the blocking and Qt channel classes.

@minrk

minrk Jan 15, 2013

Owner

Ah, I somehow missed this when reading it most recently. I think the shared base class for inprocess and zmq KernelManagers should be a criterion for merge.

@epatters

epatters Jan 15, 2013

Contributor

To be clear, the shared base class is useful only for indicating shared interface; there is no implementation to be shared here. (If I were writing ETS code, I would use an Interface for this). Given that, the issue does not seem especially pressing. Besides, @ellisonbg has already indicated his desire to work on this after the merge.

@ellisonbg

ellisonbg Jan 16, 2013

Owner

I can create a KernelManager ABC in a subsequent PR. What else needs to be
done to finish this one up? Do we want the BlockingKernelManager
subclassing to be cleaned up? There is other work blocked on this PR now,
so it would be good to finish it up. But let's first figure out exactly
what needs to be done for this to go in (maybe make a task list!)

On Tue, Jan 15, 2013 at 3:43 PM, Evan Patterson notifications@github.comwrote:

In IPython/inprocess/kernelmanager.py:

  •    self._pause = True
    
  • def unpause(self):
  •    """ Unpause the heartbeat. """
    
  •    self._pause = False
    
  • def is_beating(self):
  •    """ Is the heartbeat running and responsive (and not paused). """
    
  •    return not self._pause
    
    +#-----------------------------------------------------------------------------
    +# Main kernel manager class
    +#-----------------------------------------------------------------------------
    +
    +class InProcessKernelManager(HasTraits):

To be clear, the shared base class is useful only indicating shared
interface; there is no implementation to be shared here. (If I were writing
ETS code, I would use an Interface for this). Given that, the issue does
not seem especially pressing. Besides, @ellisonbghttps://github.com/ellisonbghas already indicated his desire to work on this after the merge.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2724/files#r2658749.

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

@minrk

minrk Jan 16, 2013

Owner

If you want to make a KM ABC as another PR,
then I have no more objections to this as-is.

On Wed, Jan 16, 2013 at 10:12 AM, Brian E. Granger <notifications@github.com

wrote:

In IPython/inprocess/kernelmanager.py:

  •    self._pause = True
    
  • def unpause(self):
  •    """ Unpause the heartbeat. """
    
  •    self._pause = False
    
  • def is_beating(self):
  •    """ Is the heartbeat running and responsive (and not paused). """
    
  •    return not self._pause
    
    +#-----------------------------------------------------------------------------
    +# Main kernel manager class
    +#-----------------------------------------------------------------------------
    +
    +class InProcessKernelManager(HasTraits):

I can create a KernelManager ABC in a subsequent PR. What else needs to be
done to finish this one up? Do we want the BlockingKernelManager
subclassing to be cleaned up? There is other work blocked on this PR now,
so it would be good to finish it up. But let's first figure out exactly
what needs to be done for this to go in (maybe make a task list!)
… <#13c4490b6baaeeee_>
On Tue, Jan 15, 2013 at 3:43 PM, Evan Patterson notifications@github.comwrote:
In IPython/inprocess/kernelmanager.py: > + self._pause = True > + > + def
unpause(self): > + """ Unpause the heartbeat. """ > + self._pause = False >

    • def is_beating(self): > + """ Is the heartbeat running and responsive
      (and not paused). """ > + return not self._pause > + > + >
      +#-----------------------------------------------------------------------------
      +# Main kernel manager class >
      +#-----------------------------------------------------------------------------
    • +class InProcessKernelManager(HasTraits): To be clear, the shared
      base class is useful only indicating shared interface; there is no
      implementation to be shared here. (If I were writing ETS code, I would use
      an Interface for this). Given that, the issue does not seem especially
      pressing. Besides, @ellisonbg https://github.com/ellisonbg<
      https://github.com/ellisonbg>has already indicated his desire to work on
      this after the merge. — Reply to this email directly or view it on GitHub<
      https://github.com/ipython/ipython/pull/2724/files#r2658749>.
      -- Brian E. Granger Cal Poly State University, San Luis Obispo
      bgranger@calpoly.edu and ellisonbg@gmail.com


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2724/files#r2669297.

@ellisonbg

ellisonbg Jan 16, 2013

Owner

OK let's merge this then.

Owner

ellisonbg commented Jan 15, 2013

Overall, this look like really clean code. I just left a few inline comments.

ellisonbg added a commit that referenced this pull request Jan 16, 2013

@ellisonbg ellisonbg merged commit 98e3fdd into ipython:master Jan 16, 2013

1 check passed

default The Travis build passed
Details

This is great, but is there any documentation or example code to actually implement this? The old gist that epatters wrote does not work. I am guessing there were some name changes somewhere in here?

I am also excited about this feature, but can't seem to find any documentation about how to use it except for one thing! I did see the attached .ipynb in the PR stack, checked it out on nbviewer, but couldn't get it working. I'm obviously missing something. Perhaps a special way to start ipython notebook from command line? Any pointers would be appreciated. TY!

cpbotha commented May 1, 2013

I would like to double-check: This work has not been backported to the 0.13.x branch, and is currently only available on master, right?

Owner

minrk commented May 1, 2013

Correct

Contributor

rossant commented May 13, 2013

embedded_qtconsole.py (from here) does not appear to work on master:

ImportError: No module named kernelmanager
Owner

minrk commented May 13, 2013

That's correct, some things have been renamed. There is an example in IPython now.

Contributor

rossant commented May 13, 2013

Great, I missed that. Thanks!

hsyed commented May 24, 2013

Could someone please help with my SO question :
http://stackoverflow.com/questions/16737323/embedding-ipython-into-a-pyqt4-app

Hmm, I am wondering if this code even made it into 13.2

Many thanks.

hsyed commented May 28, 2013

Ok, so I modified the code fragment from SO (see above). but I am getting the same error. I am on OS X and using QT (4.8.4) and PyQt4 (4.10.1) The git version of ipython is whatever is current today,

Owner

minrk commented May 28, 2013

There is an example in the IPython tree. Your error indicates that you have imported PyQt prior to importing any IPython objects, and the IPython qt code requires PyQt API v2, which is not the default for PyQt in Python 2.

hsyed commented May 29, 2013

Many thanks, that solved it. I would suggest that the error message mentions the potential import sequence issue.

Owner

minrk commented May 29, 2013

that's a good idea, thanks

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #2724 from pberkes/embedded-ipython-v2
In-process kernel support (take 3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment