Opened 6 years ago

Closed 6 years ago

#22198 closed enhancement (fixed)

Make cypari2 Python2 / Python3 compatible

Reported by: Vincent Delecroix Owned by:
Priority: major Milestone: sage-7.6
Component: c_lib Keywords:
Cc: Luca De Feo, Jeroen Demeyer Merged in:
Authors: Vincent Delecroix, Jeroen Demeyer Reviewers: Jeroen Demeyer, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 7096c82 (Commits, GitHub, GitLab) Commit: 7096c82b4ec94c5d148231b7df518ca5154aad2a
Dependencies: #22185 Stopgaps:

Status badges

Description

Currently string handling in cypari2 is done via PyString_AsString which does not exist in Python 3. We rely on Cython to make proper conversions to bytes.

Attachments (1)

cython_string.pyx (492 bytes) - added by Vincent Delecroix 6 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 6 years ago by Vincent Delecroix

Branch: u/vdelecroix/22198
Commit: ff6a3fd4bedecacb6f3061ccd51c1ee725b2a864
Status: newneeds_review

Last 10 new commits:

183588e21808: fix some more .python() .sage()
8828867Python 3 compatibility in doctests
87b6ac3Merge branch 't/21807/21807' into HEAD
ee54f07Python 3 compatibility in doctests
7a35c89Merge commit '6022cab1880d6f3820e0f028671ddd2983eae42b'; commit 'ee54f071a26c63821f475d2832c7bb1fbbdd7e95' into ticket/22183
235efd3Rename PariInstance -> Pari
6f04abaRemove unused imports from sage
c258dcaRename gen -> Gen
0c4433cFix documentation
ff6a3fd22198: do not depend on PyString_AsString

comment:2 Changed 6 years ago by Jeroen Demeyer

Did you actually check that Python 3 works with this patch?

comment:3 Changed 6 years ago by Vincent Delecroix

How can I do that?!

At least it works in cypari2 with commit 88bf24e.

comment:4 Changed 6 years ago by Jeroen Demeyer

Branch: u/vdelecroix/22198u/jdemeyer/22198

comment:5 Changed 6 years ago by Jeroen Demeyer

Commit: ff6a3fd4bedecacb6f3061ccd51c1ee725b2a864683816d54d501f344124ae80f4a90170e517b186

This is a much simpler version, relying on Cython to convert str -> bytes.


New commits:

683816d22198: do not depend on PyString_AsString

comment:6 Changed 6 years ago by Jeroen Demeyer

Note: Cython converts Unicode strings to bytes by using sys.getdefaultencoding() which seems like a reasonable thing to do. In Python 2, this encoding is ASCII by default, in Python 3 it is UTF-8 by default.

comment:7 Changed 6 years ago by Vincent Delecroix

Status: needs_reviewneeds_work

For me it compiles but does not work in Python 3

>>> import cypari2
>>> pari = cypari2.Pari()
>>> pari("zeta(2)")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cypari2/pari_instance.pyx", line 831, in cypari2.pari_instance.Pari.__call__ (cypari2/pari_instance.c:13408)
  File "cypari2/gen.pyx", line 4534, in cypari2.gen.objtogen (cypari2/gen.c:128337)
TypeError: expected bytes, str found
Last edited 6 years ago by Vincent Delecroix (previous) (diff)

comment:8 Changed 6 years ago by Vincent Delecroix

Actually, in Cython for Python 3 given a string s the conversion

<bytes> s

does not do anything!! (tested with Cython version 0.25.2)

Last edited 6 years ago by Vincent Delecroix (previous) (diff)

comment:9 Changed 6 years ago by Vincent Delecroix

In your patch, bytes do not get catch in Python 3.

Last edited 6 years ago by Vincent Delecroix (previous) (diff)

comment:10 Changed 6 years ago by Jeroen Demeyer

<bytes>s is not a conversion, it's a cast! So yes, it doesn't "do" anything and that's a feature.

comment:11 Changed 6 years ago by Vincent Delecroix

With your patch, unicode are also not treated correctly (see 7).

Last edited 6 years ago by Vincent Delecroix (previous) (diff)

comment:12 Changed 6 years ago by Vincent Delecroix

And conversion does fail

Python 3.5.2
>>> bytes("hello")
Traceback (most recent call last):
...
TypeError: string argument without an encoding

I don't understand where the default encoding that you mentioned in 6 is intended to happen.

EDIT: this command fails in the same way in Cython

Last edited 6 years ago by Vincent Delecroix (previous) (diff)

comment:13 Changed 6 years ago by Jeroen Demeyer

The default encoding is something Cython-specific when passing unicode or bytes to C as a char*. It's not about converting within Python from unicode to bytes.

comment:14 in reply to:  13 ; Changed 6 years ago by Vincent Delecroix

Replying to jdemeyer:

The default encoding is something Cython-specific when passing unicode or bytes to C as a char*. It's not about converting within Python from unicode to bytes.

I know and I already mentioned it in 12: the code behaves the same when compiled in Cython. You can check with

Python 3.5.2
>>> import cython
>>> cython.inline('bytes("hello")')
Traceback (most recent call last):
...
TypeError: string argument without an encoding
Last edited 6 years ago by Vincent Delecroix (previous) (diff)

comment:15 in reply to:  14 Changed 6 years ago by Jeroen Demeyer

Replying to vdelecroix:

I know and I already mentioned it in 12: the code behaves the same when compiled in Cython.

Yes, and I already mentioned that it's not about bytes(foo), it's about passing foo as a char* to C.

comment:16 Changed 6 years ago by Jeroen Demeyer

Summary: make cypari2 Python2 / Python3 compatibleMake cypari2 Python2 / Python3 compatible

comment:17 Changed 6 years ago by git

Commit: 683816d54d501f344124ae80f4a90170e517b186819ed9ec1916e2a8f4104b2b937ffb42a76db350

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

819ed9eTrac #22198: do not depend on PyString_AsString

comment:18 Changed 6 years ago by Jeroen Demeyer

Status: needs_workneeds_review

Does this work?

comment:19 Changed 6 years ago by Vincent Delecroix

I don't think so: bytes and basestring are disjoint in Python 3. Let me try.

comment:20 Changed 6 years ago by Vincent Delecroix

Actually it fails at compilation

ImportError: Building module cython_string failed: ['LookupError: unknown encoding: default\n']

The encoding used seems to be the encoding of the file, not of the system. Adding # encoding: utf-8 at the top of the file make it works.

Changed 6 years ago by Vincent Delecroix

Attachment: cython_string.pyx added

comment:21 in reply to:  19 Changed 6 years ago by Vincent Delecroix

Replying to vdelecroix:

I don't think so: bytes and basestring are disjoint in Python 3. Let me try.

Indeed

Python 3.5.2
>>> import cypari2
>>> pari = cypari2.Pari()
>>> pari(b"zeta(2)")
Traceback (most recent call last):
...
PariError: syntax error, unexpected variable name, expecting $end

comment:22 Changed 6 years ago by Vincent Delecroix

Branch: u/jdemeyer/22198u/vdelecroix/22198
Commit: 819ed9ec1916e2a8f4104b2b937ffb42a76db350ff6a3fd4bedecacb6f3061ccd51c1ee725b2a864

New commits:

ff6a3fd22198: do not depend on PyString_AsString

comment:23 Changed 6 years ago by git

Commit: ff6a3fd4bedecacb6f3061ccd51c1ee725b2a8646c55c787706ed2205cb85a554b752ae829d710c5

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

819ed9eTrac #22198: do not depend on PyString_AsString
6c55c78Trac #22198: fix bytes for Python3

comment:24 Changed 6 years ago by Vincent Delecroix

please see my last commit 6c55c78 that does work in the standalone cypari.

comment:25 Changed 6 years ago by Vincent Delecroix

Authors: Vincent DelecroixVincent Delecroix, Jeroen Demeyer
Reviewers: Jeroen Demeyer, Vincent Delecroix
Status: needs_reviewpositive_review

comment:26 Changed 6 years ago by Jeroen Demeyer

Branch: u/vdelecroix/22198u/jdemeyer/22198

comment:27 Changed 6 years ago by Jeroen Demeyer

Commit: 6c55c787706ed2205cb85a554b752ae829d710c57096c82b4ec94c5d148231b7df518ca5154aad2a
Status: positive_reviewneeds_review

Does this work too? It's a slightly more efficient version of your code.


New commits:

7096c82Trac #22198: fix bytes for Python3

comment:28 Changed 6 years ago by Vincent Delecroix

It does.

comment:29 Changed 6 years ago by Jeroen Demeyer

Positive review then?

comment:30 Changed 6 years ago by Vincent Delecroix

Status: needs_reviewpositive_review

comment:31 Changed 6 years ago by Volker Braun

Branch: u/jdemeyer/221987096c82b4ec94c5d148231b7df518ca5154aad2a
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.