Opened 5 years ago

Closed 5 years ago

#23648 closed defect (worksforme)

Py3: TypeError from Cython string conversion

Reported by: rws Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: python3 Keywords: Py3, unicode
Cc: chapoton, embray Merged in:
Authors: Frédéric Chapoton Reviewers:
Report Upstream: N/A Work issues:
Branch: public/23648 (Commits, GitHub, GitLab) Commit: 7e014227489fa0d52a9528681833670ab7926904
Dependencies: #24222 Stopgaps:

Status badges

Description (last modified by chapoton)

With Py3, as reported in https://github.com/pynac/pynac/issues/271

In [2]: import sage
In [3]: from sage.libs.pynac.constant import PynacConstant
In [4]: f = PynacConstant('foo', 'foo', 'real')
...
TypeError: expected bytes, str found

It's is in this part of the Sage-Pynac interface:

  /* "sage/libs/pynac/constant.pyx":68
 *             self.pointer = <GConstant *>&g_NaN
 *         else:
>>> *          self._object = new GConstant(name, ConstantEvalf, texname, domain)             # <<<<<<<<<<<<<<
 *             self.pointer = self._object
 * 
 */
  /*else*/ {
    __pyx_t_2 = __Pyx_PyObject_AsWritableString(__pyx_v_name); if (unlikely((!__pyx_t_2) && PyErr_Occurred())) __PYX_ERR(0, 68, __pyx_L1_error)
    __pyx_t_3 = __Pyx_PyObject_AsWritableString(__pyx_v_texname); if (unlikely((!__pyx_t_3) && PyErr_Occurred())) __PYX_ERR(0, 68, __pyx_L1_error)
    __pyx_v_self->_object = new constant(__pyx_t_2, ConstantEvalf, __pyx_t_3, __pyx_v_domain);

Apparently the error comes from this function (or below): https://github.com/cython/cython/blob/master/Cython/Utility/TypeConversion.c#L202

EDIT: also, one currently gets

In [28]: f = PynacConstant(b'foo', b'foo', 'real')

In [29]: f.__repr__()
Out[29]: b'foo'

Change History (34)

comment:1 Changed 5 years ago by jdemeyer

  • Component changed from cython to python3
  • Keywords cython removed

comment:2 Changed 5 years ago by chapoton

so this is not a cython problem ? where is the issue then ?

comment:3 Changed 5 years ago by jdemeyer

It's just your typical unicode vs. bytes issue. Some API expects bytes but is given unicode instead.

comment:4 Changed 5 years ago by chapoton

well, same kind of thing as here then ?

cypari2/auto_gen.pxi in cypari2.gen.Gen_auto.Polrev()

cypari2/pari_instance.pyx in cypari2.pari_instance.get_var()

TypeError: string argument without an encoding

Hopefully this one will be fixed by #23518

comment:5 Changed 5 years ago by chapoton

Confirmed in the latest python3 build.

EDIT: And this is a major blocking point.

Last edited 5 years ago by chapoton (previous) (diff)

comment:6 Changed 5 years ago by chapoton

  • Description modified (diff)
  • Status changed from new to needs_info

comment:7 Changed 5 years ago by rws

Jeroen, can you please clarify where this should be fixed?

comment:8 Changed 5 years ago by chapoton

  • Branch set to public/23648
  • Commit set to 587938f63792af7a0d893e1126e5bc0cbd86c3a0
  • Status changed from needs_info to needs_review

New commits:

069fd36new method "to_bytes" and change in pynac constant
587938fadding an alias for u

comment:9 Changed 5 years ago by jdemeyer

  1. Please remove to_unicode = u, that is not relevant here.
  1. I suggest to use sys.getfilesystemencoding() instead of hard-coding UTF-8. The reason is that the conversions you are doing here are really analogous to the implicit bytes <-> unicode conversions that Python 3 does internally. In those cases, Python 3 mostly uses sys.getfilesystemencoding().

comment:10 Changed 5 years ago by jdemeyer

  1. Maybe renaming the function to string_to_bytes to make it clear that it accepts only string-like objects, it does not convert everything to bytes.
  1. For efficiency, I would implement this in a Cython file as cpdef functions such that Cython code can benefit from fast calls. Maybe a new file like src/sage/cpython/string.pyx?

comment:11 Changed 5 years ago by git

  • Commit changed from 587938f63792af7a0d893e1126e5bc0cbd86c3a0 to 7e014227489fa0d52a9528681833670ab7926904

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

7e01422trac 23648

comment:12 Changed 5 years ago by chapoton

  • Cc embray added

comment:13 Changed 5 years ago by jdemeyer

Alternatively, you could directly implement conversion char * -> str (skipping the intermediate bytes step) using PyUnicode_DecodeFSDefault in Python 3.

comment:14 follow-ups: Changed 5 years ago by chapoton

How is your last comment related to #23857 ?

I am not able to turn the branche here easily into a cpdef function. Could one of you do that please ?

comment:15 in reply to: ↑ 14 Changed 5 years ago by jdemeyer

Replying to chapoton:

How is your last comment related to #23857 ?

It is not strictly related to #23857. It is just that #23857 also fixes some bytes vs str bugs, but only for C++ code.

comment:16 in reply to: ↑ 14 Changed 5 years ago by chapoton

Replying to chapoton:

How is your last comment related to #23857 ?

I am not able to turn the branche here easily into a cpdef function. Could one of you do that please ?

*ping* ?

comment:17 Changed 5 years ago by jdemeyer

Traceback please!

comment:19 Changed 5 years ago by chapoton

  • Keywords unicode added

comment:20 Changed 5 years ago by chapoton

Here is some traceback (using ipython3 with a failed python3-built):

In [3]: from sage.all import *
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-90a1b1fe16c5> in <module>()
----> 1 from sage.all import *

/home/chapoton/sage3/local/lib/python3.6/site-packages/sage/all.py in <module>()
    100 from sage.matrix.all     import *
    101 
--> 102 from sage.symbolic.all   import *
    103 from sage.modules.all    import *
    104 from sage.monoids.all    import *

/home/chapoton/sage3/local/lib/python3.6/site-packages/sage/symbolic/all.py in <module>()
      5 
      6 from .ring import SR
----> 7 from .constants import (pi, e, NaN, golden_ratio, log2, euler_gamma, catalan,
      8                        khinchin, twinprime, mertens, glaisher)
      9 from .expression import Expression, solve_diophantine

/home/chapoton/sage3/local/lib/python3.6/site-packages/sage/symbolic/constants.py in <module>()
    842         return sympy.GoldenRatio
    843 
--> 844 golden_ratio = GoldenRatio().expression()
    845 
    846 class Log2(Constant):

/home/chapoton/sage3/local/lib/python3.6/site-packages/sage/symbolic/constants.py in __init__(self, name)
    772                            kash='(1+Sqrt(5))/2', giac='(1+sqrt(5))/2')
    773         Constant.__init__(self, name, conversions=conversions,
--> 774                           latex=r'\phi', domain='positive')
    775 
    776     def minpoly(self, bits=None, degree=None, epsilon=0):

/home/chapoton/sage3/local/lib/python3.6/site-packages/sage/symbolic/constants.py in __init__(self, name, conversions, latex, mathml, domain)
    286 
    287         from sage.libs.pynac.constant import PynacConstant
--> 288         self._pynac = PynacConstant(self._name, self._latex, self._domain)
    289         self._serial = self._pynac.serial()
    290         constants_table[self._serial] = self

/home/chapoton/sage3/src/sage/libs/pynac/constant.pyx in sage.libs.pynac.constant.PynacConstant.__cinit__ (build/cythonized/sage/libs/pynac/constant.cpp:2527)()

TypeError: expected bytes, str found

comment:21 Changed 5 years ago by chapoton

ping ?

Once again, I am not able to make a cpdef function, so please do that.

comment:22 Changed 5 years ago by chapoton

I have created a new ticket that just wants to introduce the conversion tools: #24186.

comment:23 Changed 5 years ago by rws

No patchbot on this until now, strange.

comment:24 Changed 5 years ago by chapoton

Authors field is empty..

comment:25 Changed 5 years ago by rws

  • Dependencies set to #24222

comment:26 Changed 5 years ago by jdemeyer

  • Authors set to Frédéric Chapoton

comment:27 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This should use the functions introduced in #24222.

comment:28 Changed 5 years ago by chapoton

  • Milestone changed from sage-8.2 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

Let us close this one as invalid, please

comment:29 Changed 5 years ago by jdemeyer

Why? Is it already fixed?

comment:30 Changed 5 years ago by chapoton

It seems so. I am not able to reproduce that in ipython. And I am also currently no longer able to have a starting python3-sage, sadly.

comment:31 Changed 5 years ago by rws

  • Status changed from needs_review to needs_info

So we need more info, no?

comment:32 Changed 5 years ago by chapoton

  • Status changed from needs_info to needs_review

No, no need for more info. Maybe I was not clear enough.

Using an ipython shell, I can do exactly what I did in the first part of the ticket description. And there is no longer any failure. So the problem has been fixed somewhere in one of the previous py3 tickets.

Believe me, one should close this one and concentrate on the many other py3 tickets..

comment:33 Changed 5 years ago by rws

  • Status changed from needs_review to positive_review

comment:34 Changed 5 years ago by embray

  • Resolution set to worksforme
  • Status changed from positive_review to closed

Indeed, this is fixed now.

Note: See TracTickets for help on using tickets.