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

Implement support for the free-threaded build of CPython 3.13 #1015

Merged
merged 13 commits into from
Oct 19, 2024

Conversation

lysnikolaou
Copy link
Contributor

What do these changes do?

Implement support for the free-threaded build of CPython 3.13!

In more detail, this PR does the following:

  • Add a CI job that runs the tests under the free-threaded build.
  • Enable building free-threaded wheels.
  • Add pythoncapi_compat.h and move away from all the borrowed reference APIs.
  • Mark extension module as thread-safe
  • Add locking around pair_list_global_version.

Are there changes in behavior for the user?

There shouldn't be any.

Related issue number

Related to #1014.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Sep 18, 2024
@bdraco
Copy link
Member

bdraco commented Sep 18, 2024

The changes look good to me and I didn't see any ref count issues, however I am by no means an expert on the C API

@bdraco
Copy link
Member

bdraco commented Sep 18, 2024

Assuming the CI passes (might take a while to run since there is a lot of activity right now), I'd like another maintainer who has more experience with the C API to take a look before merging.

@bdraco
Copy link
Member

bdraco commented Sep 18, 2024

Thanks @lysnikolaou

CHANGES/1015.feature.rst Outdated Show resolved Hide resolved
@bdraco
Copy link
Member

bdraco commented Sep 18, 2024

It looks like one of the existing actions unrelated to this PR is using an old upload-artifact. It would be nice if github told us which one.

@lysnikolaou
Copy link
Contributor Author

Yeah, I saw that as well. Opened #1016 for it.

@bdraco
Copy link
Member

bdraco commented Sep 22, 2024

Looks like its almost there 👍

@lysnikolaou
Copy link
Contributor Author

Looks like it's good to go on my side!

@bdraco
Copy link
Member

bdraco commented Sep 23, 2024

LGTM, but I'd still like another set of 👀 #1015 (comment) before merging

@lysnikolaou
Copy link
Contributor Author

Friendly ping here!

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Here's my CI/CD feedback. I can't say I'm experienced with C-API, however. Still, I think we can merge this and start a release process to see if more rare architectures also succeed building the new wheels. It should be enough to see if cibuildwheel passes in those jobs that only run on release, since it also runs some testing. We can then abort the release once we know. Or we could just attempt a regular release.

.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
CHANGES/1015.feature.rst Show resolved Hide resolved
.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
@lysnikolaou lysnikolaou force-pushed the free-threading-support branch from 899a644 to 41caba0 Compare October 15, 2024 13:31
@lysnikolaou
Copy link
Contributor Author

Thanks for the reviews everyone! I've addressed all of the feedback points.

@bdraco
Copy link
Member

bdraco commented Oct 15, 2024

It looks like we don't have any benchmarks for this repository so can't check the performance impact. I guess I'll need to write some.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.86%. Comparing base (217d16e) to head (a60248d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1015   +/-   ##
=======================================
  Coverage   94.86%   94.86%           
=======================================
  Files          23       23           
  Lines        2686     2686           
  Branches      254      254           
=======================================
  Hits         2548     2548           
  Misses         67       67           
  Partials       71       71           
Flag Coverage Δ
CI-GHA 94.86% <ø> (ø)
MyPy 67.51% <ø> (ø)
pytest 98.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This was referenced Oct 16, 2024
@bdraco bdraco force-pushed the free-threading-support branch from 41caba0 to b302ec3 Compare October 17, 2024 00:36
@webknjaz webknjaz enabled auto-merge (squash) October 18, 2024 18:03
@bdraco
Copy link
Member

bdraco commented Oct 18, 2024

It looks like the testing fails because there is no cffi wheel for free-threaded 3.13 so it tries to build it and since libffi isn't installed in the cibuildwheel env, it fails

@bdraco bdraco disabled auto-merge October 18, 2024 19:01
Copy link

codspeed-hq bot commented Oct 18, 2024

CodSpeed Performance Report

Merging #1015 will not alter performance

Comparing lysnikolaou:free-threading-support (a60248d) with master (217d16e)

Summary

✅ 38 untouched benchmarks

@webknjaz webknjaz merged commit 888553e into aio-libs:master Oct 19, 2024
48 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants