[Packaging][Python] PyArrow fails building with nightlies Cython

[raulcd]

Describe the bug, including details regarding any error messages, version, and platform.

We use a nightlies version of Cython for free-threaded PyArrow:

https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/cython/3.3.0a0/cython-3.3.0a0-py3-none-any.whl

This is currently failing with:

[13/115] Compiling Cython CXX source for lib...
FAILED: CMakeFiles/lib_pyx /arrow/python/build/temp.linux-x86_64-cpython-313t/CMakeFiles/lib_pyx 
cd /arrow/python/build/temp.linux-x86_64-cpython-313t && /opt/python/cp313-cp313t/bin/python -m cython --cplus --warning-errors --no-c-in-traceback -Xfreethreading_compatible=True --directive embedsignature=True --working /arrow/python --output-file /arrow/python/build/temp.linux-x86_64-cpython-313t/lib.cpp /arrow/python/pyarrow/lib.pyx

Error compiling Cython file:
------------------------------------------------------------
...
 options.field_merge_options = _parse_field_merge_options(
 "default" if promote_options == "none" else promote_options
 )

 with nogil:
 options.unify_schemas = promote_options != "none"
 ^
------------------------------------------------------------
pyarrow/table.pxi:6320:48: Coercion from Python not allowed without the GIL

Error compiling Cython file:
------------------------------------------------------------
...
 options.field_merge_options = _parse_field_merge_options(
 "default" if promote_options == "none" else promote_options
 )

 with nogil:
 options.unify_schemas = promote_options != "none"
 ^
------------------------------------------------------------
pyarrow/table.pxi:6320:48: Operation not allowed without gil

Taking a look at latest Cython commits the following could be related:
cython/cython@46e961e

I am unsure what the problem is and whether is a regression on Cython or we should fix the comparison to require the GIL.

This is happening both on main and the 23.0.1 maintenance branch so it has to be fixed for the release.

Component(s)

Packaging, Python

[pitrou]

Here the fix should be trivial (move the offending line outside of the nogil block) :)

That said, this raises the question: why do we test with a Cython nightly?

[AlenkaF]

That said, this raises the question: why do we test with a Cython nightly?

That was added at the time when there were no Cython wheels for 3.13 free-threaded available

arrow/ci/docker/python-wheel-manylinux.dockerfile

Line 116 in d2315fe

# Remove once there are released Cython wheels for 3.13 free-threaded available

I can open a PR that removes nightly Cython install.

[raulcd]

Here the fix should be trivial (move the offending line outside of the nogil block) :)

lol, I guess my question was more on why was this now an issue but if it's not relevant we can just move it outside the nogil block. We can fix that once Cython releases 3.3.0 if that's an issue then. I agree, we can fix now by removing the nightlies Cython version usage.

[pitrou]

lol, I guess my question was more on why was this now an issue

I have no idea if that's a regression in Cython or if we were silently breaking the rules before (does this comparison occur in C-land or in Python-land?). Perhaps @da-woods can elaborate.

But in any case, moving the offending line doesn't hurt, this isn't a performance bottleneck.

[da-woods]

I have no idea if that's a regression in Cython or if we were silently breaking the rules before (does this comparison occur in C-land or in Python-land?). Perhaps @da-woods can elaborate.

To me it looks like you were silently breaking the rules (i.e. the code before the change looks like it needs the GIL).

Potentially it might be possible to write it in a way that doesn't use the GIL (at least on the full CPython API) but I'm not completely sure if that's a good idea.

[scoder]

Thanks for notifying us. It seems a good thing that you tested with a nightly and it's probably worth keeping that, at least as an "allowed to fail" build job or regular out of band run.

I don't see why Python string comparisons work without the GIL and it's an oversight that this wasn't detected before. The old string comparison code that we used was this:

https://github.com/cython/cython/pull/7477/files#diff-d38ed1f4a00909a7d38713c9927577f6a2be71c88e248ce55694f9e3020da5bf

It's not nogil-compatible outside of CPython (e.g. PyPy or the Limited API) and probably crashes also in CPython when you compare a Unicode string to another known builtin type (which is unlikely to occur in real code because it always raises an exception). Cython should have rejected this comparison in a nogil section. I created cython/cython#7500 for that.

The fix on your side is simple, just move the option setting line out of the with nogil: block. It's the right thing to do, regardless of the Cython version used.