Skip to content
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

switch to IPython v3 protocol, require IPython 3.0 or later #325

Merged
merged 2 commits into from Jul 2, 2015

Conversation

stevengj
Copy link
Member

It seems like the time is ripe to switch completely to IPython 3. This means that IPython 2 is no longer supported.

cc: @shashi, @Carreau, @minrk

@stevengj
Copy link
Member Author

By the way, one thing that was unclear to me in the protocol spec was what messages to send if there is an exception thrown from the execute_request code. Experimentally, it seems like we need to send both an error message and an execute_reply with status="error" in order for the traceback to show up. This is confusing because the execute_reply message already contains the traceback; why is the error message required too?

@Carreau
Copy link
Contributor

Carreau commented Jun 25, 2015

I'm not sure anymore but that's because one of the channel is REQ/REP (so you need to REP or you block the rest) and the other is PUB/SUB for when you have many clients connected that "monitor" what is hapening.

@stevengj
Copy link
Member Author

One argument against doing this now is that Ubuntu is apparently still shipping IPython 2.3. Is that a concern for us?

@minrk
Copy link
Contributor

minrk commented Jun 28, 2015

@stevengj a request should always produce a reply that is a concise indicator of success or failure. The IOPub channel handles the messages that produce rich representations of output. The result is there's a little redundancy in the execute_reply message and error. It is probably a mistake for execute_reply to include the traceback.

@Carreau it is not REQ/REP, but DEALER/ROUTER, so the lock-step is not strictly enforced. The conclusion is correct, though.

@Carreau
Copy link
Contributor

Carreau commented Jun 29, 2015

@stevengj a request should always produce a reply that is a concise indicator of success or failure. The IOPub channel handles the messages that produce rich representations of output. The result is there's a little redundancy in the execute_reply message and error. It is probably a mistake for execute_reply to include the traceback.

Do we want to note that somewhere to simplify msgspec v6 (if v6 is here one day)

@minrk
Copy link
Contributor

minrk commented Jun 29, 2015

@Carreau it's probably a good idea to keep a long-running note of things we don't like / may change in the msg spec. Probably an Issue on jupyter_client is good enough (I again regret not calling that repo jupyter_protocol), if you want to open it.

@stevengj
Copy link
Member Author

Rebased.

@stevengj
Copy link
Member Author

Updated the README and the notebook() command in Julia itself to no longer use or refer to --profile julia.

I guess this means that the IPython qtconsole and console front-ends are no longer supported from Julia, since we no longer have a profile. But I don't think anyone was using these...?

@Carreau
Copy link
Contributor

Carreau commented Jun 30, 2015

it could be nice to "revive" them, and we can perfectly update to use the new kernel architecture.
I guess they already do, I just don't remember the command line flags.

@stevengj
Copy link
Member Author

It's nice to have them as long as they can use the new kernel architecture; I don't want to go back to maintaining a separate profile.

@stevengj
Copy link
Member Author

Ah, great, ipython qtconsole --kernel julia-0.3 does the trick.

@stevengj
Copy link
Member Author

It gives an error because we aren't including a payload:

  File "/Users/stevenj/anaconda/lib/python2.7/site-packages/IPython/qt/console/frontend_widget.py", line 767, in _process_execute_ok
    payload = msg['content']['payload']
KeyError: 'payload'

The messaging spec says that the payload in the execute_reply is deprecated and optional. Is this a bug in the qtconsole?

@Carreau
Copy link
Contributor

Carreau commented Jun 30, 2015

Awesome ! If you are on recent enough, I guest you can even jupyter qtconsole --kernel julia-0.3 !

@stevengj
Copy link
Member Author

Same thing in ipython console --kernel julia-0.3

  File "/Users/stevenj/anaconda/lib/python2.7/site-packages/IPython/terminal/console/interactiveshell.py", line 201, in handle_execute_reply
    for item in content["payload"]:
KeyError: 'payload'

@Carreau
Copy link
Contributor

Carreau commented Jun 30, 2015

To any question "is it a bug in qtconsole" the answer is most likely "yes", as none of us use it.

@Carreau
Copy link
Contributor

Carreau commented Jun 30, 2015

I'll open issues on both.

@stevengj
Copy link
Member Author

stevengj commented Jul 1, 2015

@shashi, @jiahao, looks good to merge?

One of the big benefits of doing this is that the Interact package will no longer have to have two separate branches for IPython 2 vs. 3.

@stevengj
Copy link
Member Author

stevengj commented Jul 1, 2015

(It would be nice to have a Travis test suite for IJulia. How does IPython do its testing?)

@minrk
Copy link
Contributor

minrk commented Jul 1, 2015

For the browser/notebook stuff? We use casper+phantomjs for browser tests, but aren't happy with it, largely due the phantom 1.9 being pinned to the ancient WebKit in Qt4. Phantom 2 is based on Qt5, which may alleviate some of the issues (until Qt5 WebKit becomes outdated).

@stevengj
Copy link
Member Author

stevengj commented Jul 1, 2015

@minrk, how do you test the IPython kernel? Presumably this can be done in a front-end-independent fashion; we'll leave the browser testing to you.

@minrk
Copy link
Contributor

minrk commented Jul 1, 2015

Ah, we have some tests that use jupyter_client to exercise the kernel. We have started a project called jupyter_kernel_test, which is meant to make it easier to do similar exercises for other Jupyter kernels. It's brand new, though. We'd love feedback.

@stevengj
Copy link
Member Author

stevengj commented Jul 2, 2015

@minrk, looks like what we would want, except that it requires bleeding-edge versions of everything. (I have IPython 3.2, and that's not good enough even if I install all of the other Jupyter modules from git, because the kernel test relies on ipython/ipython#8214.) This means that we probably can't use it for automated testing for quite some time, unfortunately.

@stevengj stevengj mentioned this pull request Jul 2, 2015
@Carreau
Copy link
Contributor

Carreau commented Jul 2, 2015

Well 4.x should be released soonish.
You can most likely set TravisCI with a matrix where you get differents versions of the Python packages. So it is most likely possible to get testing.

Let's how many kernels authors are at SciPy and if we can polis that.

@minrk
Copy link
Contributor

minrk commented Jul 2, 2015

@stevengj I expect we should have the relevant pieces out within a week or so. I don't think we should make the test tool require IPython, so I will look into why that's happening now.

@minrk
Copy link
Contributor

minrk commented Jul 2, 2015

@stevengj the IPython.paths reliance was a mistake, and shouldn't be in there much longer.

@stevengj
Copy link
Member Author

stevengj commented Jul 2, 2015

Thanks @minrk! It might be nice to allow the full path of the kernel spec file to be specified, so that you could test a kernel without actually installing it.

@minrk
Copy link
Contributor

minrk commented Jul 2, 2015

That's a good idea. Want to open an issue for that on the test repo?

@stevengj
Copy link
Member Author

stevengj commented Jul 2, 2015

I've made an ipython2 branch to make it easier for people to use the old version if they can't upgrade IPython for some reason.

stevengj added a commit that referenced this pull request Jul 2, 2015
switch to IPython v3 protocol, require IPython 3.0 or later
@stevengj stevengj merged commit 5be8253 into master Jul 2, 2015
@stevengj stevengj deleted the ipython_v3 branch July 2, 2015 20:28
@shashi
Copy link
Contributor

shashi commented Jul 5, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants