Support older versions of backports.shutil_get_terminal_size

[timokau]

Because of a broken version of backports.shutil_get_terminal_size, some distros show tracebacks without spacing as follows:

    ZeroDivisionErrorTraceback (most recent call last)
    ...
    ZeroDivisionError: rational division by zero

Apparently debian has the same problem.

This patch makes the doctests accept both formats.

Component: porting

Author: Timo Kaufmann

Branch: d24dd41

Reviewer: Julian Rüth

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

[seblabbe]

comment:2

There is a log file in the commit.

[sagetrac-git]

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

f79070d

Make the ZeroDivisionError format more flexible

[sagetrac-git]

Changed commit from fb04c0c to f79070d

[timokau]

comment:4

Oh sorry, I should've reviewed before pushing. Removed.

[jdemeyer]

comment:5

You need a patched version of https://github.com/chrippa/backports.shutil_get_terminal_size

[timokau]

comment:9

It seems like this patch was only included to fix this formatting error (#20599). This is a bit meta, but is there a general patching policy for sage?

Sage patches a lot of its dependencies and often has very good reasons to do so. But sometimes it is not strictly necessary. That makes life hard for packagers and also for the people maintaining the sage spkgs. In my opinion patching dependencies should be a last resort, only if there is pretty much no other option.

[jdemeyer]

comment:10

Replying to @timokau:

This is a bit meta, but is there a general patching policy for sage?

There is no such policy. But it might be a good topic for discussion on sage-packaging (do you know about that mailing list?)

[timokau]

comment:11

Yes I do, but to my understanding the purpose of that list is packagers working together to solve packaging issues right? Such a policy would benefit packagers, but upstream developers would need to be the ones actually deciding on it and adhering to it. So I don't think discussing that on sage-packaging is very useful.

Maybe sage-devel, if there is no more proper place to discuss that?

[timokau]

comment:12

I have posted on sage-devel about this (https://groups.google.com/forum/#!topic/sage-devel/-lBph6WWg5E), so this ticket can go back to topic.

[saraedum]

comment:13

Your patch looks good to me. I think a # see #25320 for the apparently unnecessary ... or something similar would be nice to make sure that nobody takes this out on the next refactoring.

Feel free to set this to positive review once you added a comment, or also if you don't deem it necessary.

[saraedum]

Reviewer: Julian Rüth

[timokau]

comment:14

Yeah I thought about that but I wasn't sure how to add comments to the doctests.

Simply a sage: # my comment line? That would also show up in the generated docs right?

[saraedum]

comment:15

Yes, I think that's acceptable.

[sagetrac-git]

Changed commit from f79070d to 3629f9a

[sagetrac-git]

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

3629f9a

Make the ZeroDivisionError format more flexible

[timokau]

comment:17

Okay, I added such a comment.

---

New commits:

3629f9a

Make the ZeroDivisionError format more flexible

[timokau]

comment:20

Sorry!

The issue seems to be the missing initial ... at

Expected:
    ZeroDivisionError...Traceback (most recent call last)

while the test code is:

sage: shell.run_cell('1/0') # see #25320 for the reason of the `...` in this test
...
ZeroDivisionError...Traceback (most recent call last)

Does anybody know why the ... in the first line is ignored? (Or why this wasn't caught by my local tests).

[kiwifb]

comment:21

Not sure. Also Volker is only quoting one failure, does that mean that the other one passes or did he imply it was also failing? A pity, that's the first time in a long time this doctest doesn't fail on me in sage-on-gentoo.

[sagetrac-git]

Changed commit from 3629f9a to c1efa49

[sagetrac-git]

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

c1efa49

Make the ZeroDivisionError format more flexible

[timokau]

comment:23

Okay I could reproduce the error locally. No idea why I didn't notice that before, I must have messed up somewhere.

Apparently python will interpret those ... at the beginning as line-continuation characters[1]. I've updated the patch with a solution for this, but its not very pretty: Print a leading dummy line.

In my opinion the best solution would be to just remove the backports patch in sage. One patch less is one problem less and this patch seems to bring little to none advantage. But judging by the mailing list discussion, @jdemeyer probably disagrees?

[1] https://stackoverflow.com/questions/5812833/can-i-have-an-ellipsis-at-the-beginning-of-the-line-in-a-python-doctest

---

New commits:

c1efa49

Make the ZeroDivisionError format more flexible

[jdemeyer]

comment:24

Replying to @timokau:

In my opinion the best solution would be to just remove the backports patch in sage.

Why? It's not the fault of Sage that upstream doesn't make a new release (EDIT: actually, that upstream is dead). One could also argue that your distro should just apply that patch to backports.shutil_get_terminal_size since it fixes an actual bug.

One patch less is one problem less and this patch seems to bring little to none advantage.

I consider correctly formatting exceptions a good feature to have.

[timokau]

comment:25

Replying to @jdemeyer:

Replying to @timokau:

In my opinion the best solution would be to just remove the backports patch in sage.

Why? It's not the fault of Sage that upstream doesn't make a new release (EDIT: actually, that upstream is dead). One could also argue that your distro should just apply that patch to backports.shutil_get_terminal_size since it fixes an actual bug.

Its not, but I still think it doesn't matter whose fault it is. Instead we should choose the most practical solution. But by now we're just repeating our arguments -- I'm just going to accept that dropping the patch is not an option.

Yeah the owner hasn't had any GitHub activity in ages. Best case would be if somebody would just fork it. Looks like there is not much maintenance needed besides reviewing the occasional patch. However I don't really feel comfortable running a python library project myself, as I have no knowledge about the ecosystem (setup.py, pypy etc).

One patch less is one problem less and this patch seems to bring little to none advantage.

I consider correctly formatting exceptions a good feature to have.

Its kinda nice but at least in my mind not even remotely worth the trouble. The wrong format still conveys all the same information and doesn't really loose any clarity.

[jdemeyer]

comment:26

Just to talk about something else: it's a bit strange that exactly the same test appears in two places. Maybe the test inside run_cell could be replaced by something not raising exceptions, say

sage: from sage.repl.interpreter import get_test_shell
sage: shell = get_test_shell()
sage: rc = shell.run_cell('2^50')
1125899906842624
sage: rc is None
True
sage: shell.quit()

[kiwifb]

comment:27

Replying to @jdemeyer:

Just to talk about something else: it's a bit strange that exactly the same test appears in two places. Maybe the test inside run_cell could be replaced by something not raising exceptions, say

sage: from sage.repl.interpreter import get_test_shell
sage: shell = get_test_shell()
sage: rc = shell.run_cell('2^50')
1125899906842624
sage: rc is None
True
sage: shell.quit()

Strange that it appears in two places? sage is littered with identical (and I mean identical) tests often in different files. The only thing peculiar about those two is proximity.

[jdemeyer]

comment:28

Replying to @kiwifb:

sage is littered with identical (and I mean identical) tests often in different files.

I know. I think it's because of the "coverage" policy that every function should have tests. If people cannot come up with good tests, they just copy an existing test to satisfy the sage-coverage tool. It's a good example of a well-intentioned tool having unintended side effects.

[sagetrac-git]

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

d24dd41

Make the ZeroDivisionError format more flexible

[sagetrac-git]

Changed commit from c1efa49 to d24dd41

[timokau]

comment:30

Replying to @jdemeyer:

Just to talk about something else: it's a bit strange that exactly the same test appears in two places. Maybe the test inside run_cell could be replaced by something not raising exceptions, say

sage: from sage.repl.interpreter import get_test_shell
sage: shell = get_test_shell()
sage: rc = shell.run_cell('2^50')
1125899906842624
sage: rc is None
True
sage: shell.quit()

I agree, that'll improve the test (doctests with textual output are inherently brittle) and solve half of this problem.

What about the other one? Could we maybe replace that with another exception that is well-behaved, regardless of the backport patch level?

I skimmed through the source and the patch to get an idea, but I can't figure out how the backport even influences the ZeroDivisionError formatting, let alone how the master.patch fixes it.

---

New commits:

d24dd41

Make the ZeroDivisionError format more flexible

---

New commits:

d24dd41

Make the ZeroDivisionError format more flexible

[jdemeyer]

comment:31

Replying to @timokau:

another exception that is well-behaved

The issue is not with the specific exception, but with how IPython formats exceptions in general.

[timokau]

comment:32

Oh so the backport just reports a wrong terminal size and IPython formats the exception to fit into that size? That makes sense.

What do you think about that dummy-line "fix"?

[timokau]

comment:33

@jdemeyer do you think that fix is acceptable?

[jdemeyer]

comment:34

Replying to @timokau:

@jdemeyer do you think that fix is acceptable?

Of course not. As I mentioned on the mailing list, I don't like "fixing" doctests. That's just hiding the problem.

[timokau]

comment:35

Considering what is causing this issue (backports.shutil_get_terminal size failing to do the one thing it is supposed to do), I now agree that the patch should be applied. I will propose it for inclusion in nix.

I still think that the sage doctests shouldn't fail because of this. The "problem" here is not essential to sages functionality. But thats not my call so if you don't want any version of this ticket in sage feel free to close this. If you want this ticket resolved but in a different way, I'll gladly implement any suggestions.

Thanks again for reviewing!

[jdemeyer]

comment:36

I won't actively oppose the patch on this ticket. If other people think it's OK, then I'm fine with it.

Personally, I think you should just add this patch to the sagemath in your distribution.

[vbraun]

Changed branch from u/gh-timokau/zero-division-format to d24dd41

[antonio-rojas]

Changed commit from d24dd41 to none

[antonio-rojas]

comment:39

This is not working for me on Arch: the test is still failing (with unpatched shutil_get_terminal_size)

[kiwifb]

comment:40

Should have reacted earlier, but yes the final version of that ticket didn't fix everything.

sage -t --long /usr/lib64/python2.7/site-packages/sage/repl/interpreter.py
**********************************************************************
File "/usr/lib64/python2.7/site-packages/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)
    <BLANKLINE>
    .../sage/rings/integer.pyx in sage.rings.integer.Integer.__div__ (.../cythonized/sage/rings/integer.c:...)()
       ...        if type(left) is type(right):
       ...            if mpz_sgn((<Integer>right).value) == 0:
    -> ...                  raise ZeroDivisionError("rational division by zero")
       ...            x = <Rational> Rational.__new__(Rational)
       ...            mpq_div_zz(x.value, (<Integer>left).value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
Got:
    dummy line
    <BLANKLINE>
    ZeroDivisionErrorTraceback (most recent call last)
    <ipython-input-1-72ac74c5f414> in <module>()
    ----> 1 Integer(1)/Integer(0)
    <BLANKLINE>
    /usr/lib64/python2.7/site-packages/sage/rings/integer.pyx in sage.rings.integer.Integer.__div__ (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/rings/integer.c:13336)()
       1914         if type(left) is type(right):
       1915             if mpz_sgn((<Integer>right).value) == 0:
    -> 1916                 raise ZeroDivisionError("rational division by zero")
       1917             x = <Rational> Rational.__new__(Rational)
       1918             mpq_div_zz(x.value, (<Integer>left).value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
**********************************************************************

but during development it was - at some point.