Python3 #13

videlec · 2017-04-29T10:17:49Z

This branch is based on top of doctests. I had to make a little hack to make the test pass, namely providing the option IGNORE_EXCEPTION_DETAIL to doctest. There is something fishy going on here.

defeo

Hello,

I agree with most of the changes. In particular, the Cython macros look ok to me; I have no better idea anyway.

Ccan you please just address the 4 comments I made?

defeo · 2017-05-12T14:36:37Z

cypari2/closure.pyx

+    Examples:
+
+    >>> from cypari2.closure import objtoclosure
+    >>> def pymul(i,j): return i*j


Why this change? I think that was meant explicitly to test lambdas.

lambda functions behavior are different between Python2 and Python3. In Python3, a lambda function has only one argument.

?? What do you mean? I'm using Python 3.6, and I see no problem with the old test

>>> objtoclosure(lambda i,j: i*j) (v1,v2,v3,v4,v5)->call_python(v1,v2,v3,v4,v5,140180350752424)

Indeed. I do not know what happend in the first place. This is back with 418cf40.

defeo · 2017-05-12T14:39:26Z

cypari2/convert.pyx

+    5
+    >>> gen_to_integer(pari("Pol(42)"))
+    42
+    >>> gen_to_integer(pari("u"))


So, did you get to the bottom of this? Why was it failing on Travis?

No idea. This is still failing on travis. However the option IGNORE_EXCEPTION_DETAIL in doctests make this failure disappear.

We may at least want to open an issue for this, or we may forget about it.

defeo · 2017-05-12T14:41:36Z

cypari2/convert.pyx

@@ -589,7 +615,7 @@ cpdef gen_to_python(Gen z):
    elif t == t_VEC or t == t_COL:
        return [gen_to_python(x) for x in z.python_list()]
    elif t == t_VECSMALL:
-        return z.python_list_small()
+       return z.python_list_small()


This diff is weird!

Indeed! Fixed by d7b7a3e

defeo · 2017-05-12T14:48:05Z

cypari2/pari_instance.pyx

-
-        EXAMPLES::
-
-            sage: pari.debugstack()  # random


Isn't there a way to leave the example undoctested?

No without a hook to doctest. This is what they did in cypari and what we might want to do... (they also have a hook for 32 vs 64 bits doctests)

All right, we can think about this later.

jdemeyer · 2017-05-18T15:22:25Z

It's wrong to assume that every string coming from PARI is ASCII. I mean, people should be able to write PARI/GP code like print("Author: Jean-François Mestre").
When in doubt, use sys.getfilesystemencoding() which is what Python uses by default.

I also don't agree with changing <bytes?> to <bytes>: it's an extra safety check.

jdemeyer · 2017-05-18T15:29:05Z

This is also wrong, because you aren't calling str() anymore:

{name} = {name}.encode('ascii') if isinstance({name}, str) else bytes({name})

This should be something like

{name} = str({name})
if PYTHON3:
    {name} = {name}.encode(filesystemencoding)

videlec · 2017-06-07T06:46:01Z

Changes

removing system.pxi and use compile_time_env in cythonization
functions to_bytes and to_string to avoid repetitions

Travis looks happy.

videlec · 2017-06-15T14:21:24Z

ping!

jdemeyer · 2017-06-16T08:23:40Z

I don't think it is good style to hard-code the Python version number in the .c sources. Better make it a run-time check.

jdemeyer · 2017-06-16T08:27:09Z

Some more docs for to_string would be welcome. What do you mean with "string" in this context? Do you mean the str type (which is bytes in Python 2 and unicode in Python 3)?

If so, it seems more reasonable to have to_bytes and to_unicode functions, where to_string = to_bytes on Python 2 and to_string = to_unicode on Python 3. That would simplify things.

Also: what is acceptable input for to_string? Both bytes and unicode on Python 2 and Python 3?

videlec · 2017-06-16T08:28:35Z

@jdemeyer concerning your harceded version, what is the point? and what are you referring to exactly? Note that only the major version (2 or 3) is taken into account. And a Python2 compiled version will not be Python3 compatible. And what is happening is during cythonization, it is not hardcoded in the compiled version.

jdemeyer · 2017-06-16T08:31:45Z

concerning your harceded version, what is the point?

You make the assumption that the version of Python which is used by Cython equals the version of Python which will use cypari2. There is no need to make that assumption.

Not making this assumption could allow to install cypari2 on systems which do not have Cython installed.

videlec · 2017-06-16T08:35:14Z

concerning your harceded version, what is the point?

You make the assumption that the version of Python which is used by Cython equals the version of Python which will use cypari2. There is no need to make that assumption.

Agreed. But the target Python version has to be known at compilation time anyway (e.g. __cmp__ should not be there in Python3). How would you pass such parameter to the setup script?

Not making this assumption could allow to install cypari2 on systems which do not have Cython installed.

Then you would need two versions of the C files. One Python2 compatible and one Python3 compatible.

I don't think this point is relevant to the current PR.

jdemeyer · 2017-06-16T08:38:02Z

e.g. cmp should not be there in Python3

That's for Cython to decide, not for us. If you write def __cmp__ in an extension type, Cython knows to ignore that on Python 3.

Cython tries hard to output .c code that is compatible with both Python 2 and Python 3. Let's not destroy that by hard-coding the Python version number in those conversion functions.

videlec · 2017-06-16T12:05:05Z

Funny cython behavior... with

IF PY_MAJOR_VERSION == 2:
    return to_bytes(s)
ELSE:
    return to_unicode(s)

I got with python2

Error compiling Cython file:
------------------------------------------------------------
...
    True
    >>> s1 == s2 == s3 == 'hello'
    True
    """
    IF PY_MAJOR_VERSION == 2:
        return to_bytes(s)
                      ^
------------------------------------------------------------    
cypari2/string_utils.pyx:71:23: Cannot convert 'bytes' object to str implicitly. This is not portable to Py3.

and with Python3

Error compiling Cython file:
------------------------------------------------------------
...
    True
    """
    IF PY_MAJOR_VERSION == 2:
        return to_bytes(s)
    ELSE:
        return to_unicode(s)
                        ^
------------------------------------------------------------    
cypari2/string_utils.pyx:73:25: Cannot convert Unicode string to 'str' implicitly. This is not portable and requires explicit encoding.
Traceback (most recent call last):

videlec · 2017-06-16T13:33:47Z

e.g. cmp should not be there in Python3

That's for Cython to decide, not for us. If you write
def cmp in an extension type, Cython knows to
ignore that on Python 3.

Cython tries hard to output .c code that is compatible
with both Python 2 and Python 3. Let's not destroy that
by hard-coding the Python version number in those
conversion functions.

Let me repeat that it is not hardcoded anywhere. There is a constant Cython macro that enables/disables some portion of the code... and it is trivial to make it configurable.

videlec · 2017-06-16T20:05:40Z

sadly network in Bordeaux is down... so Travis is not happy.

videlec · 2017-06-19T12:35:17Z

network back, Travis happy!

jdemeyer

I have no more comments about the general approach, but several minor remarks, see review.

jdemeyer · 2017-06-28T14:02:26Z

cypari2/convert.pyx

+    >>> gen_to_integer(pari("1 - 2^64")) == -18446744073709551615
+    True
+    >>> import sys
+    >>> if sys.version_info.major == 2:


The same test should be done with int instead of long (and you might as well do it on Python 2 and Python 3).

see 5b66460

jdemeyer · 2017-06-28T14:03:36Z

cypari2/convert.pyx

@@ -525,21 +535,21 @@ cpdef gen_to_python(Gen z):
    [1, 2, 3]
    >>> type(a1)
    <... 'list'>
-    >>> map(type, a1)
+    >>> list(map(type, a1))


This is ugly. Write [type(x) for x in a1].

see 3040e65

jdemeyer · 2017-06-28T14:03:46Z

cypari2/convert.pyx

    [<... 'int'>, <... 'int'>, <... 'int'>]

    >>> a2 = gen_to_python(z2); a2
    [1, 3.4, [-5, 2], inf]
    >>> type(a2)
    <... 'list'>
-    >>> map(type, a2)
+    >>> list(map(type, a2))


also 3040e65

jdemeyer · 2017-06-28T14:03:51Z

cypari2/convert.pyx

    [<... 'int'>, <... 'float'>, <... 'list'>, <... 'float'>]

    >>> a3 = gen_to_python(z3); a3
    [1, 5.2]
    >>> type(a3)
    <... 'list'>
-    >>> map(type, a3)
+    >>> list(map(type, a3))


also 3040e65

jdemeyer · 2017-06-28T14:05:37Z

cypari2/convert.pyx

@@ -607,6 +617,6 @@ cpdef gen_to_python(Gen z):
        else:
            return -INFINITY
    elif t == t_STR:
-        return str(z)
+        return to_string(<bytes> GSTR(g))


Is the <bytes> really needed here? (I haven't checked, maybe it is)

It is indeed not needed (see 92f558b)

jdemeyer · 2017-06-28T14:13:20Z

cypari2/gen.pyx

@@ -38,6 +38,9 @@ AUTHORS:



Given that you rely on explicit conversions in many other places, I think it's better to be explicit everywhere. This means dropping cython: c_string_encoding=default and adding to_string or to_bytes where needed.

see b5e5375

jdemeyer · 2017-06-28T14:15:01Z

cypari2/gen.pyx

-        >>> long(pari("Mod(2, 7)"))
-        2L
+        >>> if sys.version_info.major == 3:
+        ...     long = int


jdemeyer · 2017-06-28T14:17:11Z

cypari2/gen.pyx

-        vstr = kwds.keys()            # Variables as Python strings
-        t0 = objtogen(kwds.values())  # Replacements
+        vstr = list(kwds.keys())            # Variables as Python strings
+        t0 = objtogen(list(kwds.values()))  # Replacements


list() should not be needed here: objtogen can deal with generators.

see fa24efb

jdemeyer · 2017-06-28T14:22:34Z

cypari2/pari_instance.pyx

    cdef char s[2]
    s[0] = c
    s[1] = 0
-    sys.stdout.write(s)
+    sys.stdout.write(to_string(<bytes> s))


I don't like this since you are converting bytes to str which sys.stdout immediately needs to convert back to bytes. In Python 3, you can use sys.stdout.buffer for the underlying stream which accepts bytes. So you could do something like:

try: stdout_bytes = sys.stdout.bytes except AttributeError: stdout_bytes = sys.stdout stdout_bytes.write(s)

Does not work as is since sys.stdout is sometimes redirected (e.g. doctest). I used

try: sys.stdout.buffer.write(s) except AttributeError: sys.stdout.write(to_string(s))

See b4410fc

jdemeyer · 2017-06-28T14:24:08Z

tests/rundoctest.py


    print("="*80)
    print("Testing {}".format(mod.__name__))
-    test = doctest.testmod(mod, optionflags=doctest.ELLIPSIS|doctest.REPORT_NDIFF)
+    test = doctest.testmod(mod, optionflags=doctest.ELLIPSIS|doctest.REPORT_NDIFF|doctest.IGNORE_EXCEPTION_DETAIL)


Is this still needed?

It is. In Python3, pari error are printed

Traceback (most recent call last): ... cypari2.handle_error.PariError: MSG

while in Python2 it is

Traceback (most recent call last): ... PariError: MSG

jdemeyer

A few more comments, all easy to fix.

jdemeyer · 2017-07-13T10:25:56Z

cypari2/pari_instance.pyx

+        # avoid string conversion if possible
+        sys.stdout.buffer.write(s)
+    except AttributeError:
+        sys.stdout.write(to_string(s))


You don't need to_string() here. The whole point is that you want to write bytes without conversion.

Yes you do. It is perfectly valid to redirect sys.stdout to some other streams that might not support the buffer thing (like the doctest module does). In other words the except is not only intended for Python2. In particular the to_string is mandatory in Python3 (and useless in Python2). Note that to_string does not perform any conversion in Python2.

jdemeyer · 2017-07-13T10:26:03Z

cypari2/pari_instance.pyx

+        # avoid string conversion if possible
+        sys.stdout.buffer.write(s)
+    except AttributeError:
+        sys.stdout.write(to_string(s))


You don't need to_string() here. The whole point is that you want to write bytes without conversion.

jdemeyer · 2017-07-13T10:27:32Z

cypari2/gen.pyx

@@ -4182,14 +4193,14 @@ cdef class Gen(Gen_auto):
                return new_gen(gsubst(self.g, varn(self.g), t0.g))

        # Call substvec() using **kwds
-        vstr = kwds.keys()            # Variables as Python strings
-        t0 = objtogen(kwds.values())  # Replacements
+        vstr = iter(kwds.keys())            # Variables as Python strings


Should be kwds.iterkeys()

iterkeys does not exist in Python3: kwds.keys() is already an iterator.

jdemeyer · 2017-07-13T10:33:08Z

cypari2/string_utils.pyx

+
+import sys
+encoding = sys.getfilesystemencoding()
+cdef int PY_MAJOR_VERSION = sys.version_info.major


It would be more efficient to make this a compile-time constant with

cdef extern from *: int PY_MAJOR_VERSION

done in d9efdf8

jdemeyer · 2017-07-13T10:34:06Z

cypari2/string_utils.pyx

+    else:
+        raise TypeError
+
+cpdef unicode to_unicode(s):


Maybe cpdef inline for efficiency.

jdemeyer · 2017-07-13T10:34:11Z

cypari2/string_utils.pyx

+encoding = sys.getfilesystemencoding()
+cdef int PY_MAJOR_VERSION = sys.version_info.major
+
+cpdef bytes to_bytes(s):


Maybe cpdef inline for efficiency.

I thought it was wrong to declare an inline function inside a .pyx that is used in other parts of the code (here in auto_instance.pxi)

jdemeyer · 2017-07-13T12:39:47Z

iterkeys does not exist in Python3:

It does exist in Cython, which is what matters here.

jdemeyer · 2017-07-13T12:41:20Z

It is perfectly valid to redirect sys.stdout to some other streams that might not support the buffer thing (like the doctest module does).

The question is: can it happen that sys.stdout does not support the buffer thing and that it also does not accept bytes?

jdemeyer · 2017-07-13T12:45:52Z

I thought it was wrong to declare an inline function inside a .pyx

It is wrong to declare it as inline in a .pxd file. But it's fine to define an inline function in a .pyx file.

jdemeyer · 2017-07-13T12:47:07Z

inlined to_string

That's not what I meant, but that's actually a good solution too.

jdemeyer · 2017-07-13T13:06:56Z

Not iter(vstr.iterkeys()) but just vstr.iterkeys().

videlec · 2017-07-13T13:07:31Z

Not iter(vstr.iterkeys()) but just vstr.iterkeys().

does not work (at least in Python 3)

TypeError: dict_keys object is not an iterator

videlec · 2017-07-13T13:09:59Z

It is perfectly valid to redirect sys.stdout to some other streams that might not support the buffer thing (like the doctest module does).

The question is: can it happen that sys.stdout does not support the buffer thing and that it also does not accept bytes?

Removing the to_string, while running the doctests in Python 3 you get hundreds of

Exception ignored in: 'cypari2.pari_instance.python_puts'
TypeError: string argument expected, got 'bytes'

jdemeyer · 2017-07-13T13:12:33Z

Removing the to_string, while running the doctests in Python 3

I see. This is probably a doctest-specific thing.

jdemeyer · 2017-07-13T13:13:30Z

does not work (at least in Python 3)

TypeError: dict_keys object is not an iterator

You're right. I though that dict.keys() in Python 3 returned the same thing as dict.iterkeys() in Python 2, but apparently, that's not the case. You get an iterable but not an iterator.

jdemeyer · 2017-07-13T13:26:50Z

OK, this is a mess but I guess it's the best we can do. For me, there are no further changes needed.

videlec · 2017-07-13T13:39:41Z

@defeo do you have any comment?

videlec · 2017-07-19T09:45:39Z

@defeo Let say that 6 days mean yes. Though it would be nice to have answered either "do without me" or "wait for me" or whatever...

defeo · 2017-07-19T23:16:41Z

I'm sorry. If this is urgent, do without me. When I have time to get back to this, I'll eventually open another ticket if I see any problem.

videlec added 2 commits April 28, 2017 21:52

more verbose doctest for debugging gen_to_integer

d79f1fd

full Python2 & Python3 support

d55e015

defeo self-requested a review May 12, 2017 14:28

defeo reviewed May 12, 2017

View reviewed changes

videlec added 2 commits May 12, 2017 16:55

restore a space

d7b7a3e

use lambda in closure

418cf40

videlec mentioned this pull request May 16, 2017

Doctests #12

Merged

use compile_time_env option in cythonize for PY_MAJOR_VERSION

001d770

videlec force-pushed the python3 branch from 275358c to 001d770 Compare June 6, 2017 16:34

videlec added 4 commits June 6, 2017 19:54

remove system.pxi from gitignore

1ac5302

better handling of strings (Py2/Py3)

e94b0c7

better rundoctests

308eb2b

Merge master branch

a95e648

videlec force-pushed the python3 branch from 8fd1c2a to a95e648 Compare June 6, 2017 20:46

videlec added 3 commits June 6, 2017 23:54

string_utils.pxd

fd70584

fix autogen

aa2a078

fix args.py

576e3c5

better and simpler string_utils

f65f7b9

some more improvements towards Py2/Py3 compat

f497c2f

jdemeyer requested changes Jun 28, 2017

View reviewed changes

videlec added 4 commits July 10, 2017 09:25

tests long/int in Python2

5b66460

remove list(map(...))

3040e65

dropping c_string_encoding=default

b5e5375

remove <bytes> in to_string(<bytes> X)

92f558b

videlec force-pushed the python3 branch from 15901f9 to 92f558b Compare July 10, 2017 07:29

videlec added 2 commits July 10, 2017 09:35

remove an explicit conversion to list

fa24efb

avoid conversion bytes -> string -> bytes

b4410fc

jdemeyer requested changes Jul 13, 2017

View reviewed changes

compile time PY_MAJOR_VERSION + inlined to_string

d9efdf8

.keys() -> .iterkeys()

a286c14

videlec merged commit dea0639 into master Jul 19, 2017

videlec deleted the python3 branch July 19, 2017 09:53

videlec mentioned this pull request Jul 20, 2017

merge cypari and cypari2 #19

Open

2 tasks

Python3 #13

Python3 #13

Conversation

videlec commented Apr 29, 2017

defeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdemeyer commented May 18, 2017

jdemeyer commented May 18, 2017

videlec commented Jun 7, 2017 • edited Loading

videlec commented Jun 15, 2017

jdemeyer commented Jun 16, 2017

jdemeyer commented Jun 16, 2017

videlec commented Jun 16, 2017

jdemeyer commented Jun 16, 2017

videlec commented Jun 16, 2017

jdemeyer commented Jun 16, 2017

videlec commented Jun 16, 2017

videlec commented Jun 16, 2017

videlec commented Jun 16, 2017

videlec commented Jun 19, 2017

jdemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

videlec Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

videlec Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

videlec Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

videlec Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdemeyer commented Jul 13, 2017

jdemeyer commented Jul 13, 2017

jdemeyer commented Jul 13, 2017

jdemeyer commented Jul 13, 2017

jdemeyer commented Jul 13, 2017

videlec commented Jul 13, 2017 • edited Loading

videlec commented Jul 13, 2017

jdemeyer commented Jul 13, 2017

jdemeyer commented Jul 13, 2017

jdemeyer commented Jul 13, 2017

videlec commented Jun 7, 2017 •

edited

Loading

videlec Jul 10, 2017 •

edited

Loading

videlec Jul 10, 2017 •

edited

Loading

videlec Jul 10, 2017 •

edited

Loading

videlec Jul 10, 2017 •

edited

Loading

videlec commented Jul 13, 2017 •

edited

Loading