#28622 closed defect (fixed)
Patchbot and Python 3 doctest failures
Reported by:  jhpalmieri  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  python3  Keywords:  python3 
Cc:  Merged in:  
Authors:  John Palmieri  Reviewers:  Frédéric Chapoton, Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  56e1642 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
With a Python 3 patchbot and a Ubuntu 18.04 computer running make ptestlong
, there are doctest failures in three files:
sage t long src/sage/repl/attach.py # 4 doctests failed sage t long src/sage/libs/singular/function.pyx # 1 doctest failed sage t long src/sage/numerical/backends/generic_backend.pyx # 2 doctests failed
This can be replicated by hand. cd to SAGE_ROOT for a Sage built with Python 3, run Python, and do
>>> import subprocess, os, sys >>> tee = subprocess.Popen(["tee", 'LOGFILE.log'], stdin=subprocess.PIPE) >>> os.dup2(tee.stdin.fileno(), sys.stdout.fileno()) >>> os.dup2(tee.stdin.fileno(), sys.stderr.fileno()) >>> os.system('./sage tp src/sage/repl/attach.py src/sage/libs/singular/function.pyx src/sage/numerical/backends/generic_backend.pyx')
Note that in a fresh Python session, the following does not lead to errors:
>>> import os >>> os.system('./sage tp src/sage/repl/attach.py src/sage/libs/singular/function.pyx src/sage/numerical/backends/generic_backend.pyx')
Change History (29)
comment:1 Changed 2 years ago by
comment:2 Changed 2 years ago by
 Component changed from PLEASE CHANGE to python3
 Type changed from PLEASE CHANGE to defect
comment:3 Changed 2 years ago by
A quicker way to get the failures:
>>> from sage_patchbot.patchbot import Tee >>> with Tee('/path/to/logfile', time=True, timeout=40): ... os.system('/path/to/SAGE_ROOT/sage tp 3 long src/sage/repl/attach.py src/sage/numerical/backends/generic_backend.pyx src/sage/libs/singular/function.pyx')
comment:4 Changed 2 years ago by
 Description modified (diff)
comment:5 Changed 2 years ago by
Could the problem be the inheritability (or not) of the file descriptors in Tee
? In any case, in sage_patchbot.patchbot
, if I comment out these lines, the doctests don't fail:

sage_patchbot/patchbot.py
diff git a/sage_patchbot/patchbot.py b/sage_patchbot/patchbot.py index b411c7c..2994db1 100755
a b class Tee(object): 142 142 self._saved = os.dup(sys.stdout.fileno()), os.dup(sys.stderr.fileno()) 143 143 self.tee = subprocess.Popen(["tee", self.filepath], 144 144 stdin=subprocess.PIPE) 145 os.dup2(self.tee.stdin.fileno(), sys.stdout.fileno())146 os.dup2(self.tee.stdin.fileno(), sys.stderr.fileno())145 # os.dup2(self.tee.stdin.fileno(), sys.stdout.fileno()) 146 # os.dup2(self.tee.stdin.fileno(), sys.stderr.fileno()) 147 147 if self.time: 148 148 print(now_str()) 149 149 self.start_time = time.time()
Of course also the log file doesn't get written.
comment:6 Changed 2 years ago by
 Description modified (diff)
comment:7 Changed 2 years ago by
 Description modified (diff)
comment:8 Changed 2 years ago by
Could there be some connection to the use of Python's traceback
module? I'm trying to find common threads among the three files with failures, and I'm not getting very far, but if I make this change, then tests pass in repl/attach.py
:

src/sage/repl/attach.py
diff git a/src/sage/repl/attach.py b/src/sage/repl/attach.py index c350ec33af..e1c98e32ad 100644
a b characterbycharacter:: 36 36 sage: try: 37 37 ....: attach(src) 38 38 ....: except Exception: 39 ....: traceback.print_exc()39 ....: raise 40 40 Traceback (most recent call last): 41 41 ... 42 42 exec(preparse_file(f.read()) + "\n", globals) … … characterbycharacter:: 48 48 sage: try: 49 49 ....: attach(src) 50 50 ....: except Exception: 51 ....: traceback.print_exc()51 ....: raise 52 52 Traceback (most recent call last): 53 53 ... 54 54 exec(code, globals)
comment:9 Changed 2 years ago by
The following change to repl/attach.py
is almost good enough:

src/sage/repl/attach.py
diff git a/src/sage/repl/attach.py b/src/sage/repl/attach.py index c350ec33af..bdfaad4f7f 100644
a b Check that no file clutter is produced:: 20 20 ['foobar.sage'] 21 21 sage: detach(src) 22 22 23 In debug mode backtraces contain code snippets. We need to manually 24 print the traceback because the python doctest module has special 25 support for exceptions and does not match them 26 characterbycharacter:: 23 Check printing of code snippets in debug mode:: 27 24 28 sage: import traceback29 25 sage: with open(src, 'w') as f: 30 26 ....: _ = f.write('# first line\n') 31 27 ....: _ = f.write('# second line\n') … … characterbycharacter:: 33 29 ....: _ = f.write('# fourth line\n') 34 30 35 31 sage: load_attach_mode(attach_debug=False) 36 sage: try: 37 ....: attach(src) 38 ....: except Exception: 39 ....: traceback.print_exc() 32 sage: attach(src) 40 33 Traceback (most recent call last): 41 34 ... 42 35 exec(preparse_file(f.read()) + "\n", globals) … … characterbycharacter:: 45 38 sage: detach(src) 46 39 47 40 sage: load_attach_mode(attach_debug=True) 48 sage: try: 49 ....: attach(src) 50 ....: except Exception: 51 ....: traceback.print_exc() 41 sage: attach(src) 52 42 Traceback (most recent call last): 53 43 ... 54 44 exec(code, globals)
The problem is that it would allow some doctests to pass that should fail. From the file:
sage: with open(src, 'w') as f: ....: _ = f.write('# first line\n') ....: _ = f.write('# second line\n') ....: _ = f.write('raise ValueError("third") # this should appear in the source snippet\n') ....: _ = f.write('# fourth line\n')
Then this passes, as it should:
sage: load_attach_mode(attach_debug=True) sage: attach(src) Traceback (most recent call last): ... exec(code, globals) File ".../foobar.sage....py", line ..., in <module> raise ValueError("third") # this should appear in the source snippet ValueError: third
But this also passes: note that the second to last line is different:
sage: load_attach_mode(attach_debug=True) sage: attach(src) Traceback (most recent call last): ... exec(code, globals) File ".../foobar.sage....py", line ..., in <module> raise ValueError("third") # this is gibberish ValueError: third
I guess this is what is meant by "We need to manually print the traceback because ..."
comment:10 Changed 2 years ago by
Nice to see that you are making progress. Sorry that I cannot help.
comment:11 Changed 2 years ago by
 Description modified (diff)
comment:12 Changed 2 years ago by
FWIW, I systematically get these doctest failures outside the patchbot framework, namely on a Ubuntu 18.04 computer when doing make ptestlong
, cf. https://groups.google.com/d/msg/sagerelease/yFwTkcr5AVY/ifqU73ZAAAJ
comment:13 Changed 2 years ago by
Couldn't some of them be yet another cases of "flush missing", as the previous py3 problems that were fixed ? In the repl_attach case, the correct answer seems to appear in the next doctest.
comment:14 Changed 2 years ago by
Interesting question. If I add some sys.stdout.flush()
commands, then I can get the repl/attach.py
doctests to pass when run as in the ticket description (using tee and output redirection), but then doctests fail when run the usual way. I'll keep experimenting.
comment:15 Changed 2 years ago by
 Branch set to u/jhpalmieri/flushing
comment:16 followup: ↓ 19 Changed 2 years ago by
 Commit set to 6ade77020fca15a58b5a37eeffdcaf8f2ca1a91c
Okay, here is an attempt at a fix. It fixes the particular problem in the ticket description, at least for me on OS X. Eric, does it help with your situation?
Should I mark it as "needs review", or should some explanatory comments be added? Or is it even the correct approach?
New commits:
6ade770  trac 28622: flush or redirect some output

comment:17 Changed 2 years ago by
This looks good enough for me, even if this is maybe more fixing symptoms than illness.
I will launch my atlas patchbot on the branch.
comment:18 Changed 2 years ago by
This seems to work with python2 and to fix the python3 issues also on Ubuntu patchbot. I am extremely tempted to give a positive review.
comment:19 in reply to: ↑ 16 ; followup: ↓ 20 Changed 2 years ago by
Replying to jhpalmieri:
Okay, here is an attempt at a fix. It fixes the particular problem in the ticket description, at least for me on OS X. Eric, does it help with your situation?
I am just running make ptestlong
and shall report soon...
comment:20 in reply to: ↑ 19 Changed 2 years ago by
Replying to egourgoulhon:
Replying to jhpalmieri:
Okay, here is an attempt at a fix. It fixes the particular problem in the ticket description, at least for me on OS X. Eric, does it help with your situation?
I am just running
make ptestlong
and shall report soon...
Here is the report from my Ubuntu 18.04 computer:
 sage t long warnlong 53.0 src/sage/rings/polynomial/polynomial_rational_flint.pyx # 1 doctest failed  Total time for all tests: 1357.6 seconds
In other words, all the doctests discussed here are passed! The only failure is that discussed in #28334.
So +1 for the positive review.
comment:21 Changed 2 years ago by
 Status changed from new to needs_review
ok, so let us first turn to "needs review"
comment:22 Changed 2 years ago by
John, do you agree that this could be set to positive ?
comment:23 Changed 2 years ago by
It’s okay with me.
comment:24 Changed 2 years ago by
 Keywords python3 added
 Reviewers set to Frédéric Chapoton, Eric Gourgoulhon
 Status changed from needs_review to positive_review
Good. I am setting to positive.
comment:25 followup: ↓ 27 Changed 2 years ago by
Shouldn't we add reference to this ticket to the added lines of code to avoid inadvertent removal ?
comment:26 Changed 2 years ago by
 Commit changed from 6ade77020fca15a58b5a37eeffdcaf8f2ca1a91c to 56e1642222b6afd0e35c53458110ee4fc31e7453
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
56e1642  trac 28622: add ticket reference when we use stdout.flush()

comment:27 in reply to: ↑ 25 Changed 2 years ago by
 Status changed from needs_review to positive_review
Replying to dcoudert:
Shouldn't we add reference to this ticket to the added lines of code to avoid inadvertent removal ?
Done. I'm setting back to positive review, also.
comment:28 Changed 2 years ago by
 Branch changed from u/jhpalmieri/flushing to 56e1642222b6afd0e35c53458110ee4fc31e7453
 Resolution set to fixed
 Status changed from positive_review to closed
comment:29 Changed 16 months ago by
 Commit 56e1642222b6afd0e35c53458110ee4fc31e7453 deleted
 Reviewers changed from Frédéric Chapoton, Eric Gourgoulhon to Frédéric Chapoton, Eric Gourgoulhon
The failures:
and
and