[Python] Building with Cython 3 shows warnings

[bdice]

Describe the enhancement requested

When building against PyArrow's Cython interface with Cython 3.0.0 beta 1, I see the following warnings:

warning: /home/bdice/mambaforge/envs/cython_beta/lib/python3.10/site-packages/pyarrow/lib.pxd:33:60: The keyword 'nogil' should appear at the end of the function signature line. Placing it before 'except' or 'noexcept' will be disallowed in a future version of Cython.
warning: /home/bdice/mambaforge/envs/cython_beta/lib/python3.10/site-packages/pyarrow/includes/libarrow.pxd:1715:24: Rvalue-reference as function argument not supported
warning: /home/bdice/mambaforge/envs/cython_beta/lib/python3.10/site-packages/pyarrow/includes/libarrow.pxd:1740:26: Rvalue-reference as function argument not supported
warning: /home/bdice/mambaforge/envs/cython_beta/lib/python3.10/site-packages/pyarrow/includes/libarrow.pxd:1756:23: Rvalue-reference as function argument not supported
warning: /home/bdice/mambaforge/envs/cython_beta/lib/python3.10/site-packages/pyarrow/includes/libarrow.pxd:1770:24: Rvalue-reference as function argument not supported
warning: /home/bdice/mambaforge/envs/cython_beta/lib/python3.10/site-packages/pyarrow/includes/libarrow.pxd:2115:20: Rvalue-reference as function argument not supported

For the nogil warning, I think the fix is straightforward - just move the nogil qualifier to the end of the line.

For the Rvalue-reference warnings, I discussed some similar warnings with @shwina, who initially contributed the PR that added these warnings. He said the best fix is to remove definitions with Rvalue references (T&&) as arguments, because Cython can't use them anyway.

Let me know if a PR to fix these warnings would be helpful.

Component(s)

Python

[shwina]

Right, my understanding is that Cython doesn't really know about rvalue references or move semantics. To generate correct C++ code involving functions that accept rvalue references, you can declare them as accepting type T, and explicitly use move() when passing arguments to them.

# distutils: language=c++                                                                                                                                                              
# extra_compile_args: -std=c++17                                                                                                                                                       
                                                                                                                                                                                       
from libcpp.utility cimport move                                                                                                                                                       
                                                                                                                                                                                       
                                                                                                                                                                                       
cdef extern from *:                                                                                                                                                                    
    """                                                                                                                                                                                
    class Foo {                                                                                                                                                                        
      public:                                                                                                                                                                          
        Foo() = default;                                                                                                                                                               
    };                                                                                                                                                                                 
                                                                                                                                                                                       
    void bar(Foo&& x) {                                                                                                                                                                
        return;                                                                                                                                                                        
    }
    """                                                                                                                                                                                                                                                                                                                                                                  
    cppclass Foo:                                                                                                                                                                      
        Foo()                                                                                                                                                                          
                                                                                                                                                                                       
    void bar(Foo)  # no need to declare this void bar(Foo&&)                                                                                                                                                               
                                                                                                                                                                                       
cdef Foo f = Foo()                                                                                                                                                                     
                                                                                                                                                                                       
# bar(f) -> compile error
bar(move(f))  # works

[westonpace]

Ok, removing the rvalue's in that way sounds reasonable. How do we regress this? Ideally we could get some kind of cython 3 build in our CI. How close is cython 3 to releasing?

[bdice]

I don’t have a good sense of the timeline, but Cython 3 is now “beta” status. For now, I would propose fixing the warnings as a one-time pass. Then later when Cython 3 is released, use it in CI with warnings as errors. What do you think?

[westonpace]

That seems fine but a number of people contribute to the cython bindings (and not all of them are reading this issue) so I think there is a reasonable chance that warnings will creep back in.

[h-vetinari]

I was thinking about opening this issue because I saw a bunch of references to arrow in cython/cython#5305. It's possible that some have been fixed on main here, or that they're dependent on the exact configuration (e.g. whether specifying language_level=2 or =3).

How close is cython 3 to releasing?

Based on the progress so far (and some new blockers arising as people start testing beta1 in earnest), I'm guessing it'll still take several weeks to have beta2, much less the final release.

[jorisvandenbossche]

I think it's fine to already start fixing some of those cases without CI, while also look at adding CI support eventually (I don't think that should be difficult, we can install the --pre wheel with pip in one of the builds, that's what eg pandas does)

In the meantime, should we actually pin cython to <3 in our "requires" table in pyproject.toml?

[westonpace]

I started trying to fix these but ran into some problems that I think are bugs in cython?

This one is a warning (the method __pyx_convert_vector_to_py_int is a cython method I think):

/home/pace/dev/arrow/python/build/temp.linux-x86_64-cpython-311/lib.cpp: In function ‘PyObject* __pyx_convert_vector_to_py_int(const std::vector<int>&)’:
/home/pace/dev/arrow/python/build/temp.linux-x86_64-cpython-311/lib.cpp:236294:49: warning: comparison of integer expressions of different signedness: ‘Py_ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
236294 |                   for (__pyx_t_5 = 0; __pyx_t_5 < __pyx_t_4; __pyx_t_5+=1) {
       |                                       ~~~~~~~~~~^~~~~~~~~~~

This one is an error (the method input_stream is in our pxi files but I verified it does not call PyMemoryView_Check and the two places we do call that method the capitalization is correct):

/home/pace/dev/arrow/python/build/temp.linux-x86_64-cpython-311/lib.cpp: In function ‘PyObject* __pyx_pf_7pyarrow_3lib_214input_stream(PyObject*, PyObject*, PyObject*, PyObject*)’:
/home/pace/dev/arrow/python/build/temp.linux-x86_64-cpython-311/lib.cpp:202114:31: error: ‘PyMemoryview_Check’ was not declared in this scope; did you mean ‘PyMemoryView_Check’?
202114 |                   __pyx_t_9 = PyMemoryview_Check(__pyx_v_source);
       |                               ^~~~~~~~~~~~~~~~~~
       |                               PyMemoryView_Check

And then this code:

    @property
    def dim_names(self):
        return tuple(frombytes(x) for x in tuple(self.stp.dim_names()))

at some point unwraps into:

		/* "pyarrow/tensor.pxi":556                                                                                                                                                                        
 *     @property                                                                                                                                                                                                   
 *     def dim_names(self):                                                                                                                                                                                        
 *         return tuple(frombytes(x) for x in tuple(self.stp.dim_names()))             # <<<<<<<<<<<<<<                                                                                                            
 *                                                                                                                                                                                                                 
 *     @property                                                                                                                                                                                                   
 */

                static PyObject *__pyx_pf_7pyarrow_3lib_15SparseCOOTensor_9dim_names_7__get___genexpr(CYTHON_UNUSED PyObject *__pyx_self, std::vector<std::string>  const __pyx_genexpr_arg_0) {
                  struct __pyx_obj_7pyarrow_3lib___pyx_scope_struct_15_genexpr *__pyx_cur_scope;
                  PyObject *__pyx_r = NULL;
                  __Pyx_RefNannyDeclarations
                  int __pyx_lineno = 0;
                  const char *__pyx_filename = NULL;
                  int __pyx_clineno = 0;
                  __Pyx_RefNannySetupContext("genexpr", 0);
                  __pyx_cur_scope = (struct __pyx_obj_7pyarrow_3lib___pyx_scope_struct_15_genexpr *)__pyx_tp_new_7pyarrow_3lib___pyx_scope_struct_15_genexpr(__pyx_ptype_7pyarrow_3lib___pyx_scope_struct_15_genexpr, __pyx_empty_tuple, NULL);
                  if (unlikely(!__pyx_cur_scope)) {
                    __pyx_cur_scope = ((struct __pyx_obj_7pyarrow_3lib___pyx_scope_struct_15_genexpr *)Py_None);
                    __Pyx_INCREF(Py_None);
                    __PYX_ERR(6, 556, __pyx_L1_error)
                  } else {
                    __Pyx_GOTREF((PyObject *)__pyx_cur_scope);
                  }
                  __pyx_cur_scope->__pyx_genexpr_arg_0 = __pyx_genexpr_arg_0;
                  {
                    __pyx_CoroutineObject *gen = __Pyx_Generator_New((__pyx_coroutine_body_t) __pyx_gb_7pyarrow_3lib_15SparseCOOTensor_9dim_names_7__get___2generator17, NULL, (PyObject *) __pyx_cur_scope, __pyx_n_s_genexpr, __pyx_n_s_SparseCOOTensor___get___locals_g, __pyx_n_s_pyarrow_lib); if (unlikely(!gen)) __PYX_ERR(6, 556, __pyx_L1_error)
                    __Pyx_DECREF(__pyx_cur_scope);
                    __Pyx_RefNannyFinishContext();

However, the __pyx_obj_7pyarrow_3lib___pyx_scope_struct_15_genexpr is declared as:

/* "pyarrow/tensor.pxi":556                                                                                                                                                                                        
 *     @property                                                                                                                                                                                                   
 *     def dim_names(self):                                                                                                                                                                                        
 *         return tuple(frombytes(x) for x in tuple(self.stp.dim_names()))             # <<<<<<<<<<<<<<                                                                                                            
 *                                                                                                                                                                                                                 
 *     @property                                                                                                                                                                                                   
 */
struct __pyx_obj_7pyarrow_3lib___pyx_scope_struct_15_genexpr {
  PyObject_HEAD
  std::vector<std::string>  const __pyx_genexpr_arg_0;
  PyObject *__pyx_v_x;
  PyObject *__pyx_t_0;
  Py_ssize_t __pyx_t_1;
};

Since __pyx_genexpr_arg_0 is const this line triggers an error:

__pyx_cur_scope->__pyx_genexpr_arg_0 = __pyx_genexpr_arg_0;

/home/pace/dev/arrow/python/build/temp.linux-x86_64-cpython-311/lib.cpp: In function ‘PyObject* __pyx_pf_7pyarrow_3lib_15SparseCOOTensor_9dim_names_7__get___genexpr(PyObject*, std::vector<std::__cxx11::basic_string<char> >)’:
/home/pace/dev/arrow/python/build/temp.linux-x86_64-cpython-311/lib.cpp:161732:58: error: passing ‘const std::vector<std::__cxx11::basic_string<char> >’ as ‘this’ argument discards qualifiers [-fpermissive]
161732 |                   __pyx_cur_scope->__pyx_genexpr_arg_0 = __pyx_genexpr_arg_0;
       |                                                          ^~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/11/vector:72,
                 from /home/pace/miniconda3/envs/conbench3/include/arrow/array/array_base.h:24,
                 from /home/pace/miniconda3/envs/conbench3/include/arrow/array.h:41,
                 from /home/pace/miniconda3/envs/conbench3/include/arrow/pch.h:23,
                 from /home/pace/dev/arrow/python/pyarrow/src/arrow/python/pch.h:23,
                 from /home/pace/dev/arrow/python/build/temp.linux-x86_64-cpython-311/CMakeFiles/lib.dir/cmake_pch.hxx:5,
                 from <command-line>:
/usr/include/c++/11/bits/vector.tcc:198:5: note:   in call to ‘std::vector<_Tp, _Alloc>& std::vector<_Tp, _Alloc>::operator=(const std::vector<_Tp, _Alloc>&) [with _Tp = std::__cxx11::basic_string<char>; _Alloc = std::allocator<std::__cxx11::basic_string<char> >]’
  198 |     vector<_Tp, _Alloc>::
      |     ^~~~~~~~~~~~~~~~~~~

Is there somewhere I should file these against the cython project or are they already known issues?

[h-vetinari]

Is there somewhere I should file these against the cython project or are they already known issues?

A bunch of them are known, see. e.g. cython/cython#5305 (e.g. the PyMemoryview_Check part; fixed in cython master but not released yet). The last one is I believe due to -Werror? In any case, you can file a bug here, the cython project is pretty responsive to issues (even just about warnings), especially now after beta1 / before 3.0 GA.

[westonpace]

I filed an issue for the const warning. A workaround was proposed for the short term. I have verified that, with that workaround in place, I can build using the latest cython 3 from master (not with 3.0b1 because of the PyMemoryview_Check issue).

[jorisvandenbossche]

Thanks Weston! I opened #34756 for adding a build with Cython 3 to our CI