-
-
Notifications
You must be signed in to change notification settings - Fork 170
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
Support the free-threaded build of CPython 3.13 #1456
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
CodSpeed Performance ReportMerging #1456 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1456 +/- ##
==========================================
- Coverage 95.57% 95.22% -0.35%
==========================================
Files 33 33
Lines 6010 6032 +22
Branches 364 364
==========================================
Hits 5744 5744
- Misses 240 262 +22
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
78f84fd
to
1c857ca
Compare
9bfa0d3
to
44ee894
Compare
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
for more information, see https://pre-commit.ci
4c8fe2f
to
3527edc
Compare
One alternative to this would be to use |
I'm still looking into the uv integration, I'm not sure it's ready for prime time just yet. I'll be looking into that separately. But feel free to send a PR with the caching fix separately. |
The performance regression in the writer looks real. I can't come up with a better solution though that is thread-safe. |
We could try to make it so that the global buffer is used in the non-free-threaded build, but not in the free-threaded one. Cython, however, makes that particularly hard. There's two possible solutions:
Both of the solutions are sub-optimal. Thoughts? |
The |
Are there any plans to expose Py_GIL_DISABLED in Cython? If so, might make more sense to come back and fix it when that happens. If we do want to write it without Cython, should we consider Rust instead of C? |
Co-authored-by: J. Nick Koston <[email protected]>
buf = <char *> PyMem_Malloc(BUF_SIZE) | ||
if buf == NULL: | ||
PyErr_NoMemory() | ||
return | ||
writer.buf = buf |
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 is this branch marked as not covered? https://app.codecov.io/gh/aio-libs/yarl/commit/dcb3ae1102d49a77b4bd824839c9a72c80cc229c?flags%5B0%5D=pytest&dropdown=coverage#fec01998dabbadc38df09608935ec4b7-R98
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 this might be because the test job installs older cython: https://github.com/aio-libs/yarl/actions/runs/13526871108/job/37800294520?pr=1456#step:8:3. Could you try updating it there to see if it helps?
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.
This is probably happening because the quansight-labs/setup-python
action returns the version number without the trailing t
so one set of coverage results might be overriding the other. I've fixed that.
Also opend #1480 to get rid of the hypothesis warnings.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
41c79ea
to
ace56a4
Compare
What do these changes do?
Are there changes in behavior for the user?
No.
Related issue number
Resolves #1455
Checklist