Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-41475: [Python] Build with Python 3.13 #42034

Merged
merged 3 commits into from
Jun 26, 2024
Merged

Conversation

tacaswell
Copy link
Contributor

@tacaswell tacaswell commented Jun 7, 2024

Rationale for this change

The private function _Py_IsFinalizing renamed to Py_IsFinalizing in Python 3.13.0a1. Without this change pyarrow will not compile with this version of Python or higher.

What changes are included in this PR?

add a version-gated #define to handle the change.

Are these changes tested?

Local build succeeds.

Are there any user-facing changes?

no

Copy link

github-actions bot commented Jun 7, 2024

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

@kou kou changed the title GH-41475: [PYTHON] build with py313 GH-41475: [Python] Build with py313 Jun 8, 2024
@kou kou requested a review from pitrou June 8, 2024 20:13
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 8, 2024
@pitrou
Copy link
Member

pitrou commented Jun 10, 2024

Instead of doing such adjustements one by one, how about we vendor a version of https://github.com/python/pythoncapi-compat ? cc @jorisvandenbossche

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 10, 2024
tacaswell and others added 2 commits June 10, 2024 13:42
The private function `_Py_IsFinalizing` renamed to `Py_IsFinalizing` in Python
3.13.0a1
Co-authored-by: Sutou Kouhei <[email protected]>
@jorisvandenbossche
Copy link
Member

Instead of doing such adjustements one by one, how about we vendor a version of https://github.com/python/pythoncapi-compat ?

I am not very familiar with it to estimate how much we would make use of it (this PR also essentially only adds 3 lines, and the actual changes to change _Py_IsFinalizing to Py_IsFinalizing is still something that we would had to do). But yes, if that tool is exactly meant for this, maybe why not just use it.

I quickly tested its upgrade script on the pyarrow C++ code, and what it does change is replace a bunch of .. == Py_None cases with Py_IsNone(..).

But maybe the more interesting usage of the tool would be that we can use newer C API functions in our code that might make our PyArrow C++ easier to read or maintain? (but again not sure how much of those cases would actually apply here)

@jorisvandenbossche
Copy link
Member

(I am also fine with considering https://github.com/python/pythoncapi-compat for a follow-up issue, and at least ensure now pyarrow builds with Python 3.13)

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 11, 2024
Co-authored-by: Joris Van den Bossche <[email protected]>
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 11, 2024
@pitrou
Copy link
Member

pitrou commented Jun 11, 2024

I quickly tested its upgrade script on the pyarrow C++ code, and what it does change is replace a bunch of .. == Py_None cases with Py_IsNone(..).

But maybe the more interesting usage of the tool would be that we can use newer C API functions in our code that might make our PyArrow C++ easier to read or maintain? (but again not sure how much of those cases would actually apply here)

Yes, the idea would be to use newer C API functions as in this PR. Vendoring it should be trivial: there is a single .h file.

@jorisvandenbossche jorisvandenbossche changed the title GH-41475: [Python] Build with py313 GH-41475: [Python] Build with Python 3.13 Jun 19, 2024
@jorisvandenbossche
Copy link
Member

I don't have time this or coming week to look into vendoring pythoncapi-compat, ad given the upcoming feature freeze for 17.0 the week after, I would suggest we merge this for now and track using pythoncapi-compat as a follow-up issue.

@jorisvandenbossche jorisvandenbossche added this to the 17.0.0 milestone Jun 20, 2024
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 20, 2024
@raulcd
Copy link
Member

raulcd commented Jun 26, 2024

@github-actions crossbow submit -g python

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am running extra CI for this. If tests are successful I'll merge as is and we can open a follow up issue to vendor pythoncapi-compat as suggested

Copy link

Revision: 79c37cd

Submitted crossbow builds: ursacomputing/crossbow @ actions-d83aa38dd0

Task Status
example-python-minimal-build-fedora-conda GitHub Actions
example-python-minimal-build-ubuntu-venv GitHub Actions
test-conda-python-3.10 GitHub Actions
test-conda-python-3.10-cython2 GitHub Actions
test-conda-python-3.10-hdfs-2.9.2 GitHub Actions
test-conda-python-3.10-hdfs-3.2.1 GitHub Actions
test-conda-python-3.10-pandas-latest GitHub Actions
test-conda-python-3.10-pandas-nightly GitHub Actions
test-conda-python-3.10-spark-v3.5.0 GitHub Actions
test-conda-python-3.10-substrait GitHub Actions
test-conda-python-3.11 GitHub Actions
test-conda-python-3.11-dask-latest GitHub Actions
test-conda-python-3.11-dask-upstream_devel GitHub Actions
test-conda-python-3.11-hypothesis GitHub Actions
test-conda-python-3.11-pandas-upstream_devel GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
test-conda-python-3.12 GitHub Actions
test-conda-python-3.8 GitHub Actions
test-conda-python-3.8-pandas-1.0 GitHub Actions
test-conda-python-3.8-spark-v3.5.0 GitHub Actions
test-conda-python-3.9 GitHub Actions
test-conda-python-3.9-pandas-latest GitHub Actions
test-cuda-python GitHub Actions
test-debian-12-python-3-amd64 GitHub Actions
test-debian-12-python-3-i386 GitHub Actions
test-fedora-39-python-3 GitHub Actions
test-ubuntu-20.04-python-3 GitHub Actions
test-ubuntu-22.04-python-3 GitHub Actions

@raulcd
Copy link
Member

raulcd commented Jun 26, 2024

Failures are unrelated

@raulcd raulcd merged commit 1ce69ec into apache:main Jun 26, 2024
15 checks passed
@raulcd raulcd removed the awaiting merge Awaiting merge label Jun 26, 2024
@raulcd
Copy link
Member

raulcd commented Jun 26, 2024

I've created the follow up issue #43069 to vendor https://github.com/python/pythoncapi-compat

@tacaswell tacaswell deleted the fix/py313 branch June 26, 2024 21:01
Copy link

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

There were 12 benchmark results with an error:

There was 1 benchmark result indicating a performance regression:

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

zanmato1984 pushed a commit to zanmato1984/arrow that referenced this pull request Jul 9, 2024
### Rationale for this change

The private function `_Py_IsFinalizing` renamed to `Py_IsFinalizing` in Python 3.13.0a1.  Without this change pyarrow will not compile with this version of Python or higher.

### What changes are included in this PR?

add a version-gated `#define` to handle the change.

### Are these changes tested?

Local build succeeds. 

### Are there any user-facing changes?

no

* GitHub Issue: apache#41475

Lead-authored-by: Thomas A Caswell <[email protected]>
Co-authored-by: Joris Van den Bossche <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Raúl Cumplido <[email protected]>
jorisvandenbossche added a commit that referenced this pull request Aug 21, 2024
### 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants