Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update doctests for compatibility with ipython 8.x #33170

Closed
antonio-rojas opened this issue Jan 14, 2022 · 25 comments
Closed

Update doctests for compatibility with ipython 8.x #33170

antonio-rojas opened this issue Jan 14, 2022 · 25 comments

Comments

@antonio-rojas
Copy link
Contributor

Besides trivial output format changes, the only breakage seems to be tracebacks no longer showing cython source code

**********************************************************************
File "src/sage/repl/interpreter.py", line 77, in sage.repl.interpreter
Failed example:
    print("dummy line"); shell.run_cell('1/0') # see #25320 for the reason of the `...` and the dummy line in this test
Expected:
    dummy line
    ...
    ZeroDivisionError...Traceback (most recent call last)
    <ipython-input-...> in <module>...
    ----> 1 Integer(1)/Integer(0)
    .../sage/rings/integer.pyx in sage.rings.integer.Integer...div...
    ...
    -> ...                  raise ZeroDivisionError("rational division by zero")
       ...            x = <Rational> Rational.__new__(Rational)
       ...            mpq_div_zz(x.value, ....value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
Got:
    dummy line
    ---------------------------------------------------------------------------
    ZeroDivisionError                         Traceback (most recent call last)
    Input In [1], in <module>
    ----> 1 Integer(1)/Integer(0)
    <BLANKLINE>
    File /usr/lib/python3.10/site-packages/sage/rings/integer.pyx:1987, in sage.rings.integer.Integer.__truediv__ (build/cythonized/sage/rings/integer.c:13679)()
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
**********************************************************************

The update itself requires a bunch of new dependencies

CC: @mkoeppe @kiwifb @collares

Component: packages: standard

Author: Antonio Rojas

Branch/Commit: 4d2b53f

Reviewer: Gonzalo Tornaría

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

@antonio-rojas antonio-rojas added this to the sage-9.5 milestone Jan 14, 2022
@antonio-rojas
Copy link
Contributor Author

Branch: u/arojas/update_to_ipython_8

@antonio-rojas
Copy link
Contributor Author

Commit: c373551

@antonio-rojas

This comment has been minimized.

@antonio-rojas
Copy link
Contributor Author

New commits:

c373551Fix tests expected output to match ipython 8

@8d1h
Copy link
Mannequin

8d1h mannequin commented Jan 30, 2022

comment:3

(Thank you for maintaining Sage on archlinux! Just updated to 9.5)

I think it might be a good idea to document somewhere how to disable the autoformatter and suggestion in IPython 8, which are enabled by default (also useful for people who haven't updated to IPython 8 and want to enable these features)

shell = get_ipython()
shell.autoformatter = None
shell.pt_app.auto_suggest = None
# from prompt_toolkit.auto_suggest import AutoSuggestFromHistory
# shell.autoformatter = "black"
# shell.pt_app.auto_suggest = AutoSuggestFromHistory()

@mkoeppe
Copy link
Member

mkoeppe commented Jan 30, 2022

comment:4

According to release notes, IPython 8 is still in alpha/beta stage, https://ipython.readthedocs.io/en/stable/whatsnew/version8.html#ipython-8-0

So I don't think we should do the actual update soon. Perhaps open a separate ticket for the update and only put doctest adjustments on the present ticket?

@yyyyx4
Copy link
Member

yyyyx4 commented Feb 1, 2022

comment:5

Replying to @8d1h:

I think it might be a good idea to document somewhere how to disable the autoformatter and suggestion in IPython 8, which are enabled by default

In my opinion, we should actually disable these features by default. They change Sage's behavior in a potentially very irritating way and many users won't even know what is causing it and which keywords to search for to change it back.

@collares
Copy link
Contributor

comment:6

I filed alexmojaki/stack_data#21 to address the lack of Cython source code in tracebacks. I also asked for a clarification with regards to IPython 8's stability and, according to the developer, IPython 8 is a stable release; the notice in the release notes about it being alpha/beta quality just wasn't removed soon enough.

@collares
Copy link
Contributor

comment:7

Although I won't have time to test it until next week, upstream just reported that Cython source code should now appear on tracebacks if Python package stack_data is updated to 0.2.0.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2022

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

a90a314Fix cython source test output for ipython 8 syntax

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2022

Changed commit from c373551 to a90a314

@antonio-rojas
Copy link
Contributor Author

comment:9

Replying to @collares:

Although I won't have time to test it until next week, upstream just reported that Cython source code should now appear on tracebacks if Python package stack_data is updated to 0.2.0.

Confirmed, thanks. Test still failed because of slightly different syntax, should be fixed now.

@tornaria
Copy link
Contributor

comment:10

In addition to the failure reported in the description (which a90a314 fixes) I'm getting two more doctest failures when upgrading to ipython 8.0.1:

$ sage -t src/sage/repl/interpreter.py src/sage/repl/interface_magic.py 
Running doctests with ID 2022-02-28-15-14-42-bbd14288.
Using --optional=build,pip,sage,sage_spkg,void
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,pandoc,pdf2svg,plantri,pynormaliz,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.plot,sage.rings.number_field,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 2 files.
sage -t --warn-long 32.0 --random-seed=265810042777151130630499733658806769461 src/sage/repl/interpreter.py
**********************************************************************
File "src/sage/repl/interpreter.py", line 425, in sage.repl.interpreter.SagePreparseTransformer
Failed example:
    shell.run_cell(bad_syntax)
Expected:
      File "<string>", line unknown
    SyntaxError: Mismatched ']'
    <BLANKLINE>
Got:
      File <string>
    SyntaxError: Mismatched ']'
    <BLANKLINE>
**********************************************************************
1 item had failures:
   1 of  14 in sage.repl.interpreter.SagePreparseTransformer
    [137 tests, 1 failure, 3.36 s]
sage -t --warn-long 32.0 --random-seed=265810042777151130630499733658806769461 src/sage/repl/interface_magic.py
**********************************************************************
File "src/sage/repl/interface_magic.py", line 262, in sage.repl.interface_magic.InterfaceMagic.cell_magic_factory
Failed example:
    shell.run_cell('%%gap foo\n1+1;\n')
Expected:
    ...File "<string>", line unknown
    SyntaxError: Interface magics have no options, got "foo"
    <BLANKLINE>
Got:
    Traceback (most recent call last):
    <BLANKLINE>
      File /usr/lib/python3.10/site-packages/IPython/core/interactiveshell.py:3251 in run_code
        exec(code_obj, self.user_global_ns, self.user_ns)
    <BLANKLINE>
      Input In [1] in <module>
        get_ipython().run_cell_magic('gap', 'foo', '1+1;\n')
    <BLANKLINE>
      File /usr/lib/python3.10/site-packages/IPython/core/interactiveshell.py:2257 in run_cell_magic
        result = fn(*args, **kwargs)
    <BLANKLINE>
      File /usr/lib/python3.10/site-packages/sage/repl/interface_magic.py:295 in cell_magic
        raise SyntaxError('Interface magics have no options, got "{0}"'.format(line))
    <BLANKLINE>
      File <string>
    SyntaxError: Interface magics have no options, got "foo"
    <BLANKLINE>
**********************************************************************
1 item had failures:
   1 of  11 in sage.repl.interface_magic.InterfaceMagic.cell_magic_factory
    [30 tests, 1 failure, 1.56 s]
----------------------------------------------------------------------
sage -t --warn-long 32.0 --random-seed=265810042777151130630499733658806769461 src/sage/repl/interpreter.py  # 1 doctest failed
sage -t --warn-long 32.0 --random-seed=265810042777151130630499733658806769461 src/sage/repl/interface_magic.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 15.6 seconds
    cpu time: 3.0 seconds
    cumulative wall time: 4.9 seconds
Features detected for doctesting: 

The following patch fixes these:

diff --git a/src/sage/repl/interface_magic.py b/src/sage/repl/interface_magic.py
index 8a455b69b0e..95d89628992 100644
--- a/src/sage/repl/interface_magic.py
+++ b/src/sage/repl/interface_magic.py
@@ -260,7 +260,7 @@ class InterfaceMagic(object):
             2
             120
             sage: shell.run_cell('%%gap foo\n1+1;\n')
-            ...File "<string>", line unknown
+            ...File ...<string>...
             SyntaxError: Interface magics have no options, got "foo"
             <BLANKLINE>
             sage: shell.run_cell('%%gap?')
diff --git a/src/sage/repl/interpreter.py b/src/sage/repl/interpreter.py
index cabb71097bc..47adea3979e 100644
--- a/src/sage/repl/interpreter.py
+++ b/src/sage/repl/interpreter.py
@@ -423,7 +423,7 @@ def SagePreparseTransformer(lines):
         sage: from sage.repl.interpreter import get_test_shell
         sage: shell = get_test_shell()
         sage: shell.run_cell(bad_syntax)
-          File "<string>", line unknown
+          File ...<string>...
         SyntaxError: Mismatched ']'
         <BLANKLINE>
         sage: shell.quit()

@antonio-rojas
Copy link
Contributor Author

comment:11

Replying to @tornaria:

In addition to the failure reported in the description (which a90a314 fixes) I'm getting two more doctest failures when upgrading to ipython 8.0.1:

This is fixed in the branch already

@tornaria
Copy link
Contributor

comment:12

Replying to @antonio-rojas:

Replying to @tornaria:

In addition to the failure reported in the description (which a90a314 fixes) I'm getting two more doctest failures when upgrading to ipython 8.0.1:

This is fixed in the branch already

My bad, it's there indeed as c373551, thanks.

Sorry for the noise.

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 4, 2022
@tornaria
Copy link
Contributor

comment:14

This works ok with ipython 8.0.1, but it's already broken on ipython 8.1.0 and 8.1.1:

Failed example:
    print("dummy line"); shell.run_cell('1/0') # see #25320 for the reason of the `...` and the dummy line in this test
Expected:
    dummy line
    ...
    ZeroDivisionError...Traceback (most recent call last)
    ...in <module>...
    ----> 1 Integer(1)/Integer(0)
    .../sage/rings/integer.pyx... in sage.rings.integer.Integer...div...
    ...
    -> ...                  raise ZeroDivisionError("rational division by zero")
       ...            x = <Rational> Rational.__new__(Rational)
       ...            mpq_div_zz(x.value, ....value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
Got:
    dummy line
    ---------------------------------------------------------------------------
    ZeroDivisionError                         Traceback (most recent call last)
    Input In [1], in <cell line: 1>()
    ----> 1 Integer(1)/Integer(0)
    <BLANKLINE>
    File /usr/lib/python3.10/site-packages/sage/rings/integer.pyx:1987, in sage.rings.integer.Integer.__truediv__ (build/cythonized/sage/rings/integer.c:13693)()
       1985 if type(left) is type(right):
       1986     if mpz_sgn((<Integer>right).value) == 0:
    -> 1987         raise ZeroDivisionError("rational division by zero")
       1988     x = <Rational> Rational.__new__(Rational)
       1989     mpq_div_zz(x.value, (<Integer>left).value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
**********************************************************************

Can you add the following patch on top of your previous commits?

--- a/src/sage/repl/interpreter.py
+++ b/src/sage/repl/interpreter.py
@@ -78,7 +78,7 @@ Check that Cython source code appears in tracebacks::
     dummy line
     ...
     ZeroDivisionError...Traceback (most recent call last)
-    ...in <module>...
+    ...
     ----> 1 Integer(1)/Integer(0)
     .../sage/rings/integer.pyx... in sage.rings.integer.Integer...div...
     ...

The other file (interface_magic.py) is still ok with ipython 8.1.

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.6 Mar 18, 2022
@mkoeppe mkoeppe changed the title Update to ipython 8 Update doctests for compatibility with ipython 8.x Mar 18, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 18, 2022

Changed commit from a90a314 to 4d2b53f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 18, 2022

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

4d2b53fUpdate test for ipython 8.1

@antonio-rojas
Copy link
Contributor Author

comment:17

Done, thanks. Needs review since this is now only about fixing tests.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 18, 2022

Author: Antonio Rojas

@tornaria
Copy link
Contributor

comment:19

Thanks, I'll give positive review once CI on void-linux/void-packages#36209 is done.

@tornaria
Copy link
Contributor

comment:20

All ok on void linux using system ipython 8.1 (glibc 64 and 32 bit, musl 64 bit tested at void-linux/void-packages#36209)

@tornaria
Copy link
Contributor

Reviewer: Gonzalo Tornaría

@mkoeppe
Copy link
Member

mkoeppe commented Mar 19, 2022

comment:21

Replying to @collares:

I also asked for a clarification with regards to IPython 8's stability and, according to the developer, IPython 8 is a stable release; the notice in the release notes about it being alpha/beta quality just wasn't removed soon enough.

I've opened #33530 for the actual update. This is for Sage 9.7 because the update will require us to drop support for Python 3.7.

@vbraun
Copy link
Member

vbraun commented Mar 21, 2022

Changed branch from u/arojas/update_to_ipython_8 to 4d2b53f

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

No branches or pull requests

6 participants