-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
In-process kernel support (take 2) #2397
Conversation
@ccordoba12, I'm now responding to your questions in #2382 (comment). Sorry for the delay.
This is just like the out-of-process situation: each frontend should have its own kernel manager, but the kernel managers may be connected to the same kernel. However, unlike the out-of-process situation, you must explicitly tell the kernel about the frontends.
|
@@ -37,7 +37,7 @@ class ZMQShellDisplayHook(DisplayHook): | |||
topic=None | |||
|
|||
session = Instance(Session) | |||
pub_socket = Instance('zmq.Socket') | |||
pub_socket = Any() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of these Any()
traits, do you think it's worthwhile to do a simple trait validator that checks for the necessary interfaces (just send_multipart
, I believe)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IPython requires 2.6 now, doesn't it? We could make an ABC that checks for the necessary methods, and then use a plain Instance
trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Robert. I'll try this.
Thanks for doing this, it does make it easier to see what's actually going on. Ultimately, we certainly will want to make it work without zmq imports, because once it is up to snuff, plain single-process terminal IPython should be using the embedded Kernel/KernelManager code, so we finally have a consolidated codebase. |
I don't have time for a full review, but most of it looks good. One concern is that the embedded kernel manager is almost entirely a copy/paste from the original with no inheritance. That doesn't seem right to me, and the cost of large duplicate code, particularly in a relatively unused path like this (e.g. the old pykernel), has shown itself to be quite high. Can you make the case for not reusing any code? Also, a minor naming consideration: We have a notion of 'embedding', which is IPython running in a particular namespace. This is largely unrelated to that, where embedded means the kernel is in the same process as the frontend. Perhaps there is a better name that we are not already using to mean something else? |
I think you should look at the kernel managers a little more carefully. In contrast to the Also, notice that for the kernel manager subclasses (both blocking and Qt), where it does make sense to share code, I have done so through the (somewhat kludgey) technique of mixin classes. As to the name I don't particularly care. Originally, I eschewed |
Thanks @epatters for your explanation. It seems this really is the right approach for us. One more thing: it's not entirely clear for me if Besides, I downloaded your branch to try to play with it but it's giving me a segmentation fault if I try to run your gist or even when I'm just trying to start qtconsole. I don't know where that could be coming from, because master is working just fine. |
I'm afraid I know very little about IPython's parallel infrastructure. What did you have in mind? As for the segfault---that's not good. What platform are running? What version of PySide or PyQt? Could you give a stack trace? Thanks. |
Could you add an example like that in the gist to the examples directory (at least for Qt - I wouldn't promote the terminal one at present)? Also, one key use case for this is 'embed a shell with access to Python objects already in the process' - what's the most efficient way to start a kernel with specific objects in the namespace? |
Hello, Thomas K guided me to this branch from stack overflow. I am trying to get the example code to run (https://gist.github.com/3659874#file_embedded_qtconsole.py) using this branch. But I am getting the following error : Traceback (most recent call last): I'm not a python expert, or a QT expert (C/C++ is my Forté). I'm also interested in what Thomas is talking about in the previous comment --i.e., passing objects from my pyqt app into the ipython embedded shell. |
@takluyver, I'll work on this. I have a question for you, though. What is the best way to modify/replace the user namespace using @hsyed, are you running PyQt? If so, what version? I only tested the code under PySide---probably a terrible mistake. |
@epatters : If possible, the simplest way is to pass either
|
@epatters Yes I am using PyQt. I suppose I could install pyside. But it will probably go pear shaped since my code runs PyQt ? PyQt is at 4.9.4. |
@hsyed, PyQt should absolutely work. I'm going to test it out right now. |
Or, what I mean to say, is that PyQt is supposed to work. |
@hsyed, this should fix the PyQt4 problems (including yours, @ccordoba12). |
@takluyver, the reason I ask is that I assumed that updating the user namespace would be as simple as
Is that right? |
Good point, thanks. In fact, there's a |
Great, that's exactly what I wanted to know. I'll add an updated example to the docs. |
I wasn't sure exactly where to put this, so I created a 'frontend' directory. |
I haven't tried the code yet. But I was wondering if I could share references between the two interpreters ? Say if I wanted to pass a reference to some functions between a GUI app and an embedded interpreter, would this work ? |
@hsyed, you could indeed share references between the two interpreters, provided that everything remains in process. |
@hsyed: That's what I was just discussing with @epatters. Have a look at the example that's now in the pull request, specifically the As a technical detail, there's only one interpreter, used by both the PyQt application and the in-process kernel. So you just need to add the relevant objects to the namespace that the IPython shell is using. That's what the |
@takluyver fantastic, Was a bit confused since technically one could always serialize a dictionary across using pass-by-copy :D |
Blast, GitHub! Anyway, @fperez, please let me know if anything remains to be done here. |
Oh, and my last day before heading back to school is Friday, so we need to get this wrapped up soon. |
Mmh, something's gone awry with those tests. While running them via nosetests as you did above I get the same results, if I use the official test runner, So I'm afraid that before we merge this one, we'll absolutely have to figure out what's going on here and get the 'real' test suite passing. For more details on how to run our tests, see the testing section of our docs. This is what I get:
|
@fperez, I think this is related to something I was worried about, but which I forgot to bring up here, namely that InteractiveShell seems to be a singleton. This assumption doesn't work so well for the single-process model, in which it is perfectly reasonable to have multiple kernels, each with their own interactive shell. At least, that has been my mental model and it's something that people will expect to work. Here's the code which is surely problematic: I know very little about the motivation and uses for the singleton interactive shell, so you are probably better placed than I am to decide what to do about this. What do you recommend? |
There are two layers to the singleton stuff:
I think we could move away from this assumption, but it would probably take On Wed, Sep 26, 2012 at 1:32 PM, Evan Patterson notifications@github.comwrote:
Brian E. Granger |
Thanks for the information, Brian. What do you recommend we do right now? I certainly can't undertake any serious project-wide refactor. |
Hmm, that is going to be a tough one. I haven't followed the discussions You need only a few things:
What about creating a single kernel that all qtconsoles connect to and just Might be a bit ugly, but would avoid having to dive into some complex On Wed, Sep 26, 2012 at 2:38 PM, Evan Patterson notifications@github.comwrote:
Brian E. Granger |
@epatters, I just made a single-commit PR against your branch that fixes the problem for now. The long-term solution is to cleanly rework our assumptions about where we truly need singletons, but that's indeed far too much of a refactor to undertake in the context of this PR. Now, because of these issues, you are likely only going to be able to use one kernel at a time per-process, but that's still progress. And with these tests in-place, we can then gradually refactor things out until we confine our singleton assumptions only to where they make sense (at the application level, which is where they match the OS singleton - the process). |
…eton. Since the in-process group makes its own shells, it should avoid creating the global singleton.
Thanks, @fperez. I cherry-picked the commit. I can confirm that |
Mmh, it works for fine for me whether I run it on the whole project or part of it... That's very strange... |
BTW, the error reported by the Travis build is b/c your pylab test isn't protected by a decorator to skip it if matplotlib isn't present. See this for an example on how to do it. |
Thanks. Hopefully the tests start passing on the build bot now. |
Did you sort out the problem on your machine? I don't understand what's going on for you... I added this line: print(sys.argv[1:]); sys.exit(0) # dbg at line 502 of Why don't you try debugging it there until we sort it out... What platform are you on? |
@fperez, I will look at this first thing tomorrow. Unfortunately, I have a Friday deadline for some unrelated work and will be offline for the rest of the day. |
Oh, and my work machine is OS X. |
OK; @minrk, @ellisonbg if you guys could run test_pr on this one from your OSX boxes, it would help us to have an extra data point, as it's passing fine for me on linux. @epatters, the current failure on Travis looks like a legitimate issue on 3.2 only (the 2.x builds are now passing). Could you look into that? And that log shows one thing we need to fix: the in-process kernel should be initialized with tracebacks in no-color mode so that we can read them without all the ANSI gunk on the screen. |
passed on 2.6/7, failed on 3.2. |
I believe the failure is coming because
or maybe
Edit: hmf, maybe not. I should try these suggestions out first. Anyway, I second @fperez's suggestion to turn off the colorization (how?) so the traceback is readable. |
Hello again, I'm wondering if there is a way of getting the widget to get keyboard focus when I click on it ? I have my ipython widget in a tab layout and when I click on the tab I have to click again into the widget, it would be nice if the currsor would move to its position when the widget is activated. Also, do you know how I would configure the embedded shell to have |
@epatters I'm just checking the pull request queue. Will you have time to sort out the |
Any update on this? I've been messing around with putting this into a project but I don't want to get left behind by the main trunk of ipython. |
Hi, I just wanted to bump this pull request: using an in-process shell for application scripting is a scenario that I see quite frequently and it would be great to have it in IPython. What is left to be done for the PR to be merged? I may be able to spend some time on it. |
Closing because superseded by PR #2724 |
This PR is functionally equivalent to #2382 but involves substantially less refactoring. Note, however, that ZMQ is now required.
The gist referenced in the previous PR contains usage examples: https://gist.github.com/3659874.