GH-43536: [Python] Do not use borrowed references APIs #43540

lysnikolaou · 2024-08-02T17:27:41Z

Rationale for this change

For better reference safety under Python free-threaded builds (i.e. with the GIL removed), we should be using Py(List|Dict)_GetItemRef that return strong references and are implemented in a thread-safe manner.

What changes are included in this PR?

Vendor a copy of https://github.com/python/pythoncapi-compat
Port to strong reference APIs for lists and dicts

Are these changes tested?

I ran the tests with the free-threaded build before and after, and there's the same expected failures.

GitHub Issue: [Python] Support for the free-threaded build of CPython 3.13 #43536

github-actions · 2024-08-02T17:28:06Z

⚠️ GitHub issue #43536 has been automatically assigned in GitHub to PR creator.

jorisvandenbossche · 2024-08-05T10:19:30Z

General question: are the #ifdef / #else needed, or could we also simply always use the variants that return strong references, also when free-threading is not enabled? Or has that a performance cost?

(or if the answer is that those APIs are not supported for older versions of CPython, adopting pythoncapi-compat might help? #43069)

lysnikolaou · 2024-08-05T11:02:49Z

We could use the strong reference APIs in the non-free-threaded build as well.

Adopting pythoncapi-compat would certainly help. Glad that there's an open issue about that already. I can work on a PR that vendors the compat header.

pitrou · 2024-08-06T13:11:39Z

These changes add a lot of conditional code. Also, sometimes they are not necessary, if we are sure the container (list/dict) will not be mutated in another thread.

python/pyarrow/src/arrow/python/benchmark.cc

python/pyarrow/src/arrow/python/deserialize.cc

python/pyarrow/src/arrow/python/numpy_to_arrow.cc

python/pyarrow/src/arrow/python/python_to_arrow.cc

lysnikolaou · 2024-08-06T14:59:43Z

Thanks @pitrou for the review! I updated the PR to add pythoncapi_compat.h and use the strong reference APIs from there.

python/pyarrow/src/arrow/python/platform.h

python/pyarrow/src/arrow/python/pythoncapi_compat.h

python/pyarrow/src/arrow/python/platform.h

python/pyarrow/src/arrow/python/vendored/pythoncapi_compat.h

lysnikolaou · 2024-08-13T12:43:47Z

This is ready for another review.

pitrou

LGTM, thanks a lot @lysnikolaou . I will rebase and try to run more CI.

pitrou · 2024-08-15T10:03:40Z

@github-actions crossbow submit -g python -g wheel

github-actions · 2024-08-15T10:07:02Z

Revision: 202de1a

Submitted crossbow builds: ursacomputing/crossbow @ actions-ceaf2f2afb

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest-numpy-1.26
test-conda-python-3.10-pandas-latest-numpy-latest
test-conda-python-3.10-pandas-nightly-numpy-nightly
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0-numpy-1.19
test-conda-python-3.9
test-conda-python-3.9-pandas-latest-numpy-latest
test-conda-python-emscripten
test-cuda-python
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-39-python-3
test-ubuntu-20.04-python-3
test-ubuntu-22.04-python-3
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

pitrou · 2024-08-15T12:24:25Z

CI failures look unrelated, I'll merge.

conbench-apache-arrow · 2024-08-15T20:36:58Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 894f72f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change #43540 already vendored `pythoncapi_compat.h`, so closing #43069 by using this as well for `Py_IsFinalizing` (which was added in #42034, and for which we opened that follow-up issue to use `pythoncapi_compat.h` instead) Authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>

github-actions bot added Component: Python awaiting review Awaiting review labels Aug 2, 2024

pitrou requested changes Aug 6, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 6, 2024

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 6, 2024

pitrou reviewed Aug 6, 2024

View reviewed changes

python/pyarrow/src/arrow/python/platform.h Outdated Show resolved Hide resolved

pitrou reviewed Aug 6, 2024

View reviewed changes

python/pyarrow/src/arrow/python/pythoncapi_compat.h Outdated Show resolved Hide resolved

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 6, 2024

lysnikolaou mentioned this pull request Aug 7, 2024

[Python] Support for the free-threaded build of CPython 3.13 #43536

Closed

11 tasks

pitrou reviewed Aug 7, 2024

View reviewed changes

python/pyarrow/src/arrow/python/platform.h Outdated Show resolved Hide resolved

pitrou reviewed Aug 7, 2024

View reviewed changes

python/pyarrow/src/arrow/python/vendored/pythoncapi_compat.h Show resolved Hide resolved

pitrou changed the title ~~GH-43536: [Python] Do not use borrowed references APIs under free-threaded CPython~~ GH-43536: [Python] Do not use borrowed references APIs Aug 15, 2024

pitrou approved these changes Aug 15, 2024

View reviewed changes

lysnikolaou added 5 commits August 15, 2024 12:03

Do not use borrowed references APIs under free-threaded CPython

a427bd9

Add pythoncapi_compat and use strong reference APIs

ba84ac0

Move pythoncapi_compat.h to vendored subdirectory

d713b2a

Fix clang-format issues

dd76029

Do not include pythoncapi_compat.h from public headers

202de1a

pitrou force-pushed the remove-borrowed-references branch from 174a1ef to 202de1a Compare August 15, 2024 10:03

pitrou added 2 commits August 15, 2024 12:07

Fix lint

3f97169

Add RAT exclusion for vendored C++ files in PyArrow

7352f19

pitrou requested review from assignUser, jonkeane, kou and raulcd as code owners August 15, 2024 10:18

pitrou merged commit 894f72f into apache:main Aug 15, 2024
11 checks passed

pitrou removed the awaiting change review Awaiting change review label Aug 15, 2024

jorisvandenbossche mentioned this pull request Aug 20, 2024

GH-43069: [Python] Use Py_IsFinalizing from pythoncapi_compat.h #43767

Merged

This was referenced Sep 9, 2024

GH-43536: [Python][CI] Add a Crossbow job with the free-threaded build #43671

Merged

[Python] Fix threading bugs under the free-threaded version related to borrowed references and Pandas initialization #44046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43536: [Python] Do not use borrowed references APIs #43540

GH-43536: [Python] Do not use borrowed references APIs #43540

lysnikolaou commented Aug 2, 2024 •

edited by pitrou

Loading

github-actions bot commented Aug 2, 2024

jorisvandenbossche commented Aug 5, 2024 •

edited

Loading

lysnikolaou commented Aug 5, 2024

pitrou commented Aug 6, 2024

lysnikolaou commented Aug 6, 2024

lysnikolaou commented Aug 13, 2024

pitrou left a comment

pitrou commented Aug 15, 2024

github-actions bot commented Aug 15, 2024

pitrou commented Aug 15, 2024

conbench-apache-arrow bot commented Aug 15, 2024

GH-43536: [Python] Do not use borrowed references APIs #43540

GH-43536: [Python] Do not use borrowed references APIs #43540

Conversation

lysnikolaou commented Aug 2, 2024 • edited by pitrou Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

github-actions bot commented Aug 2, 2024

jorisvandenbossche commented Aug 5, 2024 • edited Loading

lysnikolaou commented Aug 5, 2024

pitrou commented Aug 6, 2024

lysnikolaou commented Aug 6, 2024

lysnikolaou commented Aug 13, 2024

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Aug 15, 2024

github-actions bot commented Aug 15, 2024

pitrou commented Aug 15, 2024

conbench-apache-arrow bot commented Aug 15, 2024

lysnikolaou commented Aug 2, 2024 •

edited by pitrou

Loading

jorisvandenbossche commented Aug 5, 2024 •

edited

Loading