Opened 20 months ago

Closed 19 months ago

Last modified 19 months ago

#25549 closed defect (fixed)

.pxd files should not use PY_MAJOR_VERSION compile-time variable

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.3
Component: cython Keywords:
Cc: SimonKing, embray Merged in:
Authors: Jeroen Demeyer Reviewers: Simon King
Report Upstream: N/A Work issues:
Branch: 85451ee (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

sage: cython('''from sage.cpython.string cimport str_to_bytes''')
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-1-217de50fbb18> in <module>()
----> 1 cython('''from sage.cpython.string cimport str_to_bytes''')

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3756)()
    352             True
    353         """
--> 354         return self.get_object()(*args, **kwds)
    355 
    356     def __repr__(self):

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_compile(code, **kwds)
   1009     with open(tmpfile,'w') as f:
   1010         f.write(code)
-> 1011     return cython_import_all(tmpfile, get_globals(), **kwds)

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_import_all(filename, globals, **kwds)
    899       code
    900     """
--> 901     m = cython_import(filename, **kwds)
    902     for k, x in iteritems(m.__dict__):
    903         if k[0] != '_':

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_import(filename, **kwds)
    874     - the module that contains the compiled Cython code.
    875     """
--> 876     name, build_dir = cython(filename, **kwds)
    877 
    878     oldpath = sys.path

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython(filename, verbose, compile_message, use_cache, create_local_c_file, annotate, sage_namespace, create_local_so_file)                                                                                                                                        
    636                         You can fix your code by adding "from {} cimport {}".
    637                         """.format(pxd, name))
--> 638         raise RuntimeError(cython_messages.strip())
    639 
    640     if verbose >= 0:

RuntimeError: Error compiling Cython file:
------------------------------------------------------------
...
    # Missing from cpython.unicode in Cython 0.27.3
    char* PyUnicode_AsUTF8(object s)


cdef inline str char_to_str(const char* c, encoding=None, errors=None):
    IF PY_MAJOR_VERSION <= 2:
      ^
------------------------------------------------------------

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/cpython/string.pxd:25:7: Compile-time name 'PY_MAJOR_VERSION' not defined

Error compiling Cython file:
------------------------------------------------------------
...
        TypeError: expected bytes, list found
    """
    if not isinstance(b, bytes):
        raise TypeError(f"expected bytes, {type(b).__name__} found")

    IF PY_MAJOR_VERSION <= 2:
      ^
------------------------------------------------------------

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/cpython/string.pxd:70:7: Compile-time name 'PY_MAJOR_VERSION' not defined

Error compiling Cython file:
------------------------------------------------------------
...
        TypeError: expected str ... list found
    """
    cdef const char* err
    cdef const char* enc

    IF PY_MAJOR_VERSION <= 2:
      ^
------------------------------------------------------------

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/cpython/string.pxd:106:7: Compile-time name 'PY_MAJOR_VERSION' not defined


Error compiling Cython file:
------------------------------------------------------------
...
from sage.cpython.string cimport str_to_bytes^
------------------------------------------------------------

_home_jdemeyer__sage_temp_sage4_21832_tmp_mx5xCy_pyx_0.pyx:1:0: 'sage/cpython/string/str_to_bytes.pxd' not found

Since the Cython code is almost C anyway, the easiest solution is probably to implement it actually in C (replacing the IF by #if).

Change History (15)

comment:1 Changed 20 months ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 20 months ago by jdemeyer

  • Branch set to u/jdemeyer/_pxd_files_should_not_use_py_major_version_compile_time_variable

comment:3 Changed 20 months ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to 85451ee7a2e43c2704e00c1fe9c868c17696be5e
  • Status changed from new to needs_review

New commits:

85451eeImplement string conversion in C

comment:4 follow-up: Changed 20 months ago by SimonKing

What is the rationale for this:

 cpdef inline str bytes_to_str(b, encoding=None, errors=None):
@@ -64,14 +51,13 @@ cpdef inline str bytes_to_str(b, encoding=None, errors=None):
         ...
         TypeError: expected bytes, list found
     """
-    if not isinstance(b, bytes):
+    if type(b) is not bytes:
         raise TypeError(f"expected bytes, {type(b).__name__} found")

That's to say, why don't you allow sub-types of bytes?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 20 months ago by jdemeyer

Replying to SimonKing:

That's to say, why don't you allow sub-types of bytes?

Mainly to be on the safe side. I have no idea what the C API does with subclasses of bytes.

Whenever a real use case for bytes subclasses comes up, we can always revisit this.

comment:6 in reply to: ↑ 5 Changed 20 months ago by SimonKing

  • Reviewers set to Simon King
  • Status changed from needs_review to positive_review

Replying to jdemeyer:

Replying to SimonKing:

That's to say, why don't you allow sub-types of bytes?

Mainly to be on the safe side. I have no idea what the C API does with subclasses of bytes.

Whenever a real use case for bytes subclasses comes up, we can always revisit this.

OK.

Apart from that, the code looks fine to me, all tests pass, and you added a test that shows that the bug is fixed.

comment:7 Changed 19 months ago by vbraun

  • Branch changed from u/jdemeyer/_pxd_files_should_not_use_py_major_version_compile_time_variable to 85451ee7a2e43c2704e00c1fe9c868c17696be5e
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:8 follow-up: Changed 19 months ago by embray

  • Commit 85451ee7a2e43c2704e00c1fe9c868c17696be5e deleted

I guess I don't mind, but what even is the use case for this:

cython('''from sage.cpython.string cimport str_to_bytes''')

?

IMO str_to_bytes() is just a convenience utility for use within Sage, and not otherwise. It's very easy to implement similar wrappers for one's own purposes.

comment:9 Changed 19 months ago by embray

Or for that matter, what's wrong with just documenting "so and so requires the following compile-time variables to be defined"?

comment:10 follow-ups: Changed 19 months ago by embray

On the third hand, I think it's common enough, especially writing code that is Python 2/3-compatible, to want to have the Python version as a compile-time variable, so maybe Cython really ought to provide this by default...

comment:11 in reply to: ↑ 8 Changed 19 months ago by jdemeyer

Replying to embray:

I guess I don't mind, but what even is the use case for this:

cython('''from sage.cpython.string cimport str_to_bytes''')

Third-party code already using Sage that wants to be Python 2/3 compatible.

comment:12 in reply to: ↑ 10 Changed 19 months ago by SimonKing

Replying to embray:

On the third hand, I think it's common enough, especially writing code that is Python 2/3-compatible, to want to have the Python version as a compile-time variable, so maybe Cython really ought to provide this by default...

This may be. However, Cython currently doesn't provide it.

In fact I did try to use from sage.cpython.string cimport str_to_bytes in a third-party package for Sage. So, the reason for this ticket was an actual use case.

comment:13 Changed 19 months ago by embray

That's fine. It just sucks that in order to write such code in a pxd file it's...basically impossible unless you move it into a pure C file.

Wild-haired idea: allow some directives in Cython files to provide compile-time variables, with short eval-able expressions providing their values, perhaps with a few default imports provided. So one can write # cython: <something something> PY_MAJOR_VERSION=sys.version_info[0] and have that variable provided to the Cython compiler automatically.

Not sure how that would work when compiling modules with functions imported from .pxd files, if it should even work at all.

comment:14 in reply to: ↑ 10 Changed 19 months ago by jdemeyer

Replying to embray:

On the third hand, I think it's common enough, especially writing code that is Python 2/3-compatible, to want to have the Python version as a compile-time variable

I'm not entirely convinced here. In most cases, you can write Python 2/3 compatible Cython code without needing the PY_MAJOR_VERSION compile-time variable.

comment:15 Changed 19 months ago by embray

True, but I think this demonstrates that there's a case for it. You can also write such code with normal if statements as well, but I think it goes without saying that it's better to compile away such branches if possible.

Note: See TracTickets for help on using tickets.