Opened 10 years ago
Last modified 5 years ago
#12719 closed enhancement
Upgrade to IPython 0.13 — at Version 181
Reported by:  kini  Owned by:  tbd 

Priority:  critical  Milestone:  sage5.7 
Component:  packages: standard  Keywords:  sd40.5 
Cc:  iandrus, vbraun, jason, AlexanderDreyer, robertwb  Merged in:  
Authors:  Mike Hansen, Volker Braun, Jason Grout  Reviewers:  Volker Braun, Mike Hansen, Jason Grout 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  #13459, #9191  Stopgaps: 
Description (last modified by )
We need to upgrade IPython to the shiny new thing. Well, not really  the shiny new thing was 0.11, and 0.13 is now stable. It's been mostly rewritten and is now a lot more modular, as I understand it. Definitely worth using in Sage.
Note: To use the QT console (sage ipython qtconsole
), you need qt/qtdevel in your base OS install, the optional zeromq/pyzmq spkgs from #12843, and the optional sip/PyQt spkgs from #13022.
Install the IPython 0.13.1 spkg at http://sage.math.washington.edu/home/jason/ipython0.13.1.spkg
apply:
To the scripts repository:
To the root repository:
To the sage library:
 trac_127195.1beta1.patch (or trac_12719rebasedto13211.patch if #13211 has been applied)
 trac_12719move_to_preparser_rebased.patch
 trac_12719_ipython_fixes.patch
 trac_12719_dedent.patch
 trac_12719crash.patch
 trac_12719cellmagics013.patch
 12719newipythontake2.patch
 12719displayhook.patch
 12719fixtransformer.patch
 127195.4rc1.patch
 12719removeplugin.patch
 12719displayhooklibrary.patch
 trac_12719pager.patch
 trac_12719preparsedoctest.patch
 trac_12719gcdrepr.patch
So assuming that #13211 has been applied:
cd SAGE_ROOT ./sage hg qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719_ROOT_configuration_files.patch ./sage hg qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719ipythondir013.patch ./sage hg R local/bin qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719scripts.patch ./sage hg R local/bin qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719_remove_sage_gdb_ipython.patch ./sage hg R local/bin qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/12719testdisplayhook.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719rebasedto13211.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719move_to_preparser_rebased.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719_ipython_fixes.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719_dedent.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719crash.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719cellmagics013.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/12719newipythontake2.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/12719displayhook.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/12719fixtransformer.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/127195.4rc1.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/12719removeplugin.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/12719displayhooklibrary.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719pager.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719preparsedoctest.patch ./sage hg R devel/sage qimport P http://trac.sagemath.org/sage_trac/rawattachment/ticket/12719/trac_12719gcdrepr.patch ./sage i http://sage.math.washington.edu/home/jason/ipython0.13.1.spkg ./sage br
Fixes #11223?
Change History (202)
comment:1 Changed 10 years ago by
comment:2 followup: ↓ 80 Changed 10 years ago by
Would be nice but we need some porting, here is what typically happen if you try to use ipython0.12 with sage (we had several reports in sageongentoo before I pinned down the version used by sage):
$ sage   Sage Version 4.7.2, Release Date: 20111029   Type notebook() for the GUI, and license() for information.   Traceback (most recent call last): File "/usr/bin/sageipython", line 19, in <module> from IPython.CrashHandler import CrashHandler ImportError: No module named CrashHandler
comment:3 Changed 10 years ago by
Yes, obviously :) I'm just making this ticket since it is absolutely must be an eventual goal for us and nobody else has made a ticket yet, even though IPython 0.12 has been out for about three months.
comment:4 Changed 10 years ago by
To be honest Fernando Perez and I talked about it on the mailing list at the time of ipython 0.11 and I said I would tell him what happens when we use ipython 0.11+ with sage because he is interested. For various reason I haven't been able to go back to it. But some sageongentoo users eventually got a taste of it when it finally landed in the portage tree.
So first hurdle: do something about the crash handling.
comment:5 Changed 10 years ago by
 Cc iandrus added
comment:6 Changed 10 years ago by
I'm working on this now, and have made quite a bit of progress. As an added bonus, making the Sage customizations to IPython is quite a bit cleaner. I should hopefully be able to clean out the mess in sage/misc/preparser_ipython.py and sage/misc/interpreter.py .
comment:7 Changed 10 years ago by
This will fix #4413.
comment:8 Changed 10 years ago by
Great, I'm glad you're on the case! :)
comment:9 Changed 10 years ago by
 Cc vbraun added
comment:10 Changed 10 years ago by
What about zmq/pyzmq? It would really add to the functionality and I'm interested in the parallel/cluster support. But, strictly speaking, its not necessary to run ipython. Its also used in the new notebook so maybe its time for it to become a standard package?
comment:11 Changed 10 years ago by
 Cc jason added
CCing Jason as he expressed interest on sagedevel.
comment:12 Changed 10 years ago by
mhansen: we'd love to see your workinprogress if it's available somewhere. I just spent some time reading the IPython docs again and thinking about how we could use it to implement the notebook hub and worker processes.
comment:13 Changed 10 years ago by
I've posted patches which let Sage work with IPython 0.12 as well as cleaning up some old messy code. These should be in a state where they're ready to review.
I haven't done anything with preparing an spkg yet.
comment:14 Changed 10 years ago by
 Description modified (diff)
I've updated the instructions in the description. Please correct if I messed something up.
comment:15 Changed 10 years ago by
 Status changed from new to needs_review
comment:16 Changed 10 years ago by
 Status changed from needs_review to needs_work
 Work issues set to spkg
comment:17 followup: ↓ 19 Changed 10 years ago by
Just curious: do your patches work with the current 0.12, or did you base them on the current git master (or does it not matter?). On the IPython mailing list, they mentioned that 0.12 had some big bugs, so they were thinking about cutting a quick 0.12.1. Also, they expect 0.13 in about a month.
comment:18 Changed 10 years ago by
I was thinking more about security today with the zmq messages. I think it may be more secure to have a default ipcontroller transport of IPC in a directory inside of $DOT_SAGE, rather than tcp over the loopback interface. That would be more secure in that random people on your computer wouldn't be able to try to register with the IPython hub. I emailed the IPython mailing list about it: http://thread.gmane.org/gmane.comp.python.ipython.devel/7789
What do you think?
comment:19 in reply to: ↑ 17 Changed 10 years ago by
Replying to jason:
Just curious: do your patches work with the current 0.12, or did you base them on the current git master (or does it not matter?). On the IPython mailing list, they mentioned that 0.12 had some big bugs, so they were thinking about cutting a quick 0.12.1. Also, they expect 0.13 in about a month.
I've looked at the current git master as I was making these changes, and I think these changes should work fine with what's on there. I'd be surprised if it made a difference.
comment:20 Changed 10 years ago by
I just tested things against git master and everything seems fine.
comment:21 Changed 10 years ago by
 Description modified (diff)
I'm trying to apply these to 5.0beta12. I think I probably got the order wrong in the description, so it would be great if you could check that. But regardless, none of the patches seem to apply to 5.0beta12 cleanly (each has rejects).
comment:22 Changed 10 years ago by
 Description modified (diff)
 Work issues changed from spkg to patches don't apply
I just tried applying to beta6 and the scripts patch still had problems.
Changed 10 years ago by
comment:23 Changed 10 years ago by
The scripts patch should apply fine to the scripts repository. I've rebased the other ones to work on the newer 5.0 beta's.
comment:24 Changed 10 years ago by
 Cc AlexanderDreyer added
After applying the patches to sage5.0 beta12 I still have ip = IPython.ipapi.get()
at the end of sage/misc/edit_module.py
, which crashes Sage on startup.
comment:25 Changed 10 years ago by
I get a crash when I apply these (with a latest git IPython). I've posted the crash report at http://sage.math.washington.edu/home/jason/Sage_crash_report.txt
comment:26 Changed 10 years ago by
Is the zmq communication in ipython going through tcp/localhost sockets, or through UDS sockets? I really think we should use UDS sockets with the files in $DOTSAGE so that it is more secure (i.e., random other people on the computer can't connect to the kernel).
comment:27 Changed 10 years ago by
By default we should not use zeromq. For example, if you want to fork you should do so before creating a zmq context. The zmq context creates threads and open file descriptors.
Apart from that, you should either use IPC or TCP+magic cookie, much like the X window system. Implementation wise its probably easiest to allow any socket and alway check the magic cookie.
comment:28 Changed 10 years ago by
I don't think we have a choice of whether or not to use zmq with the new ipython. ZMQ is an integral part of how it communicates results from the backend to the frontend. They do have a system for signing messages and ignoring unsigned messages (is that the magic cookie thing?). UDS sockets is IPC, so we're good there. My problem with TCP+magic cookie is that it still allows someone else to attempt to connect to the various sockets, whereas using IPC, with the socket files in $DOTSAGE, filesystem permissions prevent others from even attempting to connect.
comment:29 Changed 10 years ago by
It may be that I'm misunderstanding how ipython is using zmq when you just execute 'ipython'. It may be that it uses inprocess communication (using zmq) instead of opening ports on localhost. I've posted to the ipythondevel mailing list about this.
comment:30 Changed 10 years ago by
Given that you can build IPython without zeromq support it surely must be possible to run without?
I don't see the problem with tcp + magic cookie. Yes you can connect but your connection attempt will be ignored. And the difference in pyzmq to ipc + magic cookie is a single string. So why bother with ipc without magic cookie?
comment:31 Changed 10 years ago by
Thomas on the ipython mailing list confirmed that the plain ipython command doesn't use zmq, so that was a big misunderstanding I had about the new ipython. So never mind what I said about sockets.
Changed 10 years ago by
comment:32 Changed 10 years ago by
Okay, I fixed the problem with edit_module.
Sorry I wasn't clear about the default IPython not using zmq. There is a experimental console shell that uses two processes which can be accessed with "ipython console", but we aren't using that at all.
comment:33 Changed 10 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
 Work issues patches don't apply deleted
comment:34 Changed 10 years ago by
For the record, the patches now apply to my 5.0beta12 and Sage starts up.
comment:35 Changed 10 years ago by
When doing %edit, I edit the sage library file, but the file isn't copied over to the library after editing, so the changes don't take effect. This doesn't hold back this issue, though, since it didn't work before either.
comment:36 Changed 10 years ago by
Although zeromq is not, strictly speaking, necessary for IPython it might be of interest that I made zeromq and pyzmq spkgs at #12843
comment:37 Changed 9 years ago by
 Description modified (diff)
comment:38 Changed 9 years ago by
Not working on sage5.0.rc0 (system Darwin 10.8.0). I installed the spkg and applied the patches in the given order and places, but still have the following error lauching Sage :
Traceback (most recent call last): File "/Users/pece/sage5.0.rc0/local/bin/sageipython", line 19, in <module> from IPython.CrashHandler import CrashHandler ImportError: No module named CrashHandler
I even installed the patches from #12843 without any success.
comment:39 followup: ↓ 41 Changed 9 years ago by
I have OSX 10.6.8: Darwin Kernel Version 10.8.0
. I'm running the IPython git master and the patches, and it seems to work well. Did you do sage b
to compile the patches? Maybe the problem is in the IPython 0.12.1>0.13 changes.
comment:40 Changed 9 years ago by
Hmmm, it looks like the scripts/ patch wasn't applied (the problematic line is deleted in the patch). Can you check again that trac_12719scripts.patch is applied to the SAGE_ROOT/local/bin repository?
comment:41 in reply to: ↑ 39 ; followup: ↓ 125 Changed 9 years ago by
Replying to jason:
Maybe the problem is in the IPython 0.12.1>0.13 changes.
On sage5.0.rc0, I managed to apply the three patches and sage starts correctly. So I guess it's not a matter of IPython 0.13 changes. Although, the sage prompt is not in color anymore (my file .sage/ipython/ipythonrc is set to dark background color).
Is the ipython notebook suppose to work? How should I test it?
Also, doing sage ipython notebook
creates this error :
$ sage ipython notebook Traceback (most recent call last): ... ImportError: The IPython Notebook requires tornado >= 2.1.0
comment:42 Changed 9 years ago by
The first patch (trac_12719.patch
) applies only with fuzz 2 on sage5.0
comment:43 Changed 9 years ago by
 Keywords sd40.5 added
 Status changed from needs_review to needs_work
Lots of doctests fail because the display hook is not correctly set up. I'll have a look into this if nobody beats me to it...
comment:44 Changed 9 years ago by
 Description modified (diff)
The new ipython configuration files are once more incompatible with the previous ones. My patch versions the directory now (that is, it is named .sage/ipythonVERSION) and deletes the SAGE_ROOT/ipythonrc stuff. I think all sagespecific configuration should go into sage/misc/interface.py
comment:45 Changed 9 years ago by
Here is some reasonable documentation about Prefilter vs. InputSplitter: http://mail.scipy.org/pipermail/ipythondev/2011March/007334.html
comment:46 Changed 9 years ago by
 Description modified (diff)
 Reviewers set to Volker Braun
 Status changed from needs_work to needs_review
The trac_12719_ipython_fixes.patch switches form Prefilter to InputSplitter.
I'm giving a positive review to everything that Mike wrote.
comment:47 Changed 9 years ago by
With the new patch sage/misc/interpreter.py
has 100% coverage.
comment:49 followup: ↓ 75 Changed 9 years ago by
In Sage before this patch, the sage notebook, the Python command line, etc., we have:
for i in range(10): i
outputs the numbers up to 10.
With the new Ipython spkg, etc., we don't.
sage: for i in range(10): i sage: Exiting Sage (CPU time 0m0.07s, Wall time 2m3.22s). blastoff:sage wstein$ sage ipython Python 2.7.2 (default, May 17 2012, 13:12:30) In [1]: for i in range(10): i In [2]:
I'm disappointed, especially because I thought this through and in sagews I recently wrote code so this works correctly in a single cell, even if there are several instances of this. It's a matter of passing the 'single' option to exec, as is documented in the Python reference. I wonder why they screwed this up in Ipython 0.12...?
Anyway, I'm not saying this is a show stopper at all  I want the new ipython in sage. I'm just a little concerned that literally the first thing I tried shows that they made what I consider a bad design choice. Hopefully it was just bad luck.
comment:50 Changed 9 years ago by
Cool  I tried out a bunch of other things including the new notebook (sage ipython notebook) and didn't run into any serious issues. So I'm happy with this from from a purely functional perspective. Also, deleting tons of ugly code (done in the patches), looks great.
comment:51 Changed 9 years ago by
It should be easy to get to get the correct functionality for for i in range(10): i
. The shell.run_ast_nodes
method just has to be called with interactivity="all"
instead of interactivity="last_expr"
.
comment:52 Changed 9 years ago by
Here's a review of Volker's ipython_fixes patch:
 (minor) Remove \ on line 1071
 Do one of the following:
 Fix/deprecate/remove sagegdbipython since it depends on SAGE_CLEAN
 Just throw a deprecation warning with SAGE_CLEAN
 Turn autoindent back on.
 Why add debug=True and confirm_exit=False to default config.
 Load the startup file: line 1151
comment:53 Changed 9 years ago by
 Description modified (diff)
 Reviewers changed from Volker Braun to Volker Braun, Mike Hansen
1, 3, 5: done
4: I removed the debug, but I (and most others, I asked) definitely want to keep confirm_exit=False
. Yes I really want to quit, geez!
2: I'm in favor of a) and removed it.
I've also renamed trac_13013_ROOT_configuration_files.patch (wrong ticket number) and added a patch to autodedent and eval in loops. Now the following works:
sage: for i in range(3): i 0 1 2
comment:54 Changed 9 years ago by
 Description modified (diff)
 Status changed from needs_review to positive_review
Volker's changes look good to me.
comment:55 Changed 9 years ago by
 Description modified (diff)
Changed 9 years ago by
comment:56 Changed 9 years ago by
Mike's addition of trac_12719crash.patch looks *great* to me as an addition.
comment:57 Changed 9 years ago by
 Status changed from positive_review to needs_work
Unfortunately, this doesn't apply to sage5.1.beta1:
applying trac_12719.patch patching file doc/en/reference/misc.rst Hunk #1 succeeded at 30 with fuzz 2 (offset 2 lines). patching file sage/misc/interpreter.py Hunk #1 FAILED at 0 Hunk #3 FAILED at 271 Hunk #4 FAILED at 385 Hunk #5 FAILED at 449 4 out of 5 hunks FAILED  saving rejects to file sage/misc/interpreter.py.rej patch failed, unable to continue (try v) patch failed, rejects left in working dir errors during apply, please fix and refresh trac_12719.patch
comment:58 Changed 9 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
I've rebased this on top of #11817.
comment:59 Changed 9 years ago by
 Description modified (diff)
comment:60 Changed 9 years ago by
 Status changed from needs_review to needs_work
Now it applies but I get documentation errors:
dochtml.log:/padic/scratch/jdemeyer/merger/sage5.1.beta3/local/lib/python2.7/sitepackages/sage/misc/interpreter.py:docstring of sage.misc.interpreter:12: WARNING: Block quote ends without a blank line; unexpected unindent. dochtml.log:/padic/scratch/jdemeyer/merger/sage5.1.beta3/local/lib/python2.7/sitepackages/sage/misc/interpreter.py:docstring of sage.misc.interpreter:8: WARNING: Block quote ends without a blank line; unexpected unindent. dochtml.log:/padic/scratch/jdemeyer/merger/sage5.1.beta3/local/lib/python2.7/sitepackages/sage/misc/interpreter.py:docstring of sage.misc.interpreter:8: WARNING: Block quote ends without a blank line; unexpected unindent. dochtml.log:/padic/scratch/jdemeyer/merger/sage5.1.beta3/local/lib/python2.7/sitepackages/sage/misc/interpreter.py:docstring of sage.misc.interpreter:11: WARNING: Definition list ends without a blank line; unexpected unindent. dochtml.log:/padic/scratch/jdemeyer/merger/sage5.1.beta3/local/lib/python2.7/sitepackages/sage/misc/interpreter.py:docstring of sage.misc.interpreter:13: ERROR: Unexpected indentation.
comment:61 Changed 9 years ago by
Also, trac_12719scripts.patch needs a commit message.
comment:62 Changed 9 years ago by
It seems that double question mark, like trace??
, doesn't bring up the source code anymore, but just brings up the docstring. It seems that trac_12719_ipython_fixes.patch broke this (double question marks work with just the first two patches, but not after that third patch).
comment:63 Changed 9 years ago by
 Description modified (diff)
comment:64 Changed 9 years ago by
 Description modified (diff)
Add two more patches to get the new IPython 0.13beta1 spkg above to work.
comment:65 Changed 9 years ago by
 Description modified (diff)
comment:66 Changed 9 years ago by
 Description modified (diff)
comment:67 Changed 9 years ago by
 Work issues set to ?? doesn't work
comment:68 Changed 9 years ago by
 Work issues changed from ?? doesn't work to ?? doesn't show code
comment:69 Changed 9 years ago by
 Description modified (diff)
comment:70 Changed 9 years ago by
 Description modified (diff)
comment:71 Changed 9 years ago by
I rebased trac_12719_ROOT_configuration_files.patch to sage 5.1beta6
comment:72 Changed 9 years ago by
 Description modified (diff)
 Reviewers changed from Volker Braun, Mike Hansen to Volker Braun, Mike Hansen, Jason Grout
 Summary changed from Upgrade to IPython 0.12 to Upgrade to IPython 0.13
comment:73 Changed 9 years ago by
I upgraded the spkg to 0.13, which was just released today.
comment:74 Changed 9 years ago by
 Description modified (diff)
comment:75 in reply to: ↑ 49 Changed 9 years ago by
Replying to was:
In Sage before this patch, the sage notebook, the Python command line, etc., we have:
for i in range(10): ioutputs the numbers up to 10.
With the new Ipython spkg, etc., we don't.
sage: for i in range(10): i sage: Exiting Sage (CPU time 0m0.07s, Wall time 2m3.22s). blastoff:sage wstein$ sage ipython Python 2.7.2 (default, May 17 2012, 13:12:30) In [1]: for i in range(10): i In [2]:I'm disappointed, especially because I thought this through and in sagews I recently wrote code so this works correctly in a single cell, even if there are several instances of this. It's a matter of passing the 'single' option to exec, as is documented in the Python reference. I wonder why they screwed this up in Ipython 0.12...?
Anyway, I'm not saying this is a show stopper at all  I want the new ipython in sage. I'm just a little concerned that literally the first thing I tried shows that they made what I consider a bad design choice. Hopefully it was just bad luck.
Fernando Perez responded:
BTW, regarding William's comment, that was not a mistake or omission: it was a very deliberate choice we made after lots of thought. Given that we manage numbered prompts and history, and that matplotlib produces so many plots as side effects, after much experimentation we found it worked much better to only execute with 'interactive' mode the last block.
Sage doesn't distinguish stdout from python sys.displayhook, so the opposite behavior may be more desirable, but we actually consider that a 'bad design choice' of sage, because it loses an important distinction and the cache management of output history.
So in summary: our choice is actually sensible and consistent given IPython's internal design :)
comment:76 followup: ↓ 78 Changed 9 years ago by
Fernando: I looked in the IPython source, and it seems that IPython is a little more subtle than mentioned above. By default, it only executes the last block as interactive (compile in 'single' mode) if it is an expression. In Sage, we (by default) execute the last block always in 'single' mode. I think that is the difference here.
Given that (and if I understood correctly), could you explain again your reasoning for choosing to not compile in 'single' mode if it isn't an expression?
Note to self for future reference: for i in range(10): i
invokes sys.displayhook 10 times to display i if compiled in 'single' mode.
comment:77 Changed 9 years ago by
Also, note to William: it is an easy option to set to have IPython *always* execute the last block in interactive mode. It's just not the default.
comment:78 in reply to: ↑ 76 Changed 9 years ago by
Replying to jason:
Fernando: I looked in the IPython source, and it seems that IPython is a little more subtle than mentioned above. By default, it only executes the last block as interactive (compile in 'single' mode) if it is an expression. In Sage, we (by default) execute the last block always in 'single' mode. I think that is the difference here.
Given that (and if I understood correctly), could you explain again your reasoning for choosing to not compile in 'single' mode if it isn't an expression?
And here is Fernando's response:
Precisely so that if the last block has a loop, it doesn't produce multiple Out [NN]
cells. We want to keep a onetoone mapping between a cell and its output. For example it's quite common to write code like:
for i in x: plot(x[i], label='blah_i')
If we compile that as 'single', we'd get all the Matplotlib line collection results piling up at the bottom, one for each plot call.
We played a *lot* with various options, and settled on this behavior after much experimentation. After 2 years of using it, I remain convinced it's the right solution *for us*. Not to say that Sage can't make a different choice that works better for you guys, of course!
comment:79 Changed 9 years ago by
To change this, we just need to set ast_node_interactivity
to "last"
in the InteractiveShell.
comment:80 in reply to: ↑ 2 Changed 9 years ago by
Replying to fbissey:
Would be nice but we need some porting, here is what typically happen if you try to use ipython0.12 with sage:
$ sage   Sage Version 4.7.2, Release Date: 20111029   Type notebook() for the GUI, and license() for information.   Traceback (most recent call last): File "/usr/bin/sageipython", line 19, in <module> from IPython.CrashHandler import CrashHandler ImportError: No module named CrashHandler
Hi fbissey, sorry for hijacking this a bit, but i had exactly the same problem over and over again. What I just found out (and I think it really helps) is to temporarily disable the paths containing ".local" in sys.path in the sageipython file. The reason is, that ipython seems to import the "site package" (disable it for pure python via "S"), because that's where my ipython 0.13 is.
So, by adding this to sageipython you I have both at the same time:
~~~ after import sys oldpath = sys.path sys.path = filter(lambda s : '.local' not in s, sys.path) import IPython sys.path = oldpath
comment:81 Changed 9 years ago by
 Description modified (diff)
comment:82 Changed 9 years ago by
I rebased the only rejected patch for 5.2beta1
comment:83 Changed 9 years ago by
My 2 cents, after I forced myself to use this for the last few weeks on my laptop. I have found the new ipython to be pretty annoying. The cut and paste behavior and lack of autoindent at the command line is pretty painful. The thing that made me finally give up and revert to the old 0.10 ipython was when I hit controlc at the wrong moment when starting Sage, and somehow the ipython history sqlite database got in a weird locked state. Nothing I could think to do (including deleting $HOME/.sage/ipython) got this unstuck. Maybe rebooting would have worked; I don't know. I'm concerned about how stable and welltested in the wild the new ipython is. Proceed with caution.
comment:84 Changed 9 years ago by
Working cut&past and working autoindent are contradictory goals. The terminal just sees a stream of characters, you either inject indentation or not. AFAIR I turned off autoindent, we could turn that back on. Breaks pasting of indented text, of course.
The ipython dir is now versioned, you should have deleted $DOT_SAGE/ipython0.12
/ $DOT_SAGE/ipython0.13
comment:85 Changed 9 years ago by
PS: Maybe you can upload your broken ipython dir somewhere, this would be a good testcase to catch the sqlite error...
comment:86 followup: ↓ 87 Changed 9 years ago by
IMO, leave autoindent on, and use %paste
or %cpaste
when you want to paste. %paste
tries to automatically get the contents of your clipboard. If that's not working for whatever reason, %cpaste
creates a prompt where you can paste text verbatim, which IPython doesn't read until you end input with Ctrl+D or a line containing only the text "".
comment:87 in reply to: ↑ 86 ; followup: ↓ 88 Changed 9 years ago by
Replying to kini:
IMO, leave autoindent on, and use
%paste
or%cpaste
when you want to paste.
For the record, thats not how I use the commandline. I paste code all the time, but I almost never enter multiline statements manually. So I'd rather have autoindent off by default (and no need for %cpaste
), and use %autoindent
if I want it on. Though thats of course a matter of personal preferences, and I'm happy with whatever the majority prefers...
comment:88 in reply to: ↑ 87 Changed 9 years ago by
Replying to vbraun:
Replying to kini:
IMO, leave autoindent on, and use
%paste
or%cpaste
when you want to paste.For the record, thats not how I use the commandline. I paste code all the time, but I almost never enter multiline statements manually. So I'd rather have autoindent off by default (and no need for
%cpaste
), and use%autoindent
if I want it on. Though thats of course a matter of personal preferences, and I'm happy with whatever the majority prefers...
At least it is configurable, right? I think it is best not to change the default from what it is already in all previous versions of Sage, whether or not that is what the majority prefers (unless it is a huge majority). I'm not fan of changing UI's on people unless it is absolutely necessary.
comment:89 Changed 9 years ago by
Well IPythons defaults changed, we didn't just change stuff for the fun of it. For starters, vanilla IPython now disallows any superfluous indentation, i.e.
In [1]: 1 Out[1]: 1 In [2]: 1 IndentationError: unexpected indent (<ipythoninput20066badbf200>, line 1)
comment:90 Changed 9 years ago by
Two more issues:
 when Sage starts, any lines in my init.sage file are echoed to the screen after the first "sage:" prompt. They aren't executed until I hit the return key:
  Sage Version 5.2, Release Date: 20120725   Type "notebook()" for the browserbased notebook interface.   Type "help()" for help.   sage: # Here is the beginning of init.sage. 2+2 # Here is another comment a = 5 # Here is the end of init.sage.
At this point, hitting <RETURN> inserts a 4, a newline, and the "sage:" prompt, anda
is defined to be 5.
 when I evaluate or
plot
a graphics object, two plot windows are opened:sage: P = plot(sin(x), (x, 0, 10)) sage: P # opens up two windows showing the same plot. Note also the blank line. sage: plot(P) # also opens up two windows. sage: show(P) # opens up only one window.
comment:91 Changed 9 years ago by
Regarding indentation, autoindent being on breaks sage inside Emacs (dumb terminal). So I think it should be off, at least in a dumb terminal, until/unless I can figure out how to fix this in Emacs. I haven't really looked into it at all, so it may be easy to fix inside Emacs by setting some environment variables or something. See #10504.
comment:92 Changed 9 years ago by
+1 for autoindent being on by default. That helps what is probably the vast majority of the casesinteractively typing into the command line.
comment:93 Changed 9 years ago by
I'm having a weird problem when I use IPython.parallel in sage. Here's what happens:
from IPython.parallel import Client rc = Client('/home/seshu/.sage/ipythonnew/profile_default/security/ipcontrollerclient.json') dview = rc[:] dview.map_sync(lambda x: x**10, range(32)) [0:apply]:  NameError Traceback (most recent call last)<string> in <module>() <ipythoninput4ad199b8d76dc> in <lambda>(x) NameError: global name 'Integer' is not defined
If I do anything that doesn't involve integers like dview.map_sync(lambda x: "hi", range(32)), it works fine. Sorry if this isn't the appropriate place to ask these questions. I wasn't sure where to ask since this is a bit bleeding edge. I would like to be able to use IPython.parallel from the sage notebook if possible.
comment:94 Changed 9 years ago by
The ipython engines need to import the sage modules. The fix is dview.execute("from sage.all import *"). Also I did sage sh ipcluster start.
comment:95 Changed 9 years ago by
Just FYI, for the *next* version (so not an issue on this ticket), https://github.com/ipython/ipython/pull/2299 broke Sage, I think by getting rid of PrefilterTransformer? tricks.
comment:96 Changed 9 years ago by
So I think that the only issue with this ticket is turning back on autoindentation and getting ?? to show the source code? Any other issues?
I think the parallel issues above should be moved to another ticket.
comment:97 Changed 9 years ago by
Note: ?? shows the source code fine, it seems, in the notebook (at least there doesn't seem to be a regression; ?? shows source when doing tab completion, but not on evaluating, just like before the python upgrade). It is a regression on the command line, though.
comment:98 Changed 9 years ago by
There are the issues I mentioned above. Can anyone else reproduce them?
comment:99 Changed 9 years ago by
The parallel issues only seem to effect the notebook. I've successfully gotten ipython.parallel and sage to run on a small 24 core amazon ec2 cluster, using ssh with the ipython 0.13.1 branch on github. According to the ipython guys, the ssh launcher in 0.13 is broken. I kinda have some idea of whats going with the parallel issues and the notebook as well, but i'm not sure how to fix it. I can write something for how to setup sage with ipython engines started over ssh if this package gets updated to 0.13.1.
comment:100 Changed 9 years ago by
Well, hopefully this gets merged before 0.13.1 comes out.
comment:101 Changed 9 years ago by
jhpalmieri, I can confirm both of the issues that you saw. I'm seeing them too.
comment:102 Changed 9 years ago by
 Work issues changed from ?? doesn't show code to ?? doesn't show code, init.sage displayed, double plot windows, autoindent disabled
comment:103 Changed 9 years ago by
 Work issues changed from ?? doesn't show code, init.sage displayed, double plot windows, autoindent disabled to ?? doesn't show code, init.sage displayed, double plot windows
actually, it looks like autoindent was turned back on: http://trac.sagemath.org/sage_trac/ticket/12719#comment:52
comment:104 Changed 9 years ago by
Does anyone know where in the code init.sage is run? I've searched the entire codebase for "init.sage" and I'm not finding anything that looks like it is responsible for running the file.
comment:105 Changed 9 years ago by
Ah, I found it. It's the last line in sage/misc/interpreter.py, the one that says self.shell.run_cell('%%load "%s"'%startup_file)
comment:106 Changed 9 years ago by
Okay, it looks like the problem is that we punt to the ipython load, which *pastes* the content so you can edit it first: http://ipython.org/ipythondoc/dev/interactive/qtconsole.html#load
comment:107 Changed 9 years ago by
So our conclusion on https://github.com/ipython/ipython/pull/2299 was that Sage should make %load an alias for IPython's %run, and beef up IPython's %run to be able to handle remote files. In fact, make IPython's %run use the same simple mechanism as IPython's %load would be great (though may not be possible%run uses filespecific features in python and execfile, etc.)
comment:108 Changed 9 years ago by
Is there a reason we didn't use a profile config file for all of our customizations?
I've started a different approach to the customizations we use here: https://github.com/jasongrout/sageipythonprofile
put these into the .sage/ipythonnew directory and then start with sage ipython profile=sage
comment:109 Changed 9 years ago by
 Description modified (diff)
Changed 9 years ago by
comment:110 Changed 9 years ago by
Okay, I think my "take2" patch significantly cleans things up and makes things more modular and more "IPythonic".
In particular, this seems to work nicely: do sage ipython notebook
to launch the ipython notebook, then evaluate %load_ext sage.misc.sage_extension
. All preparsing is turned on, etc. It's great!
There should probably be a bit more documentation in the sage_extension file, and full doctests should be run. But I think it's ready for other people to hammer on it too.
Here is one issue: something broke in sage startuptime
. I'm not sure it's IPython's fault, though.
Oh, and I was working from IPython master, not IPython 0.13. I hope that doesn't put a wrench in the works...
comment:111 Changed 9 years ago by
Preliminary tests seem to indicate that these patches and the new take work just fine with IPython 0.13 (at least when I revert my IPython git repository). So it's still probably ready for more people to hammer on this.
comment:112 Changed 9 years ago by
 Work issues changed from ?? doesn't show code, init.sage displayed, double plot windows to ?? doesn't show code
My take2 patch seems to have solved the double plot window problem. It also solved the init.sage/load problems, as well as the ?? problem. So that was all of the issues listed...
comment:113 Changed 9 years ago by
 Work issues ?? doesn't show code deleted
Changed 9 years ago by
comment:114 Changed 9 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
Since we now hook into the IPython display hook, we can delete our special display hook.
comment:115 Changed 9 years ago by
 Description modified (diff)
Fixed the startup issue with new patch to scripts repository
comment:116 Changed 9 years ago by
 Description modified (diff)
Changed 9 years ago by
comment:117 Changed 9 years ago by
 Description modified (diff)
Fix a transforming issue: see http://mail.scipy.org/pipermail/ipythondev/2012September/010329.html or the commit message.
comment:118 Changed 9 years ago by
 Dependencies set to #13459
 Status changed from needs_review to needs_work
This needs to be rebased to sage5.4.rc0, in particular #13459.
comment:119 Changed 9 years ago by
Also, #13361 includes the change in the startuptime.py patch, so we can delete it.
comment:120 Changed 9 years ago by
 Description modified (diff)
comment:121 Changed 9 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
Add one more patch so we don't change directories anymore.
comment:122 followup: ↓ 127 Changed 9 years ago by
Something that seems to work fine now is starting up sage in the devel/sage directory. So it seems that the issue discussed in #13361 is fixed in this ipython upgrade.
Changed 9 years ago by
comment:123 Changed 9 years ago by
 Description modified (diff)
comment:124 Changed 9 years ago by
Okay, I think this may be ready for review again.
It's a big enough change that maybe it would be good to shoot for one of the first patches in the 5.5 release, so it gets plenty of testing???
comment:125 in reply to: ↑ 41 Changed 9 years ago by
Replying to slabbe:
Also, doing
sage ipython notebook
creates this error :$ sage ipython notebook Traceback (most recent call last): ... ImportError: The IPython Notebook requires tornado >= 2.1.0
I get this too. Should I just install tornado somehow, or should this be fixed as part of this ticket?
Edit: after installing tornado (with sage sh
and easy_install tornado
), it said I had to install pyzmq. So I did that, and now the notebook runs. It looks very nice, and feels pretty snappy, maybe snappier than the Sage notebook.
comment:126 Changed 9 years ago by
Yes, tornado and zmq (pyzmq) are required for the ipython notebook. That's not part of this ticket.
And yes, the ipython notebook is very nice. After loading it up, try doing
%load_ext sage.misc.sage_extension
to get some sage goodness, like the preparser, etc.!
comment:127 in reply to: ↑ 122 ; followup: ↓ 128 Changed 9 years ago by
comment:128 in reply to: ↑ 127 Changed 9 years ago by
Replying to jdemeyer:
Replying to jason:
Something that seems to work fine now is starting up sage in the devel/sage directory.
FYI: this was fixed by #13459.
But I reverted the "import sage" that #13459 does in trac_12719scripts.patch, and it still works fine for me (running sage in devel/sage and then import sage
actually imports the sitepackages sage). Maybe IPython changed its handling of sys.path. What I'm saying is that we should test that my reverting of part of #13459 (the part where we imported sage in sageipython) actually does what I think it does.
comment:129 Changed 9 years ago by
I'm getting doctest failures because of how lists of matrices are printed. For example, with matrix2.pyx:
File ".../sage5.4.rc1/devel/sage/sage/matrix/matrix2.pyx", line 11627: sage: (d, u, v) Expected: ( [ 1 0] [ 1 0] [ 1 w] [ 0 w + 9], [w 1], [ 0 1] ) Got: ([ 1 0] [ 0 w + 9], [ 1 0] [w 1], [ 1 w] [ 0 1])
That file has 7 failures altogether. Is this because you turned off Sage's displayhook?
comment:130 Changed 9 years ago by
Interesting. I thought I used Sage's displayhook (I know I was at one time anyway), but I've also noticed the same problems on my install with printing tuples of matrices and it's been frustrating to me. I'll take a look at it.
comment:131 Changed 9 years ago by
12719displayhook.patch looks relevant, in particular removing the lines
import sage.misc.displayhook sage.misc.displayhook.install()
and removing the corresponding functions from sage/misc/displayhook.py. But I've never worked with this aspect of Sage before.
comment:132 Changed 9 years ago by
When I use either time
or %time
, the rest of the line doesn't seem to be preparsed: try
sage: srange(1/10, 1/2, 1/10) [1/10, 1/5, 3/10, 2/5]
vs.
sage: %time srange(1/10, 1/2, 1/10)  ValueError Traceback (most recent call last) ... ValueError: srange() step argument must not be zero
Or even
sage: %time 1/10 CPU times: user 0.00 s, sys: 0.00 s, total: 0.00 s Wall time: 0.00 s 0 < note the answer!
comment:133 Changed 9 years ago by
The displayhook is activated when Sage is run (try running those matrix doctests from the command line). The problem is that the doctesting framework does *not* use the interactive version of Sage; it seems to use straight python. So the display hook is not set.
It seems that the display hook used to be set when just importing Sage as a library, maybe? If so, I think that's the wrong approach. Importing a library shouldn't obliterate the display hook.
So it sounds like we need to make the doctesting framework load the right Sage things, like the preparser, the display hook, etc.
comment:134 Changed 9 years ago by
I'll take a look at %time. That indeed seems to be a problem where the preparser isn't being run.
comment:135 Changed 9 years ago by
 Description modified (diff)
For some reason, the preparser explicitly turned itself off for magic lines. I'm not sure why but I'm posting a patch to fix that.
Changed 9 years ago by
comment:136 Changed 9 years ago by
 Description modified (diff)
comment:137 Changed 9 years ago by
John, you know the doctest framework a lot better than I do. If you start up sageipython and load the sage extension, it will install the display hook, import Sage, etc. John, do you know how to get the doctest runner to execute this as part of ipython: get_ipython().extension_manager.load_extension('sage.misc.sage_extension')
grout@tiny:~/sage% sage ipython ic "get_ipython().extension_manager.load_extension('sage.misc.sage_extension')" E:128 Python 2.7.3 (default, Oct 9 2012, 11:53:24) Type "copyright", "credits" or "license" for more information. IPython 0.13  An enhanced Interactive Python. ? > Introduction and overview of IPython's features. %quickref > Quick reference. help > Python's own help system. object? > Details about 'object', use 'object??' for extra details. In [1]: (identity_matrix(3), identity_matrix(5)) Out[1]: ( [1 0 0 0 0] [0 1 0 0 0] [1 0 0] [0 0 1 0 0] [0 1 0] [0 0 0 1 0] [0 0 1], [0 0 0 0 1] )
comment:138 Changed 9 years ago by
I guess one reason to turn off the preparser for magic lines is so that in a line like
%ed file1.py
the "1" doesn't get preparsed. I don't know how the old "time" command was written; did it just call "%time"? If so, maybe that was to allow lines starting with "time" to be preparsed.
For doctesting, I don't know if IPython is used at all during doctesting. If it's possible to turn on this extension using pure Python, we can add it at the top of each file as it's doctested, with a patch like this to local/bin/sagedoctest:

sagedoctest
diff git a/sagedoctest b/sagedoctest
a b def extract_doc(file_name, library_code= 490 490 491 491 """ % os.path.join(os.getcwd(), file_name) 492 492 s += "from sage.all_cmdline import *; \n" 493 s += "ADD NEW CODE HERE\n" 493 494 s += "import sage.plot.plot; sage.plot.plot.DOCTEST_MODE=True\n" # turn off image popup 494 495 s += r""" 495 496 def warning_function(f):
Changed 9 years ago by
comment:139 Changed 9 years ago by
 Description modified (diff)
Updated instructions for new display hook patch that sets the display hook when doctesting.
comment:140 Changed 9 years ago by
For the %time command, I filed an issue with IPython a while ago: https://github.com/ipython/ipython/pull/2403
For now, it appears like we'll have to ship that patch ourselves.
comment:141 Changed 9 years ago by
 Work issues set to ship https://github.com/ipython/ipython/pull/2403
comment:142 Changed 9 years ago by
The matrixprinting seems to work now, but I'm seeing a few other doctest failures:
sage t long force_lib devel/sage/sage/interfaces/sage0.py ********************************************************************** File "/scratch/palmieri/12719/sage5.4.rc1/devel/sagemain/sage/interfaces/sage0.py", line 489: sage: sage0(4).gcd Expected: <builtin method gcd of sage.rings.integer.Integer object at 0x...> Got: <function gcd> **********************************************************************
and several failures in sage_extension.py, which would take up too much room to include here.
comment:143 Changed 9 years ago by
I think the errors in sage_extension.py
are because file names in %runfile foo
are being preparsed: for example, the first error message is pretty long, but it ends with
IOError: did not find file '/home/palmieri/.sage/temp/sage.math.washington.edu/Integer(21508)/dir_0/run_cell.py' to load or attach
Note that Integer(21508)
in the file name; this should presumably just be 21508
. This preparsing doesn't happen if the number is mixed in with letters, but for files or directories with purely numeric names, it seems to pop up. So from the command line:
sage: %runfile hello2.py # works sage: %runfile 123/hello2.py # fails sage: %runfile /home/palmieri/123/hello2.py # fails
The same thing happens with %attach
, and I assume also with the other magic functions which take file names as arguments.
comment:144 Changed 9 years ago by
I don't understand the sage0.py
failure. Does the new IPython have a different default print representation for some objects? Before the patches here:
sage: a = 4 sage: a.gcd <builtin method gcd of sage.rings.integer.Integer object at 0x480e330> sage: a.gcd? Type: builtin_function_or_method Base Class: <type 'builtin_function_or_method'> String Form: <builtin method gcd of sage.rings.integer.Integer object at 0x480e330> Namespace: Interactive Definition: a.gcd(self, n) Docstring: ...
With the patches:
sage: a = 4 sage: a.gcd <function gcd> sage: a.gcd? Type: builtin_function_or_method String Form:<builtin method gcd of sage.rings.integer.Integer object at 0x5b7c5a0> Definition: a.gcd(self, n) Docstring: ...
With the old version, the print rep for a.gcd
is the same as what's labeled the "String Form" when you do a.gcd?
. With the new version, the print rep has changed but the "String Form" has not. I'm also confused by this behavior (with the patches):
sage: a = 4 sage: G = a.gcd sage: G.__repr__() '<builtin method gcd of sage.rings.integer.Integer object at 0x4b685a0>' sage: G <function gcd> sage: str(G) '<builtin method gcd of sage.rings.integer.Integer object at 0x4b685a0>'
Why isn't G
printed using its __repr__
method?
I've tried out some other objects (e.g., matrices and elements of the Steenrod algebra) and my guess is that the print representations for methods defined in pyx files are getting changed to <function ...>
, while those defined in py files haven't changed. Any ideas why? Looking at ipython0.10.2.p1/src/IPython/UserConfig/ipythonrc
, I see the configuration option object_info_string_level 0
, which looks relevant, but we're not using this style of configuration, right?
comment:145 Changed 9 years ago by
Just a heads up that iPython 0.13.1 is set to be released on Friday. It fixes all the bugs I was having with ipython.parallel.
Changed 9 years ago by
comment:146 Changed 9 years ago by
 Description modified (diff)
We need another patch, since the pager in IPython has moved. One instance of this was fixed in one of the patches, but we need to fix others.
Note the change to sagedoc.py; this needs to be tested.
comment:147 Changed 9 years ago by
It seems like you caught all of the instances of the IPython pager:
grout@tiny:~/sage/devel/sage/sage% ack genutils * interfaces/gap3.py 493: import IPython.genutils 494: IPython.genutils.page(helptext) misc/pager.py 27: import IPython.genutils 28: return IPython.genutils.page misc/sagedoc.py 833: from IPython.genutils import page
comment:148 Changed 9 years ago by
 Description modified (diff)
comment:149 Changed 9 years ago by
Positive review to the sagedoc.py changesearch_src didn't work before, but it does work after the change.
comment:150 Changed 9 years ago by
I uploaded an IPython 0.13.1 spkg, updating to the latest stable. (the old 0.13 spkg is still in my home directory too).
Changed 9 years ago by
comment:151 Changed 9 years ago by
 Description modified (diff)
trac_12719move_to_preparser.patch doesn't apply cleanly to 5.4.rc2. Here's a rebased version.
comment:152 followup: ↓ 157 Changed 9 years ago by
 Description modified (diff)
jhpalmieri: I uploaded a new IPython 0.13.1 spkg that applies https://github.com/ipython/ipython/pull/2403. This means we can delete the 12719preparsemagic.patch patch, which will solve the %runfile problems, but keep the preparsing for %time, %timeit, and %prun.
I've updated the instructions.
comment:153 Changed 9 years ago by
Whew, with all these patchesI can't wait until we move to using git (or even mercurial properly) and we have branches for this sort of thing.
comment:154 followup: ↓ 160 Changed 9 years ago by
The cython function printing issue appears to boil down to those functions being treated as builtin functions, so this line applies: https://github.com/ipython/ipython/blob/master/IPython/lib/pretty.py#L669, rather than this line: https://github.com/ipython/ipython/blob/master/IPython/lib/pretty.py#L671
Suggestions for how to fix this? How do we distinguish between builtin functions and Cython methods?
comment:155 Changed 9 years ago by
I've posted to the IPython list about the Cython method printing: http://mail.scipy.org/pipermail/ipythondev/2012October/010503.html
comment:156 Changed 9 years ago by
 Work issues changed from ship https://github.com/ipython/ipython/pull/2403 to Cython method printing
Are there any other outstanding issues other than the Cython method printing?
comment:157 in reply to: ↑ 152 Changed 9 years ago by
Replying to jason:
jhpalmieri: I uploaded a new IPython 0.13.1 spkg that applies https://github.com/ipython/ipython/pull/2403. This means we can delete the 12719preparsemagic.patch patch, which will solve the %runfile problems, but keep the preparsing for %time, %timeit, and %prun.
Can you think of a way to doctest this? (I haven't tried anything at all, so it might be easy.)
comment:158 Changed 9 years ago by
If just putting in a magic %timeit ...
doesn't work, then this should work *if* the doctesting framework is using IPython:
sage: get_ipython().run_line_magic('time', '594.factor()') CPU times: user 0.00 s, sys: 0.00 s, total: 0.00 s Wall time: 0.00 s 2 * 3^3 * 11
comment:159 Changed 9 years ago by
 Description modified (diff)
Hey, let's add another patch to the pile!
Changed 9 years ago by
comment:160 in reply to: ↑ 154 Changed 9 years ago by
Replying to jason:
The cython function printing issue appears to boil down to those functions being treated as builtin functions, so this line applies: https://github.com/ipython/ipython/blob/master/IPython/lib/pretty.py#L669, rather than this line: https://github.com/ipython/ipython/blob/master/IPython/lib/pretty.py#L671
Suggestions for how to fix this? How do we distinguish between builtin functions and Cython methods?
I don't know. Even in previous versions of Sage:
sage: type(4.gcd) <type 'builtin_function_or_method'>
I wonder if changing this would require patching Cython. If we knew where the methods for Cython methods were defined, maybe we could just define a method _repr_pretty_
, as described in https://github.com/ipython/ipython/blob/master/IPython/lib/pretty.py#L27. Line 37373740 of local/lib/python/sitepackages/Cython/Compiler/Nodes.py
says
is_builtin_function_or_method = "PyCFunction_Check(%s)" % func_node_temp is_overridden = "(PyCFunction_GET_FUNCTION(%s) != (PyCFunction)%s)" % ( func_node_temp, self.py_func.entry.func_cname) code.putln("if (!%s  %s) {" % (is_builtin_function_or_method, is_overridden))
Is this relevant?
Alternatively, we could patch the file "pretty.py", modifying the definition of _type_pprinters
somehow, although I'm not sure how.
Alternatively, we could temporarily accept the new print representation for Cythondefined methods, and create a new ticket for fixing this. It seems like a small issue to me, and maybe it shouldn't hold up the rest of this ticket.
comment:161 Changed 9 years ago by
 Cc robertwb added
Alternatively, we could post to the Cython list and ask them :).
Ccing robertwb (see the comment or two above about Cython function printing, as well as http://mail.scipy.org/pipermail/ipythondev/2012October/010503.html, which tries to lay out the issue more concisely).
comment:162 Changed 9 years ago by
Asking the Cython developers is a great idea. Here's a real hack, but it seems to work: for builtin functions, if the print representation includes "sage.", we use that. (Now that I look at your post to the IPython list, it's just a version of the hack you described there.) This is a patch for IPython/lib/pretty.py:

pretty.py
old new 616 616 617 617 def _function_pprint(obj, p, cycle): 618 618 """Base pprint for all functions and builtin functions.""" 619 if obj.__ module__ in ('__builtin__', 'exceptions') or not obj.__module__:620 name = obj.__name__619 if obj.__repr__().find('sage.') != 1: 620 p.text(obj.__repr__()) 621 621 else: 622 name = obj.__module__ + '.' + obj.__name__ 623 p.text('<function %s>' % name) 622 if obj.__module__ in ('__builtin__', 'exceptions') or not obj.__module__: 623 name = obj.__name__ 624 else: 625 name = obj.__module__ + '.' + obj.__name__ 626 p.text('<function %s>' % name) 624 627 625 628 626 629 def _exception_pprint(obj, p, cycle):
comment:163 Changed 9 years ago by
On the Cython users mailing list, in which I essentially quoted Jason's message to the IPython list, Bradley Froehle says
There's nothing to be done in Cython here. As you point out, a.seed is a builtin (i.e. compiled) method and type(a.seed) returns `builtin_function_or_method` correctly.
So I think our options are
 hack pretty.py
 wait for a real IPython fix
 change the doctests to match the current output for now, and open a new ticket to fix this later.
Preferences? Am I missing any good choices?
comment:164 Changed 9 years ago by
Ping. Any opinions?
comment:165 Changed 9 years ago by
Is it hard to change the doctests (how many?). If that's easy, I'd say to change the doctests, especially given that any patch we make will take a long time to make it back into Sage through IPython. I'd veto "wait for a real IPython fix".
Is this the only thing holding us back now?
comment:166 Changed 9 years ago by
 Description modified (diff)
I think there is only one failing doctest:
sage t long force_lib devel/sage/sage/interfaces/sage0.py ********************************************************************** File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/12719/sage5.4.rc3/devel/sagemain/sage/interfaces/sage0.py", \ line 489: sage: sage0(4).gcd Expected: <builtin method gcd of sage.rings.integer.Integer object at 0x...> Got: <function gcd> **********************************************************************
and it should be trivial to fix. I also don't know how serious the issue is. I mean, it would be nice if 4.gcd
printed more information, but it doesn't seem crucial in any way.
Changed 9 years ago by
comment:167 Changed 9 years ago by
I'd say let's change our doctest, and then work with upstream to get a patch in. When the patch comes down (probably in a few months to a year?), we'll change our doctest again.
comment:168 Changed 9 years ago by
I changed the doctest. All tests pass for me on sage.math. Since it was already positively reviewed once before, I think we can set it back to that. What do you think?
comment:170 Changed 9 years ago by
 Status changed from needs_review to positive_review
comment:171 Changed 9 years ago by
 Milestone changed from sage5.4 to sage5.5
 Work issues Cython method printing deleted
comment:172 Changed 9 years ago by
jdemeyer  it would be great if this were merged into one of the 5.5 betas.
comment:173 Changed 9 years ago by
Of course, don't worry.
comment:174 Changed 9 years ago by
 Description modified (diff)
comment:175 Changed 9 years ago by
 Dependencies changed from #13459 to #13459, #13211
 Milestone changed from sage5.5 to sagepending
With #13211, I get
applying /release/merger/patches/trac_127195.1beta1.patch patching file sage/interfaces/gap.py Hunk #2 FAILED at 1065 1 out of 2 hunks FAILED  saving rejects to file sage/interfaces/gap.py.rej
Note that #13211 isn't certain to be merged (there were some problems before and the fixes haven't been properly tested yet).
comment:176 Changed 9 years ago by
I don't think I've ever applied #13211, and I certainly don't have it applied now. I didn't realize it was a dependency (I've just been pasting in the instructions in the description). Can we just remove the dependency? I'm not sure why we even have it.
comment:177 Changed 9 years ago by
My point is that it should be made a dependency and be rebased to #13211.
comment:178 followup: ↓ 179 Changed 9 years ago by
But if #13211's future isn't sure yet, should we add that as a dependency? Granted that the rebase (for either ticket) is a simple 12 line fix, but it'd be a shame if this huge work didn't get in because #13211 for some reason didn't pass muster.
You're the release manager, so you make the calls. Do you still want me to rebase to depend on that ticket? Does that hurt our chances of getting into 5.5?
comment:179 in reply to: ↑ 178 Changed 9 years ago by
Replying to jason:
You're the release manager, so you make the calls. Do you still want me to rebase to depend on that ticket? Does that hurt our chances of getting into 5.5?
I will test #13211 and then I'll know better whether or not it will be merged. So let's wait for now.
The best chances for getting this merged is to respond quickly when I ask for it to be rebased.
Changed 9 years ago by
comment:180 Changed 9 years ago by
 Description modified (diff)
Here's a patch rebased to #13211. Let's keep the old patch, too, just in case.
comment:181 Changed 9 years ago by
 Dependencies changed from #13459, #13211 to #13459, #9191
 Description modified (diff)
 Status changed from positive_review to needs_work
See #12167 for a related ticket: how we deal with IPython configuration files.