GH-43536: [Python] Declare support for free-threading in Cython #43606
Conversation
This is done by passing an extra flag when building the Cython extension modules. It is needed so that the GIL is not dynamically reenabled when importing `pyarrow.lib`.
|
@lysnikolaou Thanks for the PR. That said, I think it would be better to first add a CI build with a nogil Python. |
That makes sense. I'm already working on this and, at the same time, waiting for #43539 to land before I open a PR for free-threading CI. |
cpp/cmake_modules/UseCython.cmake
Outdated
| execute_process(COMMAND ${PYTHON_EXECUTABLE} -m cython --version | ||
| OUTPUT_VARIABLE CYTHON_version_output | ||
| ERROR_VARIABLE CYTHON_version_error | ||
| RESULT_VARIABLE CYTHON_version_result | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ERROR_STRIP_TRAILING_WHITESPACE) | ||
| if(NOT ${CYTHON_version_result} EQUAL 0) | ||
| set(_error_msg "Command \"${PYTHON_EXECUTABLE} -m cython --version\" failed with") | ||
| set(_error_msg "${_error_msg} output:\n${CYTHON_version_error}") | ||
| message(SEND_ERROR "${_error_msg}") | ||
| else() | ||
| if("${CYTHON_version_output}" MATCHES "^[Cc]ython version ([^,]+)") | ||
| set(CYTHON_VERSION "${CMAKE_MATCH_1}") | ||
| else() | ||
| if("${CYTHON_version_error}" MATCHES "^[Cc]ython version ([^,]+)") | ||
| set(CYTHON_VERSION "${CMAKE_MATCH_1}") | ||
| endif() | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Can we simplify this like the following for Cython 0.29.31 or later?
| execute_process(COMMAND ${PYTHON_EXECUTABLE} -m cython --version | |
| OUTPUT_VARIABLE CYTHON_version_output | |
| ERROR_VARIABLE CYTHON_version_error | |
| RESULT_VARIABLE CYTHON_version_result | |
| OUTPUT_STRIP_TRAILING_WHITESPACE | |
| ERROR_STRIP_TRAILING_WHITESPACE) | |
| if(NOT ${CYTHON_version_result} EQUAL 0) | |
| set(_error_msg "Command \"${PYTHON_EXECUTABLE} -m cython --version\" failed with") | |
| set(_error_msg "${_error_msg} output:\n${CYTHON_version_error}") | |
| message(SEND_ERROR "${_error_msg}") | |
| else() | |
| if("${CYTHON_version_output}" MATCHES "^[Cc]ython version ([^,]+)") | |
| set(CYTHON_VERSION "${CMAKE_MATCH_1}") | |
| else() | |
| if("${CYTHON_version_error}" MATCHES "^[Cc]ython version ([^,]+)") | |
| set(CYTHON_VERSION "${CMAKE_MATCH_1}") | |
| endif() | |
| endif() | |
| endif() | |
| execute_process(COMMAND ${PYTHON_EXECUTABLE} -m cython --version | |
| OUTPUT_VARIABLE CYTHON_VERSION_OUTPUT | |
| OUTPUT_STRIP_TRAILING_WHITESPACE) | |
| if(CYTHON_VERSION_OUTPUT MATCHES "^Cython version (.+)") | |
| set(CYTHON_VERSION "${CMAKE_MATCH_1}") | |
| else() | |
| set(CYTHON_VERSION "") | |
| endif() |
There was a problem hiding this comment.
Should we error out if we fail parsing the output python -m cython --version?
There was a problem hiding this comment.
I think that we don't need it.
If Cython changes cython --version format, PyArrow build will be failed with the latest Cython until we release a new PyArrow version with the latest Cython support.
If we just ignore the cython --version format change, users can still build PyArrow (but some flags may not be used).
Is -Xfreethreading_compatible=True" a required flag for Python 3.13?
There was a problem hiding this comment.
Is
-Xfreethreading_compatible=True"a required flag for Python 3.13?
It's "semi-required" for the free-threaded build of 3.13. More info in my comment above.
There was a problem hiding this comment.
Can we require Cython 3.1.0a0 or later for Python 3.13
I would prefer to not to that
There was a problem hiding this comment.
I'd rather have the CI fail than not notice that some option is disabled.
Can we check it by an unit test? In general, I want to keep CMake related code as simple as possible for easy to maintain.
Replacing the set(CYTHON_VERSION "") with an error message does not look like an additional maintenance burden, does it?
There was a problem hiding this comment.
However, we'll still need CMake code to get the Python version
I think that we can use Python_VERSION defined by CMake's FindPython.cmake instead of getting it by ourselves: https://cmake.org/cmake/help/latest/module/FindPython.html#result-variables
Replacing the
set(CYTHON_VERSION "")with an error message does not look like an additional maintenance burden, does it?
The following is my concern of it: #43606 (comment)
If Cython changes
cython --versionformat, PyArrow build will be failed with the latest Cython until we release a new PyArrow version with the latest Cython support.
(FYI: I don't object strict version detection if there are people who will maintain it. If nobody will not do it, I'll do it.)
There was a problem hiding this comment.
Can we use ${PYTHON_EXECUTABLE} -c "import cython; print(cython.__version__)" instead of ${PYTHON_EXECUTABLE} -m cython --version to avoid parsing cython --version output?
There was a problem hiding this comment.
We may want to add COMMAND_ERROR_IS_FATAL ANY when we require CMake 3.19 or later: https://cmake.org/cmake/help/latest/command/execute_process.html
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
|
I think this is much clearer now. Could you maybe do another round of review please? |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit e8e9d1a. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
This is done by passing an extra flag when building the Cython extension modules. It is needed so that the GIL is not dynamically reenabled when importing
pyarrow.lib.What changes are included in this PR?
Changes to CMake so that the extra flag is passed when building Cython extension modules.