You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "bdice (via GitHub)" <gi...@apache.org> on 2023/03/14 20:59:43 UTC
[GitHub] [arrow] bdice opened a new issue, #34564: Building with Cython 3 shows warnings
bdice opened a new issue, #34564:
URL: https://github.com/apache/arrow/issues/34564
### 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](https://github.com/cython/cython/pull/3821/). 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on issue #34564: [Python] Building with Cython 3 shows warnings
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1482854595
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on issue #34564: [Python] Building with Cython 3 shows warnings
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1473895747
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] h-vetinari commented on issue #34564: [Python] Building with Cython 3 shows warnings
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1473925381
> 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. https://github.com/cython/cython/issues/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](https://github.com/cython/cython), the cython project is pretty responsive to issues (even just about warnings), especially now after beta1 / before 3.0 GA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] h-vetinari commented on issue #34564: Building with Cython 3 shows warnings
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1468993581
I was thinking about opening this issue because I saw a bunch of references to arrow in https://github.com/cython/cython/issues/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](https://github.com/cython/cython/issues/4022#issuecomment-1404305257) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace closed issue #34564: [Python] Building with Cython 3 shows warnings
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace closed issue #34564: [Python] Building with Cython 3 shows warnings
URL: https://github.com/apache/arrow/issues/34564
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on issue #34564: Building with Cython 3 shows warnings
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1468966928
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] shwina commented on issue #34564: Building with Cython 3 shows warnings
Posted by "shwina (via GitHub)" <gi...@apache.org>.
shwina commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1468855582
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.
```cython
# 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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdice commented on issue #34564: Building with Cython 3 shows warnings
Posted by "bdice (via GitHub)" <gi...@apache.org>.
bdice commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1468970138
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on issue #34564: Building with Cython 3 shows warnings
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1468977739
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on issue #34564: [Python] Building with Cython 3 shows warnings
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1469684949
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on issue #34564: [Python] Building with Cython 3 shows warnings
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #34564:
URL: https://github.com/apache/arrow/issues/34564#issuecomment-1486483640
Thanks Weston! I opened https://github.com/apache/arrow/issues/34756 for adding a build with Cython 3 to our CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org