Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#26314 closed enhancement (fixed)

py3: Fix interfaces module for python3

Reported by: vklein Owned by:
Priority: major Milestone: sage-8.5
Component: python3 Keywords:
Cc: embray, jdemeyer Merged in:
Authors: Vincent Klein Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 4417bbb (Commits) Commit: 4417bbba102625f78452f6389e56b322e6abcf80
Dependencies: Stopgaps:

Description (last modified by vklein)

There is still two remaining problems :

  • I temporary used sleep(float(0.5)) to fix a doctest but it can't be a good solution. The problem is using sleep(0.5) doesn't pause the thread 500 ms with python3 :
sage: time sleep(0.5)
CPU times: user 48 µs, sys: 0 ns, total: 48 µs
Wall time: 52.2 µs
sage: time sleep(1)
CPU times: user 468 µs, sys: 8 µs, total: 476 µs
Wall time: 1 s

Does somebody knows if there is a ticket related to this problem or if i should open a new one ?

  • In pexpect.py the doctest
    sage: w = walltime(t); w > 0.4 and w < 10
    True
    

fails in python3 because the walltime is shorter than 0.4. I don't know why it should be considered as an error or if it is expected with python3 for some reason.

Change History (22)

comment:1 Changed 5 months ago by vklein

  • Branch set to u/vklein/26314

comment:2 Changed 5 months ago by vklein

  • Commit set to 91bdf2a370a018c54dfd9762f6263f8147f938e9
  • Description modified (diff)
  • Status changed from new to needs_info

New commits:

91bdf2aTrac #26314 : module interface, fix code and doctest for...

comment:3 Changed 5 months ago by vklein

  • Summary changed from py3: Fix interfaces modules for python3 to py3: Fix interfaces module for python3

comment:4 Changed 5 months ago by chapoton

see #26311 for the sleep problem

comment:5 Changed 5 months ago by vklein

Thanks !

comment:6 Changed 5 months ago by vklein

  • Status changed from needs_info to needs_review

comment:7 Changed 5 months ago by vklein

This ticket don't manage the walltime case.

comment:8 Changed 5 months ago by git

  • Commit changed from 91bdf2a370a018c54dfd9762f6263f8147f938e9 to 8f35e935943e0081980d8e07656fe2896acdcb1d

Branch pushed to git repo; I updated commit sha1. New commits:

8f35e93Trac #26314: Remove some unused import and variable.

comment:9 Changed 5 months ago by vklein

Remove some unused variable to make pyflakes more happy (AsciiArtString is needed by macaulay2.py).

Last edited 5 months ago by vklein (previous) (diff)

comment:10 Changed 5 months ago by chapoton

  • Cc embray jdemeyer added

@embray and @jdemeyer, any opinion on the simple changes proposed here ?

comment:11 Changed 5 months ago by vklein

... or an opinion about the "walltime" test ...

comment:12 follow-up: Changed 5 months ago by embray

For cases like:

  • src/sage/interfaces/sagespawn.pyx

    diff --git a/src/sage/interfaces/sagespawn.pyx b/src/sage/interfaces/sagespawn.pyx
    index 9b44d3a..1600d70 100644
    a b class SageSpawn(spawn): 
    145145            sage: from sage.interfaces.sagespawn import SageSpawn
    146146            sage: E = SageSpawn("sh", ["-c", "echo hello world"])
    147147            sage: _ = E.expect_peek("w")
    148             sage: E.read()
     148            sage: E.read() # py2
    149149            'hello world\r\n'
     150            sage: E.read() # py3
     151            b'hello world\r\n'

where Python 3 returns bytes, instead of making two separate cases, in the past I have done something like print(E.read().decode('ascii')). This will have the same result in both cases.

I wonder if Sage shouldn't include its own sleep() built-in that isn't broken :/

comment:13 in reply to: ↑ 12 Changed 5 months ago by vklein

Replying to embray:

where Python 3 returns bytes, instead of making two separate cases, in the past I have done something like print(E.read().decode('ascii')). This will have the same result in both cases.

I know, i hesitated between the two syntax and, as it is an EXAMPLES::, have think that the # py2-# py3 syntax might be clearer for the end-user.

I don't have a strong opinion on it, tell me what you prefer. If the past cases you talk about are also EXAMPLES:: we should stick with the decode('ascii') solution for consistency.

Last edited 5 months ago by vklein (previous) (diff)

comment:14 Changed 5 months ago by embray

Yes, I think consistency is best. There are a few other cases that also, rather than relying on the repr, just do like

sage: E.read() == b'Hello\r\n'
True

or whatever. This will also work the same on Python 2 and 3.

I have thought of adding a helper to the test framework to deal with b-prefixed strings as it does with u-strings, but it turns out they are relatively uncommon in the majority of the test suite.

comment:15 Changed 5 months ago by git

  • Commit changed from 8f35e935943e0081980d8e07656fe2896acdcb1d to 4417bbba102625f78452f6389e56b322e6abcf80

Branch pushed to git repo; I updated commit sha1. New commits:

4417bbbTrac #26314 : replace '# py2 - # py3' with ...

comment:16 Changed 5 months ago by vklein

# py2-# py3 cases replaced using decode(ascii) syntax.

comment:17 follow-up: Changed 5 months ago by chapoton

probably you do not need the initial u that you add to some doctests.

comment:18 in reply to: ↑ 17 Changed 5 months ago by embray

Replying to chapoton:

probably you do not need the initial u that you add to some doctests.

I think it is needed because on Python 2 .decode(...) will return a unicode string literal, and the doctest parser does treat this as relevant on Python 2. The only thing it does w.r.t. u-prefixes is on Python 3 it strips them from the expected output.

I usually avoid the issue by using print(...) instead, but i think this is fine as-is.

comment:19 Changed 5 months ago by vklein

It is as Eric says, these tests as they are won't work on py2 without the initial u

comment:20 Changed 4 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let it be

comment:21 Changed 4 months ago by vbraun

  • Branch changed from u/vklein/26314 to 4417bbba102625f78452f6389e56b322e6abcf80
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 4 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

This should be re-targeted for 8.5.

Note: See TracTickets for help on using tickets.