Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#14713 closed enhancement (fixed)

Update to IPython 1.2.1

Reported by: jason Owned by: jdemeyer
Priority: major Milestone: sage-6.2
Component: packages: standard Keywords:
Cc: roed Merged in:
Authors: Jason Grout Reviewers: William Stein
Report Upstream: N/A Work issues:
Branch: 0fb988d (Commits) Commit:
Dependencies: #14810 Stopgaps:

Description (last modified by ohanar)

IPython 1.2.1 has been released. Changes in IPython resulted in massive cleanup of the custom Sage transformations.

Upstream tarball: https://pypi.python.org/packages/source/i/ipython/ipython-1.2.1.tar.gz

Attachments (2)

new_transformers.patch (16.1 KB) - added by jason 6 years ago.
trac_14713_new_transformers.patch (18.8 KB) - added by vbraun 6 years ago.
Updated patch

Download all attachments as: .zip

Change History (46)

Changed 6 years ago by jason

comment:1 Changed 6 years ago by vbraun

  • Description modified (diff)

comment:2 Changed 6 years ago by vbraun

  • Dependencies set to #14810

Changed 6 years ago by vbraun

Updated patch

comment:3 Changed 6 years ago by vbraun

  • Cc roed added

Everything works except a failure in sage/doctest/forker.py when running the doctest through the sage0 interface. The pexpect log contains the traceback

MultipleInstanceError                     Traceback (most recent call last)
<ipython-input-12-54a61f46e9ab> in <module>()
----> 1 DTR.run(DT, clear_globs=False)

/home/vbraun/opt/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/doctest/forker.pyc in run(self, test, compileflags, out, clear_globs)
    623         self.no_failure_yet = True
    624         try:
--> 625             return self._run(test, compileflags, out)
    626         finally:
    627             self._fakeout.stop_spoofing()

/home/vbraun/opt/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/doctest/forker.pyc in _run(self, test, compileflags, out)
    535             elif outcome is FAILURE:
    536                 if not quiet:
--> 537                     self.report_failure(out, test, example, got, test.globs)
    538                 failures += 1
    539             elif outcome is BOOM:

/home/vbraun/opt/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/doctest/forker.pyc in report_failure(self, out, test, example, got, globs)
   1112                     prompt_config.in_template = 'debug: '
   1113                     prompt_config.in2_template = '.....: '
-> 1114                     embed(config=cfg, banner1='', user_ns=dict(globs))
   1115                 except KeyboardInterrupt:a

   1116                     # Assume this is a *real* interrupt. We need to

/home/vbraun/opt/sage-5.11.rc0/local/lib/python2.7/site-packages/IPython/terminal/embed.pyc in embed(**kwargs)
    298         config.InteractiveShellEmbed = config.TerminalInteractiveShell
    299         kwargs['config'] = config
--> 300     shell = InteractiveShellEmbed.instance(**kwargs)
    301     shell(header=header, stack_depth=2, compile_flags=compile_flags)

/home/vbraun/opt/sage-5.11.rc0/local/lib/python2.7/site-packages/IPython/config/configurable.pyc in instance(cls, *args, **kwargs)
    358             raise MultipleInstanceError(
    359                 'Multiple incompatible subclass instances of '
--> 360                 '%s are being created.' % cls.__name__
    361             )
    362 

MultipleInstanceError: Multiple incompatible subclass instances of InteractiveShellEmbed are being created.

comment:4 Changed 6 years ago by jason

I've been continuing work on this in my ipython-1.0 branch on github: https://github.com/jasongrout/sage/tree/ipython-1.0

I ran into some changes in IPython system_raw exit code that caused us problems. I posted about it here: http://mail.scipy.org/pipermail/ipython-dev/2013-August/012102.html But IPython 1.0 is released without this being resolved, so I think we ought to just update our doctests that are failing because of it.

I am also doing some experimental work on "string decorators" on that branch, inspired by what William did in sage cloud. However, it's still at a preview stage. I can turn it off by default, though, and we can continue to work on it.

comment:5 Changed 6 years ago by vbraun

The exit code thing has already been resolved in #14810

comment:6 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:7 Changed 6 years ago by was

+1 to this. I'm using ipython 1.0 with the patches from the git repo for cloud.sagemath very soon. Here's how:

cd devel/sage/
wget https://github.com/jasongrout/sage/commit/fdbe79ef7ed0ca0fa6c712c4c580ba34de1a1166.patch
wget https://github.com/jasongrout/sage/commit/c1ad5805558b15694d783bbb5c77296f2d492b2e.patch
patch --ignore-whitespace -p2 < fdbe79ef7ed0ca0fa6c712c4c580ba34de1a1166.patch
patch --ignore-whitespace -p2 < c1ad5805558b15694d783bbb5c77296f2d492b2e.patch
cd ../..
./sage -b
rm -rf local/lib/python/site-packages/IPython* local/lib/python/site-package/ipython*
./sage -sh
easy_install ipython

They work.

For convenience, here is a single hg patch:

http://wstein.org/home/wstein/tmp/trac-14713.patch

Last edited 6 years ago by was (previous) (diff)

comment:8 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:9 Changed 6 years ago by nthiery

Just for the record: some of my students often get hit by some bracktraces not being printed out properly when using %attach. This looks like https://github.com/ipython/ipython/issues/2512.

Nothing critical, but they will appreciate the upgrade :-)

Thanks!

comment:10 Changed 6 years ago by jason

2.0 will probably be released within the month or so. I'm tracking the changes on IPython master so it will be an easy upgrade.

comment:11 Changed 6 years ago by nthiery

Nice. Thanks for the update!

comment:12 Changed 6 years ago by jason

Sure, no problem. I'm tracking the changes because I'm running (close to) IPython master for the Sage cell server. My current patches are here: https://github.com/jasongrout/sage/commits/sagecell (up through change d662bf2655dcdc04100a8f29ccb88b790377b46a or so)

comment:13 follow-up: Changed 6 years ago by ohanar

  • Branch set to u/ohanar/ipython-upgrade
  • Commit set to 7293ef21fc691d367bf3e1dc663565097179040a
  • Description modified (diff)
  • Summary changed from Update to IPython 1.0 to Update to IPython 1.2.0

I see no reason to wait for IPython 2.0. There are still some issues that stem from the issue found in #14961.

comment:14 Changed 6 years ago by git

  • Commit changed from 7293ef21fc691d367bf3e1dc663565097179040a to a86c939634000fc553763b11e07ef1e8acb7e9e8

Branch pushed to git repo; I updated commit sha1. New commits:

a86c939really fix instance creation of InteractiveShellEmbed

comment:15 Changed 6 years ago by git

  • Commit changed from a86c939634000fc553763b11e07ef1e8acb7e9e8 to 2ffb13cd82cc62d86e2a657ad0f5151a56a80e8c

Branch pushed to git repo; I updated commit sha1. New commits:

24ea892dynamically set IPYTHONDIR based on the current version of ipython
2ffb13cbackport IPython pull request #4504

comment:16 Changed 6 years ago by ohanar

There is a single doctest that fails (in src/sage/misc/interpreter.py), but it doesn't match the behavior of running the test from the sage command line.

Also, we need some doctests for sage.misc.interpreter.SagePreparseTransformer.reset. Since I don't know the preparser code (and the reset keyword is documented as "a boolean"), it would be great if someone knowledgeable about this could fill in something here.

@jason I deleted the note about the prun_magics.patch since you deleted it in https://github.com/jasongrout/sage/commit/d662bf2655dcdc04100a8f29ccb88b790377b46a. Do we still need this?

comment:17 in reply to: ↑ 13 Changed 6 years ago by was

Replying to ohanar:

I see no reason to wait for IPython 2.0. There are still some issues that stem from the issue found in #14961.

I strongly agree with not waiting for IPython 2.0. They also make some controversial modal changes to the IPython notebook, and I want to see how those shake out in wider testing before switching (i.e., it might be best to wait for 2.1).

comment:18 Changed 6 years ago by was

How is this supposed to actually work?

Attempting to download package ipython-1.2.0
>>> Trying to download http://www.sagemath.org/packages/upstream/ipython/ipython-1.2.0.tar.gz
[Traceback (most recent call last):
  File "<stdin>", line 35, in <module>
  File "/usr/local/sage/sage-6.2/local/lib/python/urllib.py", line 240, in retrieve
    fp = self.open(url, data)
  File "/usr/local/sage/sage-6.2/local/lib/python/urllib.py", line 208, in open
    return getattr(self, name)(url)
  File "/usr/local/sage/sage-6.2/local/lib/python/urllib.py", line 359, in open_http
    return self.http_error(url, fp, errcode, errmsg, headers)
  File "/usr/local/sage/sage-6.2/local/lib/python/urllib.py", line 376, in http_error
    return self.http_error_default(url, fp, errcode, errmsg, headers)
  File "<stdin>", line 17, in http_error_default
IOError: [Errno 404] Not Found: '//www.sagemath.org/packages/upstream/ipython/ipython-1.2.0.tar.gz'
Error: failed to download package ipython-1.2.0
make[2]: *** [/usr/local/sage/sage-6.2/local/var/lib/sage/installed/ipython-1.2.0] Error 1
make[2]: Leaving directory `/usr/local/sage/sage-6.2/build'
make[1]: *** [all] Error 2
make[1]: Leaving directory `/usr/local/sage/sage-6.2/build'
 
real    24m14.305s

Obviously, I can get around this. However, I'm concerned about this entire approach to development...

comment:19 follow-up: Changed 6 years ago by vbraun

You need to put the tarball manually in the upstream directory until the ticket is reviewed.

comment:20 in reply to: ↑ 19 Changed 6 years ago by was

Replying to vbraun:

You need to put the tarball manually in the upstream directory until the ticket is reviewed.

That's what I did. It would be nice if the code that tries to grab the upstream from sagemath.org could also try other locations specified in the package, e.g., in this case https://pypi.python.org/packages/source/i/ipython/ipython-1.2.0.tar.gz...

I hope our longterm plan is that the process is:

   git <checkout a branch somehow>
   make

and it works, rather than

   git <checkout a branch somehow>
   make
   # see download failure -- track down packages -- put in upstream directory
   make
   # see more download failures -- track down packages -- put in upstream directory, etc.

?

comment:21 Changed 6 years ago by vbraun

The pypi url is not in the git branch. There are also some projects where you need to click through some html to get a temporary download link *cough* *sourceforge* *cough*. Really we need a common place where people can upload the tarballs.

comment:22 Changed 6 years ago by jason

+1 to moving to 1.2 now. I'm not running any extra patches beyond my 1.0 patches to get IPython master working. So if you merge this, I can stop maintaining the patches, even though I'm running master :).

And +1 to william's suggestion of having alternate URLs, since we are verifying the checksum anyway.

comment:23 follow-up: Changed 6 years ago by jason

Note that IPython 1.2.1 is out, which fixes a bug they noticed just after 1.2

comment:24 Changed 6 years ago by was

*IMPORTANT* This patch introduces this code in the middle of src/bin/sage-env *before* the path, etc., is properly set up:

if [ -x "$SAGE_LOCAL/bin/ipython" ]; then
    export IPYTHONDIR="$DOT_SAGE/ipython-"`ipython --version`
else
    # IPython is not yet installed
    export IPYTHONDIR="$DOT_SAGE/ipython-"`sed 's+\.p[0-9]*$++' "$SAGE_ROOT"/build/pkgs/ipython/package-version.txt`
fi

The result is that in a system with multiple version of Ipython (e.g., using python2.7.3 also) you'll get a big nasty ImportError: cannot import name MAXREPEAT error/traceback whenever you try to start sage (or use sage -sh). Moving the above IPYTHONDIR stuff to the bottom of the sage-env fixes the problem.

That said, it's kind of disturbing to be starting up ipython when doing "sage -sh", since that's actually a lot of overhead.

comment:25 in reply to: ↑ 23 Changed 6 years ago by was

Replying to jason:

Note that IPython 1.2.1 is out, which fixes a bug they noticed just after 1.2

The upstream tarball is: https://pypi.python.org/packages/source/i/ipython/ipython-1.2.1.tar.gz

comment:26 Changed 6 years ago by was

I know patches are stupid, but in case anybody cares, here's the patch to move the ipython version environ stuff and update the version of the tarball: http://wstein.org/tmp/ipython-1.2.1.patch

comment:27 Changed 6 years ago by git

  • Commit changed from 2ffb13cd82cc62d86e2a657ad0f5151a56a80e8c to 88a253c4a5d70855832d2de0ea258e9b0d0d9b7e

Branch pushed to git repo; I updated commit sha1. New commits:

3a0b07fipython: upgrade to version 1.2.1
061e7d6interpreter: preparser really is a stateless transformer
88a253cinterpreter: fix doctest issue

comment:28 Changed 6 years ago by git

  • Commit changed from 88a253c4a5d70855832d2de0ea258e9b0d0d9b7e to 0fb988d80d32913715f96bef242ff483f4c24b2f

Branch pushed to git repo; I updated commit sha1. New commits:

0fb988dsage-env: don't call ipython when creating the env

comment:29 Changed 6 years ago by ohanar

  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from Update to IPython 1.2.0 to Update to IPython 1.2.1

comment:30 Changed 6 years ago by ohanar

  • Description modified (diff)

comment:31 Changed 6 years ago by ohanar

  • Description modified (diff)

comment:32 follow-up: Changed 6 years ago by jason

You changed the preparser to a stateless transformer, but the preparse function *does* have state---it keeps track of the quote state, right?

comment:33 in reply to: ↑ 32 Changed 6 years ago by ohanar

Replying to jason:

You changed the preparser to a stateless transformer, but the preparse function *does* have state---it keeps track of the quote state, right?

Yes, although that seems like a hack that was only needed in old versions of ipython. Now ipython doesn't push an open ended quote state to the transformers, so the preparser doesn't actually need to keep track of this anymore.

I think there is a lot that can be done to clean up the preparser in general, but I think that is better fit for another ticket.

Last edited 6 years ago by ohanar (previous) (diff)

comment:34 Changed 6 years ago by was

I've read all of this code pretty carefully, used it for months (in various incarnations by Jason and Andrew), and think upgrading to IPython 1.2.1 should be a very high priority for Sage. I'm running a clean build and round of doctests, and if they succeed, then this should get a positive review!

comment:35 Changed 6 years ago by jason

I'll do some checking too. My worry is not that the preparser could be cleaned up (it certainly could, but that's another issue), but that changing from a coroutine transformer to a stateless transformer is introducing bugs. I'm not sure why that change was made. Could you perhaps explain why it's not a coroutine transformer still?

comment:36 Changed 6 years ago by jason

oh, I see you did have an explanation. I'll look into it to make sure.

comment:37 Changed 6 years ago by was

  • Status changed from needs_review to positive_review

Builds fine and all tests pass. The issues I pointed out above have also been addressed.

comment:38 Changed 6 years ago by jason

and I'm still building...

comment:39 Changed 6 years ago by jason

Giving a partial string as a command messes up the parser because the state is not reset, like "asdf. I'm figuring out how to exploit it.

comment:40 Changed 6 years ago by jason

All right +1. IPython *does* push open-ended quotes into the preparser if the line is a syntax error (like "a). However, the preparser is now always called with reset=True (the default), so the state doesn't carry over to the next line.

Here is a more complete explanation for it being okay to have a stateless transformation---it relies on two facts that are true right now:

  • we've switched the preparser to be an IPython python_line_transform (which assembles multiline strings into single lines)
  • the preparser has a default of reset=True, which means every time it is called, the state is discarded

If either of those changes, trouble would probably show up.

Other than that detail, this looks pretty much like the code I've been running on the sage cell server for nearly 6 months, so +1.

Last edited 6 years ago by jason (previous) (diff)

comment:41 Changed 6 years ago by vbraun

  • Reviewers set to William Stein

comment:42 Changed 6 years ago by vbraun

  • Branch changed from u/ohanar/ipython-upgrade to 0fb988d80d32913715f96bef242ff483f4c24b2f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:43 Changed 6 years ago by jdemeyer

  • Commit 0fb988d80d32913715f96bef242ff483f4c24b2f deleted

Blocker follow-up at #15972

comment:44 Changed 5 years ago by vbraun

Random test failures due to history corruption at #16372

Note: See TracTickets for help on using tickets.