Fixes ipython printing extension #1853

Merged
merged 5 commits into from Mar 8, 2013

Conversation

Projects
None yet
4 participants
@flacjacket
Member

flacjacket commented Mar 1, 2013

This fixes two things.

First, the extension had to be reloaded to import the extension. Fixes issue #3619 [1].

Second, this has been broken since IPython moved IPython.zmq to IPython.kernel.zmq. This PR fixes this. Note, there are some git versions of IPython where this will still break (between moving the zmq module and changing the version to 1.0.dev), but it should work for all stable releases and will work for the current git version. The versions that won't work are between 1/29 [2] and 2/15 [3]. This was brought up in #1757.

[1] https://code.google.com/p/sympy/issues/detail?id=3619
[2] ipython/ipython@37f3225
[3] ipython/ipython#2927

Fixes ipython printing extension
This fixes two things.

First, the extension had to be reloaded to import the extension.
Fixes issue #3619

Second, this has been broken since IPython moved IPython.zmq to
IPython.kernel.zmq. This fixes this. Note, there are some git
versions of IPython where this will still break, but it should work
for all stable releases and will work for the current git version.
@jrioux

This comment has been minimized.

Show comment Hide comment
@jrioux

jrioux Mar 1, 2013

Member

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (8fc7070) into master (3bb09d3).
✳️ PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
✳️ Python 2.7.2-final-0: pass
✳️ Python 3.2.1-final-0: pass
✳️ Sphinx 1.1.3: pass
Docs build command: make clean && make html-errors && make latex && cd _build/latex && xelatex sympy-*.tex

Member

jrioux commented Mar 1, 2013

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (8fc7070) into master (3bb09d3).
✳️ PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
✳️ Python 2.7.2-final-0: pass
✳️ Python 3.2.1-final-0: pass
✳️ Sphinx 1.1.3: pass
Docs build command: make clean && make html-errors && make latex && cd _build/latex && xelatex sympy-*.tex

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 1, 2013

Owner

Thanks for your continual fixing of this extension.

Does this address https://code.google.com/p/sympy/issues/detail?id=3566 (which I don't entirely understand, but hopefully you do)? I'm guessing it doesn't address https://code.google.com/p/sympy/issues/detail?id=3513, which is the only other issue with the IPython extension that I know about.

Owner

asmeurer commented Mar 1, 2013

Thanks for your continual fixing of this extension.

Does this address https://code.google.com/p/sympy/issues/detail?id=3566 (which I don't entirely understand, but hopefully you do)? I'm guessing it doesn't address https://code.google.com/p/sympy/issues/detail?id=3513, which is the only other issue with the IPython extension that I know about.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 1, 2013

Owner

Oh, well clearly the first part of issue 3566 is addressed.

Owner

asmeurer commented Mar 1, 2013

Oh, well clearly the first part of issue 3566 is addressed.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 1, 2013

Owner

Is is possible to test the extension, at least partially? If so, we should add tests to sympy/interactive/tests/test_ipython.py so that we can try to avoid regressions on this.

Owner

asmeurer commented Mar 1, 2013

Is is possible to test the extension, at least partially? If so, we should add tests to sympy/interactive/tests/test_ipython.py so that we can try to avoid regressions on this.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 1, 2013

Owner

So if I understand the changes correctly, the extension is only broken in git IPython, right? I'm just wondering how pressing the need to do a release to get this fix out is.

Owner

asmeurer commented Mar 1, 2013

So if I understand the changes correctly, the extension is only broken in git IPython, right? I'm just wondering how pressing the need to do a release to get this fix out is.

@flacjacket

This comment has been minimized.

Show comment Hide comment
@flacjacket

flacjacket Mar 1, 2013

Member

The second part of 3566 isn't addressed, but doing so will probably be necessary to address 3604. As it stands, even if we were to let it reload, it wouldn't change anything, so having that line doesn't adversely affect it. It doesn't do anything for 3513.

I haven't thought about testing, but some minor testing should be possible if you have IPython installed.

This only affects the git version, until 1.0 is released.

Member

flacjacket commented Mar 1, 2013

The second part of 3566 isn't addressed, but doing so will probably be necessary to address 3604. As it stands, even if we were to let it reload, it wouldn't change anything, so having that line doesn't adversely affect it. It doesn't do anything for 3513.

I haven't thought about testing, but some minor testing should be possible if you have IPython installed.

This only affects the git version, until 1.0 is released.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 2, 2013

Owner

Yes, of course it will require IPython to be installed to test.

I guess we need to be aware of the IPython release schedule then. Unfortunately, the way things are going, they will probably get a release out sooner than we will, since our release process is so slow and broken.

Owner

asmeurer commented Mar 2, 2013

Yes, of course it will require IPython to be installed to test.

I guess we need to be aware of the IPython release schedule then. Unfortunately, the way things are going, they will probably get a release out sooner than we will, since our release process is so slow and broken.

@flacjacket

This comment has been minimized.

Show comment Hide comment
@flacjacket

flacjacket Mar 2, 2013

Member

They probably will release faster, but if (when) 1.0 is pushed, we could probably get a point release out, even if it's no more than just that change.

Member

flacjacket commented Mar 2, 2013

They probably will release faster, but if (when) 1.0 is pushed, we could probably get a point release out, even if it's no more than just that change.

sympy/interactive/printing.py
+ if IPython.__version__ >= '1.0':
+ from IPython.kernel.zmq.zmqshell import ZMQInteractiveShell
+ else:
+ from IPython.zmq.zmqshell import ZMQInteractiveShell

This comment has been minimized.

Show comment Hide comment
@minrk

minrk Mar 3, 2013

Contributor

I don't recommend doing this. ipython/ipython#2981 aims to eliminate the cost of registering unused display formats,
so you shouldn't need to be checking whether you are in an IPython kernel here.

IPython doesn't have an official way to check whether you are 'in a kernel' or not, but sniffing this sort of private API that is not likely to be stable is probably not the way to do it. Our goal is to avoid the need to know this at all (especially since ultimately all IPython sessions will be kernels). Since this will be the default state going forward, if you really must check, I would check for the Terminal case, and default to the Kernel behavior rather than the other way around. Checking the Terminal case is less likely to change, and misidentifying it has a lower cost (it's never wrong in previously released IPython, and there should be no cost after 1.0 release).

@minrk

minrk Mar 3, 2013

Contributor

I don't recommend doing this. ipython/ipython#2981 aims to eliminate the cost of registering unused display formats,
so you shouldn't need to be checking whether you are in an IPython kernel here.

IPython doesn't have an official way to check whether you are 'in a kernel' or not, but sniffing this sort of private API that is not likely to be stable is probably not the way to do it. Our goal is to avoid the need to know this at all (especially since ultimately all IPython sessions will be kernels). Since this will be the default state going forward, if you really must check, I would check for the Terminal case, and default to the Kernel behavior rather than the other way around. Checking the Terminal case is less likely to change, and misidentifying it has a lower cost (it's never wrong in previously released IPython, and there should be no cost after 1.0 release).

flacjacket added some commits Mar 6, 2013

Remove IPython sniffing
Removes checks on whether or not one is in the qtconsole or notebook
before setting the unicode and latex once it is verified that your in
any IPython instance.
@flacjacket

This comment has been minimized.

Show comment Hide comment
@flacjacket

flacjacket Mar 6, 2013

Member

I removed the checks that the module has been loaded. Somewhere someone commented it wasn't necessary, as IPython handles checking that. For older versions, it shouldn't matter if the module is loaded multiple times.

I got rid of the IPython stiffing to determine if it's in the qtconsole or notebook, since registering the printers shouldn't adversely affect the terminal and we aren't doing anything that could break in the future.

As for testing, it is possible by doing something like:

In [1]: from sympy import Symbol

In [2]: Symbol('pi')
Out[2]: pi

In [3]: %load_ext sympy.interactive.ipythonprinting

In [4]: Symbol('pi')
Out[4]: π

in combination with an IPython instance created with app = init_ipython_session(). However, I'm not sure how to capture output from app.run_cell(code). If I could do that, I could setup the tests.

Member

flacjacket commented Mar 6, 2013

I removed the checks that the module has been loaded. Somewhere someone commented it wasn't necessary, as IPython handles checking that. For older versions, it shouldn't matter if the module is loaded multiple times.

I got rid of the IPython stiffing to determine if it's in the qtconsole or notebook, since registering the printers shouldn't adversely affect the terminal and we aren't doing anything that could break in the future.

As for testing, it is possible by doing something like:

In [1]: from sympy import Symbol

In [2]: Symbol('pi')
Out[2]: pi

In [3]: %load_ext sympy.interactive.ipythonprinting

In [4]: Symbol('pi')
Out[4]: π

in combination with an IPython instance created with app = init_ipython_session(). However, I'm not sure how to capture output from app.run_cell(code). If I could do that, I could setup the tests.

Add tests for ipythonprinting extension
Adds tests for the loading of the printing extension for IPython. This
only tests the plain text output, this could also be used to test
whether or not latex is enabled, png's are created, etc.
@flacjacket

This comment has been minimized.

Show comment Hide comment
@flacjacket

flacjacket Mar 6, 2013

Member

Alright, I figured out a way to test the loading of the printing extension. I realized I ended up having to pull out some IPython methods that may change and break this test, but for now, it performs as advertised.

Member

flacjacket commented Mar 6, 2013

Alright, I figured out a way to test the loading of the printing extension. I realized I ended up having to pull out some IPython methods that may change and break this test, but for now, it performs as advertised.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 6, 2013

Owner

Running my bot (Travis doesn't have IPython). My Python 2.7 has the latest IPython dev and my Python 3.3 has the 0.13.1.

Owner

asmeurer commented Mar 6, 2013

Running my bot (Travis doesn't have IPython). My Python 2.7 has the latest IPython dev and my Python 3.3 has the 0.13.1.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 6, 2013

Owner

I can't test this actually, as I still haven't managed to get zeromq for the dev IPython working yet. If @minrk or anyone else from could comment, that would be great. Otherwise, I'll just take your word that it works.

Owner

asmeurer commented Mar 6, 2013

I can't test this actually, as I still haven't managed to get zeromq for the dev IPython working yet. If @minrk or anyone else from could comment, that would be great. Otherwise, I'll just take your word that it works.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 6, 2013

Owner

Hm, I thought was supposed to ping everyone. What about just @ipython.

Owner

asmeurer commented Mar 6, 2013

Hm, I thought was supposed to ping everyone. What about just @ipython.

@flacjacket

This comment has been minimized.

Show comment Hide comment
@flacjacket

flacjacket Mar 6, 2013

Member

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (af2bc5d) into master (9bc7091).
✳️ Python 2.7.3-final-0: pass
Test command: ./bin/test ipython

Member

flacjacket commented Mar 6, 2013

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (af2bc5d) into master (9bc7091).
✳️ Python 2.7.3-final-0: pass
Test command: ./bin/test ipython

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 6, 2013

Owner

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (af2bc5d) into master (9bc7091).
✳️ Python 2.7.3-final-0: pass
Test command: bin/test ipython printing && ./bin/doctest
✳️ Python 3.3.0-final-0: pass
Test command: bin/test ipython printing && ./bin/doctest

Owner

asmeurer commented Mar 6, 2013

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (af2bc5d) into master (9bc7091).
✳️ Python 2.7.3-final-0: pass
Test command: bin/test ipython printing && ./bin/doctest
✳️ Python 3.3.0-final-0: pass
Test command: bin/test ipython printing && ./bin/doctest

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 6, 2013

Owner

I take it issue 3566 is completely solved now.

Owner

asmeurer commented Mar 6, 2013

I take it issue 3566 is completely solved now.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 6, 2013

Owner

I was able to test in Python 3 (IPython 0.13.1) notebook. The rest is hosed for me. I managed to screw up my PyQT as well, so I don't have the qtconsole either. I leave it to you and others to test this.

Owner

asmeurer commented Mar 6, 2013

I was able to test in Python 3 (IPython 0.13.1) notebook. The rest is hosed for me. I managed to screw up my PyQT as well, so I don't have the qtconsole either. I leave it to you and others to test this.

@flacjacket

This comment has been minimized.

Show comment Hide comment
@flacjacket

flacjacket Mar 6, 2013

Member

This would cover 3566 fully.

It works for me in terminal, qtconsole, and notebook.

Member

flacjacket commented Mar 6, 2013

This would cover 3566 fully.

It works for me in terminal, qtconsole, and notebook.

@jrioux

This comment has been minimized.

Show comment Hide comment
@jrioux

jrioux Mar 6, 2013

Member

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (7350422) into master (6f0e757).
✳️ PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
✳️ Python 2.7.2-final-0: pass
✳️ Python 3.2.1-final-0: pass
✳️ Sphinx 1.1.3: pass
Docs build command: make clean && make html-errors && make latex && cd _build/latex && xelatex sympy-*.tex

Member

jrioux commented Mar 6, 2013

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (7350422) into master (6f0e757).
✳️ PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
✳️ Python 2.7.2-final-0: pass
✳️ Python 3.2.1-final-0: pass
✳️ Sphinx 1.1.3: pass
Docs build command: make clean && make html-errors && make latex && cd _build/latex && xelatex sympy-*.tex

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Mar 8, 2013

Owner

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (7350422) into master (a27f079).
✳️ Python 2.5.6-final-0: pass
✳️ Python 2.6.8-final-0: pass
✳️ Python 2.7.3-final-0: pass
✳️ Python 3.2.3-final-0: pass
✳️ Sphinx 1.1.3: pass

Owner

asmeurer commented Mar 8, 2013

SymPy Bot Summary: ✳️ Passed after merging flacjacket/ipython_printing_fix (7350422) into master (a27f079).
✳️ Python 2.5.6-final-0: pass
✳️ Python 2.6.8-final-0: pass
✳️ Python 2.7.3-final-0: pass
✳️ Python 3.2.3-final-0: pass
✳️ Sphinx 1.1.3: pass

@jrioux

This comment has been minimized.

Show comment Hide comment
@jrioux

jrioux Mar 8, 2013

Member

Let's put this in and get it tested.

Member

jrioux commented Mar 8, 2013

Let's put this in and get it tested.

jrioux added a commit that referenced this pull request Mar 8, 2013

Merge pull request #1853 from flacjacket/ipython_printing_fix
Fixes ipython printing extension



This fixes two things.

First, the extension had to be reloaded to import the extension. Fixes issue #3619 [1].

Second, this has been broken since IPython moved IPython.zmq to IPython.kernel.zmq. This PR fixes this. Note, there are some git versions of IPython where this will still break (between moving the zmq module and changing the version to 1.0.dev), but it should work for all stable releases and will work for the current git version. The versions that won't work are between 1/29 [2] and 2/15 [3]. This was brought up in #1757.

[1] https://code.google.com/p/sympy/issues/detail?id=3619
[2] ipython/ipython@37f3225
[3] ipython/ipython#2927

@jrioux jrioux merged commit 72bd539 into sympy:master Mar 8, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment