#28649 closed defect (fixed)

py3: flush output from FLINT error message

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-9.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) Commit: 99578558953d5fde55127dddbeceb86d33c395e0
Dependencies: Stopgaps:

Description (last modified by jhpalmieri)

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 sage-devel discussion.

Change History (24)

comment:1 Changed 12 months ago by jhpalmieri

  • Description modified (diff)

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 non-patched versions of FLINT.

comment:2 Changed 12 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/flush-flint-msg

comment:3 Changed 12 months ago by jhpalmieri

  • Commit set to cd89b9d5a60a5e32ea3e6cafcfe5ed8d44ceaf96
  • Status changed from new to needs_review

New commits:

cd89b9dtrac 28649: flush C buffer

comment:4 Changed 12 months ago by dimpase

  • 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 12 months ago by nbruin

  • Status changed from positive_review to needs_info

It may be possible to do this cross-platform 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 cross-platform. It's probably worth considering if this will do, since it's a lot lighter weight than running cython.

comment:6 Changed 12 months ago by dimpase

we can just add this flush function somewher into sagelib

comment:7 Changed 12 months ago by jhpalmieri

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): 
    11761176            ...
    11771177            RuntimeError: FLINT exception
    11781178
     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
    11791188        Test fractional powers (:trac:`20086`)::
    11801189
    11811190            sage: P.<R> = QQ[]

comment:8 Changed 12 months ago by jhpalmieri

Both approaches work for me on OS X and on my Ubuntu virtual machine. Should they both work on Cygwin?

Last edited 12 months ago by jhpalmieri (previous) (diff)

comment:9 Changed 12 months ago by nbruin

It looks like

libc = ctypes.CDLL(None)

works too. I have no idea why. (see https://eli.thegreenplace.net/2015/redirecting-all-kinds-of-stdout-in-python/)

comment:10 Changed 12 months ago by jhpalmieri

comment:11 Changed 12 months ago by jhpalmieri

Any opinions on which approach to take? I see three options:

  1. Keep the current branch. Slow, requires a functioning Cython compiler. (Is that guaranteed with binary releases?)
  1. Add the cyflush function to the Sage library so that it is precompiled.
  1. Take the ctypes approach from comment:17, with libc = ctypes.CDLL(None).

Any opinions? I would like to get a solution to this problem merged, because there are people out there using non-Sage versions of FLINT.

comment:12 Changed 12 months ago by dimpase

I think 2 is the easiest. I don't see any benefits of using ctypes here, whatsoever.

comment:13 Changed 12 months ago by jhpalmieri

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 12 months ago by chapoton

src/sage/misc/pager.py ?

comment:15 Changed 12 months ago by chapoton

or maybe misc/sage_ostools.pyx

comment:16 Changed 12 months ago by jhpalmieri

I'm leaning toward either misc/misc_c.pyx or a new file cpython/cyflush.pyx.

comment:17 Changed 12 months ago by git

  • Commit changed from cd89b9d5a60a5e32ea3e6cafcfe5ed8d44ceaf96 to 461e0178180e869801ea2ed869c1647975701c73

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

461e017trac 28649: flush stray output from external library calls

comment:18 Changed 12 months ago by jhpalmieri

  • Status changed from needs_info to needs_review

Let's try this.

comment:19 Changed 12 months ago by git

  • Commit changed from 461e0178180e869801ea2ed869c1647975701c73 to bc01c65367345d5a8ff8258731978a4708970ded

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

bc01c65trac 28649: flush stray output from external library calls

comment:20 follow-up: Changed 12 months ago by chapoton

the EXAMPLES:: content in cyflush is overindented.

comment:21 Changed 12 months ago by git

  • Commit changed from bc01c65367345d5a8ff8258731978a4708970ded to 99578558953d5fde55127dddbeceb86d33c395e0

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

9957855trac 28649: flush stray output from external library calls

comment:22 in reply to: ↑ 20 Changed 12 months ago by jhpalmieri

Replying to chapoton:

the EXAMPLES:: content in cyflush is overindented.

Oops, sorry. Fixed.

comment:23 Changed 12 months ago by chapoton

  • 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 12 months ago by vbraun

  • Branch changed from u/jhpalmieri/flush-flint-msg to 99578558953d5fde55127dddbeceb86d33c395e0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.