upgrade to ipython 7 - Githubissues

fchapoton commented 5 years ago

Upgrade ipython and related packages:

https://pypi.org/project/ipython/ 7.13.0 (current as of 2020-06-05: 7.15.0)
https://pypi.org/project/ipykernel/ 5.2.1 (current as of 2020-06-05: 5.3.0)
https://pypi.org/project/backcall/ 0.1.0 (current as of 2020-06-05)
https://pypi.org/project/jedi/ 0.17.0 (current as of 2020-06-05)
https://pypi.org/project/prompt-toolkit/ 3.0.5 (current as of 2020-06-05)

All of these package versions support python >= 3.6 (note #29033).

Tarballs: see checksums.ini [upstream_url]. (To configure Sage to download from the upstream URLs, use ./configure --enable-download-from-upstream-url)

CC: @embray @jdemeyer @slel @kiwifb @timokau @antonio-rojas

Component: packages: standard

Keywords: upgrade

Author: Jonathan Kliem, John Palmieri, Antonio Rojas

Branch: e21a7b0

Reviewer: John Palmieri, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/28197

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`cb86032`	`trac 28197: IPython 7.13.0, with new required packages jedi, backcall`
`bbfbc4f`	`trac 28197: fix preparser for new IPython: patch taken from`
`61157b8`	`trac 28197: add new package parso`
`a9594b5`	`trac 28197: add pip as dependency for backcall, jedi, parso`

jhpalmieri commented 4 years ago

comment:46

On OS X, with this + #29042, the qqbar test is the only one that fails for me.

mkoeppe commented 4 years ago

comment:48

PYTHON_TOOLKIT should probably be PYTHON_TOOLCHAIN

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from a9594b5 to 7f73577

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7f73577 trac 28197: add pip as dependency for backcall, jedi, parso

jhpalmieri commented 4 years ago

comment:50

Thanks, fixed.

mkoeppe commented 4 years ago

comment:51

What is missing on this ticket?

jhpalmieri commented 4 years ago

comment:53

In my opinion, the only thing missing is how to resolve the qqbar doctest failure. I proposed a solution in comment:43, but I would like some feedback before adding it to the branch. Maybe I'm missing something, or maybe there is a better solution.

kliem commented 4 years ago

comment:54

The test tests two things, if I see this correctly:

Changing the recursion limit in the method allows for

a=s([3,2]).expand(8)(flatten([[QQbar.zeta(3)^d for d in range(3)], [QQbar.zeta(5)^d for d in range(5)]]))

to be exactified.

Also the recursion limit should be the same after the method was called.

If the initial recursion limit is already 3000, the test doesn't work anymore. As a can be exactified without modifying the recursion limit in the method exactify (I checked). So the test passes with the solution in comment:43, but it is almost meaningless.

I see two solutions for this:

Replace a by

a=s([3,2]).expand(10)(flatten([[QQbar.zeta(3)^d for d in range(3)], [QQbar.zeta(7)^d for d in range(7)]]))

This value cannot be exactified with recursion limit 3000, but it works fine once the method increases the recursion limit. However the doctest takes a bit longer.

We could modify the recursion limit before and after the doctest. This is faster. However, we rely on the same machinery as the method.

The advantage is that this really is a long time solution. If with some new update the default recursion limit is increased again, we can still see that the method works correctly.

jhpalmieri commented 4 years ago

comment:55

With the longer version (your first proposal), a.exactify() takes over 10 seconds on my computer, which is longer than we usually aim for with tests. So I would prefer your second proposal.

antonio-rojas commented 4 years ago

comment:56

Replying to @mkoeppe:

What is missing on this ticket?

The removed tests from the old SagePromptTransformer() need to be restored

kliem commented 4 years ago

Changed branch from public/ticket/28197 to public/ticket/28197-reb

kliem commented 4 years ago

New commits:

`f832bbe`	`trac 28197: IPython 7.13.0, with new required packages jedi, backcall`
`e3ebfc1`	`trac 28197: fix preparser for new IPython: patch taken from`
`dc6e1d8`	`trac 28197: add new package parso`
`7ee3f3d`	`trac 28197: add pip as dependency for backcall, jedi, parso`
`41e6355`	`lower the recursion limit for exactification test`

kliem commented 4 years ago

Changed commit from 7f73577 to 41e6355

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

114d322 readd prompt transformer doc tests

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 41e6355 to 114d322

kliem commented 4 years ago

Author: Jonathan Kliem, John H. Palmieri, Antonio Rojas

kliem commented 4 years ago

comment:59

As for the authors, I kind of guessed. Is this correct? Am I missing something?

Also I don't really know how we do it with order of authors. I went alphabetically. However, I just fixed doctests.

antonio-rojas commented 4 years ago

comment:60

diff --git a/src/sage/rings/qqbar.py b/src/sage/rings/qqbar.py
index 7fa7f8d..fd4b3bf 100644
--- a/src/sage/rings/qqbar.py
+++ b/src/sage/rings/qqbar.py
@@ -8091,9 +8091,16 @@ class ANBinaryExpr(ANDescr):

         We check to make sure that this method still works even. We
         do this by increasing the recursion level at each step and
-        decrease it before we return::
+        decrease it before we return.
+        We lower the recursion limit for this test to allow
+        a test in reasonable time::

-            sage: import sys; sys.getrecursionlimit()
+            sage: import sys
+            sage: old_recursion_limit = sys.getrecursionlimit()
+            sage: sys.setrecursionlimit(1000)
+
+            sage: old_recursion_limit = sys.getrecursionlimit()
+            sage: sys.getrecursionlimit()
             1000
             sage: s = SymmetricFunctions(QQ).schur()
             sage: a=s([3,2]).expand(8)(flatten([[QQbar.zeta(3)^d for d in range(3)], [QQbar.zeta(5)^d for d in range(5)]]))

The second old_recursion_limit = sys.getrecursionlimit() here doesn't look right. This will always set the recursion limit to 1000 at the end regardless of the original value

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 114d322 to 751864b

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

751864b fixed mistake

kliem commented 4 years ago

comment:62

Thanks for catching this.

Replying to @antonio-rojas:

diff --git a/src/sage/rings/qqbar.py b/src/sage/rings/qqbar.py
index 7fa7f8d..fd4b3bf 100644
--- a/src/sage/rings/qqbar.py
+++ b/src/sage/rings/qqbar.py
@@ -8091,9 +8091,16 @@ class ANBinaryExpr(ANDescr):

         We check to make sure that this method still works even. We
         do this by increasing the recursion level at each step and
-        decrease it before we return::
+        decrease it before we return.
+        We lower the recursion limit for this test to allow
+        a test in reasonable time::

-            sage: import sys; sys.getrecursionlimit()
+            sage: import sys
+            sage: old_recursion_limit = sys.getrecursionlimit()
+            sage: sys.setrecursionlimit(1000)
+
+            sage: old_recursion_limit = sys.getrecursionlimit()
+            sage: sys.getrecursionlimit()
             1000
             sage: s = SymmetricFunctions(QQ).schur()
             sage: a=s([3,2]).expand(8)(flatten([[QQbar.zeta(3)^d for d in range(3)], [QQbar.zeta(5)^d for d in range(5)]]))

The second old_recursion_limit = sys.getrecursionlimit() here doesn't look right. This will always set the recursion limit to 1000 at the end regardless of the original value

jhpalmieri commented 4 years ago

comment:63

Are all of the issues taken care of? Tests pass for me on OS X (after merging #29042).

slel commented 4 years ago

Changed keywords from none to upgrade

slel commented 4 years ago

Changed author from Jonathan Kliem, John H. Palmieri, Antonio Rojas to Jonathan Kliem, John Palmieri, Antonio Rojas

slel commented 4 years ago

comment:64

(Fixing John Palmieri's name in "Authors" field to match its spelling in other tickets.)

fchapoton commented 4 years ago

comment:65

So, can we move on here ? Patchbots are helpless to check that nothing is broken. Does the gitlab CI help in this case ?

kliem commented 4 years ago

comment:66

I started a test run pulling this ticket and #29851 in the newest develop (#29851 fixes the workflow for debian bullseye and debian sid, so this seems reasonable for a good testing experience, it just allows reinstalling python for them, so really nothing to do with our ticket).

Results will be available here https://github.com/kliem/sage/pull/16/checks.

jhpalmieri commented 4 years ago

comment:67

This still works for me on OS X, after merging with the latest develop branch. I'll switch to positive review in a few days, naming some collection of reviewers unless (a) someone objects or (b) someone beats me to it.

mkoeppe commented 4 years ago

comment:68

I have also tested an earlier version successfully as part of #29864 (namespace packages). Let's get this into the next beta for wider testing.

mkoeppe commented 4 years ago

Reviewer: John Palmieri, Matthias Koeppe

kliem commented 4 years ago

comment:69

As for the tests, there is no obvious failure. So at least installing seems to work everywhere. I didn't test any generated docker images to see how it actually looks though.

jhpalmieri commented 4 years ago

comment:70

I'm carrying out my threat to set a positive review ;)

Dear all: feel free to add yourself as a reviewer or an author (or both), as you see fit.

jhpalmieri commented 4 years ago

comment:71

For a future ticket, we could also upgrade to 7.16.1. I just tried, and it builds and passes tests on my OS X box. I want to get this ticket in as it stands, though.

kiwifb commented 4 years ago

comment:72

Replying to @jhpalmieri:

For a future ticket, we could also upgrade to 7.16.1. I just tried, and it builds and passes tests on my OS X box. I want to get this ticket in as it stands, though.

For the record, I am at 7.16.1 in sage-on-gentoo as well. Merging as it it now is the most important bit.