Opened 2 years ago
Closed 2 years ago
#28649 closed defect (fixed)
py3: flush output from FLINT error message
Reported by:  jhpalmieri  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  python3  Keywords:  
Cc:  chapoton, dimpase, egourgoulhon  Merged in:  
Authors:  John Palmieri  Reviewers:  Dima Pasechnik, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  9957855 (Commits, GitHub, GitLab)  Commit:  99578558953d5fde55127dddbeceb86d33c395e0 
Dependencies:  Stopgaps: 
Description (last modified by )
This is a second fix for the problem reported at #28334, the doctest failure in rings/polynomial/polynomial_rational_flint.pyx
. This fix does not rely on patching FLINT. See sagedevel discussion.
Change History (24)
comment:1 Changed 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
 Branch set to u/jhpalmieri/flushflintmsg
comment:3 Changed 2 years ago by
 Commit set to cd89b9d5a60a5e32ea3e6cafcfe5ed8d44ceaf96
 Status changed from new to needs_review
New commits:
cd89b9d  trac 28649: flush C buffer

comment:4 Changed 2 years ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
good, thanks. Takes extra 0.5 sec on my laptop, but OK...
comment:5 Changed 2 years ago by
 Status changed from positive_review to needs_info
It may be possible to do this crossplatform in a relatively convenient way with ctypes anyway:
import ctypes libc = ctypes.CDLL(ctypes.util.find_library("c")) libc.fflush(0r)
seems to work, and looking at the source, it seems "find_library" does make an effort to be crossplatform. It's probably worth considering if this will do, since it's a lot lighter weight than running cython.
comment:6 Changed 2 years ago by
we can just add this flush function somewher into sagelib
comment:7 Changed 2 years ago by
Using Sage's own misc.compat.find_library
would probably be better:

src/sage/rings/polynomial/polynomial_rational_flint.pyx
diff git a/src/sage/rings/polynomial/polynomial_rational_flint.pyx b/src/sage/rings/polynomial/polynomial_rational_flint.pyx index 73d1992656..aeb5a824e3 100644
a b cdef class Polynomial_rational_flint(Polynomial): 1176 1176 ... 1177 1177 RuntimeError: FLINT exception 1178 1178 1179 Flush the output buffer to get rid of stray output  see 1180 :trac:`28649`:: 1181 1182 sage: from sage.misc.compat import find_library 1183 sage: import ctypes 1184 sage: libc = ctypes.CDLL(find_library("c")) 1185 sage: libc.fflush(0r) 1186 ... 1187 1179 1188 Test fractional powers (:trac:`20086`):: 1180 1189 1181 1190 sage: P.<R> = QQ[]
comment:8 Changed 2 years ago by
Both approaches work for me on OS X and on my Ubuntu virtual machine. Should they both work on Cygwin?
comment:9 Changed 2 years ago by
It looks like
libc = ctypes.CDLL(None)
works too. I have no idea why. (see https://eli.thegreenplace.net/2015/redirectingallkindsofstdoutinpython/)
comment:10 Changed 2 years ago by
Does https://stackoverflow.com/questions/49878901/howdoesctypescdllloadlibrarynonework help? I'm worried about this on Cygwin, though.
comment:11 Changed 2 years ago by
Any opinions on which approach to take? I see three options:
 Keep the current branch. Slow, requires a functioning Cython compiler. (Is that guaranteed with binary releases?)
 Add the
cyflush
function to the Sage library so that it is precompiled.
 Take the
ctypes
approach from comment:17, withlibc = ctypes.CDLL(None)
.
Any opinions? I would like to get a solution to this problem merged, because there are people out there using nonSage versions of FLINT.
comment:12 Changed 2 years ago by
I think 2 is the easiest. I don't see any benefits of using ctypes
here, whatsoever.
comment:13 Changed 2 years ago by
Any suggestions for where that function should go? polynomial_rational_flint.pyx
is one option, since it will be used there, but that's not a particularly natural place for anyone to find it later.
comment:14 Changed 2 years ago by
src/sage/misc/pager.py ?
comment:15 Changed 2 years ago by
or maybe misc/sage_ostools.pyx
comment:16 Changed 2 years ago by
I'm leaning toward either misc/misc_c.pyx or a new file cpython/cyflush.pyx.
comment:17 Changed 2 years ago by
 Commit changed from cd89b9d5a60a5e32ea3e6cafcfe5ed8d44ceaf96 to 461e0178180e869801ea2ed869c1647975701c73
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
461e017  trac 28649: flush stray output from external library calls

comment:19 Changed 2 years ago by
 Commit changed from 461e0178180e869801ea2ed869c1647975701c73 to bc01c65367345d5a8ff8258731978a4708970ded
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bc01c65  trac 28649: flush stray output from external library calls

comment:20 followup: ↓ 22 Changed 2 years ago by
the EXAMPLES:: content in cyflush is overindented.
comment:21 Changed 2 years ago by
 Commit changed from bc01c65367345d5a8ff8258731978a4708970ded to 99578558953d5fde55127dddbeceb86d33c395e0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9957855  trac 28649: flush stray output from external library calls

comment:22 in reply to: ↑ 20 Changed 2 years ago by
comment:23 Changed 2 years ago by
 Reviewers changed from Dima Pasechnik to Dima Pasechnik, Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, good to go
comment:24 Changed 2 years ago by
 Branch changed from u/jhpalmieri/flushflintmsg to 99578558953d5fde55127dddbeceb86d33c395e0
 Resolution set to fixed
 Status changed from positive_review to closed
As I said at #28334, the fix there can be kept independently of the change here, and it should be kept, since the patch to FLINT was accepted upstream. The one here is to handle the case when using nonpatched versions of FLINT.