Opened 4 years ago

Last modified 6 months ago

#26311 new defect

py3: strange behavior of sleep() on Sage types

Reported by: embray Owned by:
Priority: major Milestone: sage-pending
Component: python3 Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

For some reason, passing Python's time.sleep() a Sage RealLiteral less than 1 returns more-or-less immediately:

sage: x = 0.5
sage: type(x)
<class 'sage.rings.real_mpfr.RealLiteral'>
sage: x2 = float(0.5)
sage: type(x2)
<class 'float'>
sage: timeit('sleep(x)')
625 loops, best of 3: 1 µs per loop
sage: timeit('sleep(x2)')
5 loops, best of 3: 500 ms per loop
sage: # One more time for the folks in back...
sage: timeit('sleep(x)')
625 loops, best of 3: 1.01 µs per loop

It's not just in the context of timeit either. 0.5 is long enough that if you do sleep(x2) directly you can feel the delay, whereas with sleep(x) there is much less or even no perceptible delay.

However,

sage: timeit('sleep(1.0)')
5 loops, best of 3: 1 s per loop

as expected.

The problem appears to stem from the (relatively) new PyTime API, and in particular this function:

static int
_PyTime_FromObject(_PyTime_t *t, PyObject *obj, _PyTime_round_t round,
                   long unit_to_ns)
{
    if (PyFloat_Check(obj)) {
        double d;
        d = PyFloat_AsDouble(obj);
        if (Py_IS_NAN(d)) {
            PyErr_SetString(PyExc_ValueError, "Invalid value NaN (not a number)");
            return -1;
        }
        return _PyTime_FromFloatObject(t, d, round, unit_to_ns);
    }
    else {
        long long sec;
        Py_BUILD_ASSERT(sizeof(long long) <= sizeof(_PyTime_t));

        sec = PyLong_AsLongLong(obj);
        if (sec == -1 && PyErr_Occurred()) {
            if (PyErr_ExceptionMatches(PyExc_OverflowError))
                _PyTime_overflow();
            return -1;
        }

        if (_PyTime_check_mul_overflow(sec, unit_to_ns)) {
            _PyTime_overflow();
            return -1;
        }
        *t = sec * unit_to_ns;
        return 0;
    }
}

So it does not properly handle custom types that implement __float__.

Reported upstream:

In the meantime I don't see that there's much to be done except to always wrap Sage types in float() before passing them to time functions (or, more extreme, provide our own wrappers for common time functions...)

Change History (12)

comment:1 Changed 4 years ago by embray

  • Description modified (diff)

comment:2 Changed 4 years ago by vklein

Thanks for the investigation.

comment:3 Changed 4 years ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:4 Changed 4 years ago by embray

  • Milestone changed from sage-8.5 to sage-8.7

Retargeting some of my tickets (somewhat optimistically for now).

comment:5 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:6 Changed 3 years ago by embray

It feels like there's been a bad habit in CPython lately of needlessly breaking support for custom numerical types.

comment:7 Changed 3 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

comment:8 follow-up: Changed 3 years ago by embray

Yeesh, that thread on bpo is a mess... :(

comment:9 in reply to: ↑ 8 ; follow-ups: Changed 3 years ago by jdemeyer

  • Description modified (diff)

Replying to embray:

Yeesh, that thread on bpo is a mess... :(

Wait until you see the code. DRY clearly does not apply to the CPython code base: I had to make the same fix 3 times.

comment:10 in reply to: ↑ 9 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

Yeesh, that thread on bpo is a mess... :(

Wait until you see the code. DRY clearly does not apply to the CPython code base: I had to make the same fix 3 times.

Have you seen Sage's code base? ;)

comment:11 Changed 3 years ago by embray

  • Milestone changed from sage-8.7 to sage-pending

Removing most of the rest of my open tickets out of the 8.7 milestone, which should be closed.

comment:12 in reply to: ↑ 9 Changed 6 months ago by gh-sheerluck

Replying to jdemeyer:

I had to make the same fix 3 times.

For this ticket 1 time is enough, solved with two line patch:

  • Python/pytime.c

    a b  
    404404}
    405405
    406406static int
    407 _PyTime_FromObject(_PyTime_t *t, PyObject *obj, _PyTime_round_t round,
     407_PyTime_FromObject(_PyTime_t *t, PyObject *xobj, _PyTime_round_t round,
    408408                   long unit_to_ns)
    409409{
     410    PyObject *obj;
     411    obj = PyNumber_Float(xobj);
    410412    if (PyFloat_Check(obj)) {
    411413        double d;
    412414        d = PyFloat_AsDouble(obj);

This patch solves only one thing:

TypeError: 'sage.rings.real_mpfr.RealLiteral' object cannot be interpreted as an integer

and only for time.sleep()

I understand that custom builded python is not for everyone.

Note: See TracTickets for help on using tickets.