Opened 7 months ago

Closed 6 months ago

#27383 closed defect (duplicate)

py3: fix output buffering issues in eclib

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: python3 Keywords:
Cc: chapoton, cremona Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

There are doctest failures under Python 3 in eclib due to output from eclib being buffered instead of printed.

Python 2 uses the standard C library I/O functions from <stdio.h>. Typically, the C library implements buffering. Now Python 3 bypasses the C library for I/O and does direct system calls from <unistd.h>. The doctest framework always flushes the Python buffers after every test. Under Python 2, this actually flushes the <stdio.h> buffer which is the same buffer used by eclib (and other C/C++ libraries). But on Python 3, this only flushes the Python-specific buffer but not the <stdio.h> buffer.

Change History (7)

comment:1 follow-up: Changed 7 months ago by jdemeyer

It would be possible to add flushing functions in eclib itself but that does not seem the right approach to me.

I would argue that eclib should actually flush whenever it's "done" producing output. Note that << endl flushes the output, so this is probably a matter of replacing some << "\n" by << endl in eclib, in particular in mw::process(). Alternatively, you could add an explicit cout.flush().

Note that flushing too often can degrade performance. But I guess that I/O performance is not an issue for eclib (you aren't producing megabytes of output per second, right?). So then there is little reason to not use << endl.

Last edited 7 months ago by jdemeyer (previous) (diff)

comment:2 in reply to: ↑ 1 ; follow-ups: Changed 7 months ago by cremona

Replying to jdemeyer:

It would be possible to add flushing functions in eclib itself but that does not seem the right approach to me.

I would argue that eclib should actually flush whenever it's "done" producing output. Note that << endl flushes the output, so this is probably a matter of replacing some << "\n" by << endl in eclib, in particular in mw::process(). Alternatively, you could add an explicit cout.flush().

Note that flushing too often can degrade performance. But I guess that I/O performance is not an issue for eclib (you aren't producing megabytes of output per second, right?). So then there is little reason to not use << endl.

Certainly I understand all that. The actual programs in eclib do this, but not all the library functions. I can work on that but surely (1) the recent eclib version can be merged without waiting for yet another one, and (2) the flushing of all output in doctests as proposed in this ticket should be done anyway?

comment:3 in reply to: ↑ 2 ; follow-up: Changed 7 months ago by jdemeyer

Indeed, this shouldn't require a new eclib release.

Concretely for this ticket, I would propose something like

  • src/sage/libs/eclib/mwrank.pyx

    diff --git a/src/sage/libs/eclib/mwrank.pyx b/src/sage/libs/eclib/mwrank.pyx
    index 5625153..e9d5b90 100644
    a b from __future__ import print_function, absolute_import 
    2323import os
    2424import sys
    2525
     26from libc.stdio cimport fflush, stdout
    2627from cysignals.memory cimport sig_free
    2728from cysignals.signals cimport sig_on, sig_off
    2829
    cdef class _mw: 
    726727        sig_on()
    727728        x,y,z = _bigint(point[0]), _bigint(point[1]), _bigint(point[2])
    728729        r = mw_process(self.curve, self.x, x.x, y.x, z.x, sat)
     730        fflush(stdout)
    729731        sig_off()
    730732        if r != 0:
    731733            raise ArithmeticError("point (=%s) not on curve." % point)

Maybe this needs to be done in a few more places, but I really have no time to work on this now.

comment:4 in reply to: ↑ 2 Changed 7 months ago by jdemeyer

Replying to cremona:

the flushing of all output in doctests as proposed in this ticket should be done anyway?

I don't think that the doctest framework is the right place to fix this. We want to fix this also in normal interactive usage, not just in doctests.

comment:5 in reply to: ↑ 3 Changed 7 months ago by cremona

Replying to jdemeyer:

Indeed, this shouldn't require a new eclib release.

Concretely for this ticket, I would propose something like

  • src/sage/libs/eclib/mwrank.pyx

    diff --git a/src/sage/libs/eclib/mwrank.pyx b/src/sage/libs/eclib/mwrank.pyx
    index 5625153..e9d5b90 100644
    a b from __future__ import print_function, absolute_import 
    2323import os
    2424import sys
    2525
     26from libc.stdio cimport fflush, stdout
    2627from cysignals.memory cimport sig_free
    2728from cysignals.signals cimport sig_on, sig_off
    2829
    cdef class _mw: 
    726727        sig_on()
    727728        x,y,z = _bigint(point[0]), _bigint(point[1]), _bigint(point[2])
    728729        r = mw_process(self.curve, self.x, x.x, y.x, z.x, sat)
     730        fflush(stdout)
    729731        sig_off()
    730732        if r != 0:
    731733            raise ArithmeticError("point (=%s) not on curve." % point)

Maybe this needs to be done in a few more places, but I really have no time to work on this now.

Thanks for those hints. I'll see if I can make that work.

comment:6 Changed 6 months ago by jhpalmieri

Is this now fixed by #27360? With that ticket, Python 3 tests pass for me in libs/eclib.

comment:7 Changed 6 months ago by jdemeyer

  • Milestone changed from sage-8.7 to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.