-
-
Notifications
You must be signed in to change notification settings - Fork 6
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 #84
base: master
Are you sure you want to change the base?
Conversation
Hey folks, thanks for all the review and sorry for the extended delay here, I was out on PTO. I've addressed most of the comments around adding some TODOs and replied to the rest. |
CodSpeed Performance ReportMerging #84 will degrade performances by 8.05%Comparing Summary
Benchmarks breakdown
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #84 +/- ##
==========================================
+ Coverage 89.02% 89.03% +0.01%
==========================================
Files 18 18
Lines 738 739 +1
Branches 75 75
==========================================
+ Hits 657 658 +1
Misses 63 63
Partials 18 18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Gentle ping on this. Is there anything I can do to move it forward? |
packaging/pep517_backend/_backend.py
Outdated
@@ -372,7 +372,8 @@ def get_requires_for_build_wheel( | |||
) | |||
|
|||
c_ext_build_deps = [] if is_pure_python_build else [ | |||
'Cython ~= 3.0.0; python_version >= "3.12"', | |||
'Cython == 3.1.0a1; python_version >= "3.13"', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the performance regression is coming from 3.1.0a1.
We should only use 3.1.0a1 if its the free-threading 3.13 so this doesn't affect the current 3.13 production builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the performance regression is coming from 3.1.0a1.
We should only use 3.1.0a1 if its the free-threading 3.13 so this doesn't affect the current 3.13 production builds
That's not very easy to do if we continue using a constraint file for Cython since there's no marker for the free-threaded build. It seems though that we could remove the contraints from a couple of places:
- When building sdists and pure python wheels: Cython is probably not needed there.
- For the configuration environment in cibuildwheel: Can be removed, since the Cython versions that ends up being installed is the one specified in the build backend.
I'm not sure how we would do that for the test requirements though, since Cython is needed there as well. We would have to use something more complicated to be able to install the correct version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz would you kindly offer some guidance here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lysnikolaou sorry for the delay
I think this is good to go once https://github.com/aio-libs/propcache/pull/84/files#r1925771536 is addressed
c_ext_build_deps = [] | ||
elif sysconfig.get_config_var("Py_GIL_DISABLED"): | ||
# TODO: Remove when there's a Cython final with free-threading support | ||
c_ext_build_deps = ['Cython == 3.1.0a1'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not compat?
c_ext_build_deps = ['Cython == 3.1.0a1'] | |
c_ext_build_deps = ['Cython ~= 3.1.0a1'] |
It'd allow other versions below 3.1.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work for sure. There's still the problem described here however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest waiting for non-alpha Cython release. A Release Candidate is ok, but alpha is not.
It's a compile-time experimental feature on cpython, I don't think anybody is expecting us to have a rock solid supported implementation. It's essentially a preview release at this point, so alpha is fine at this stage (and the only thing we can offer). |
What do these changes do?
Are there changes in behavior for the user?
No.
Related issue number
Checklist