-
Notifications
You must be signed in to change notification settings - Fork 835
Bump supported cpython version to 3.14 for testing #4811
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
Conversation
Thanks for the PR. I am not comfortable bumping this check at this point. If we did, unsuspecting users will likely install software built with the next PyO3 version (call it 0.24) when 3.14 stable lands... and they will likely have a terrible experience with random crashes and other frustrating hard-to-debug and dangerous issues. At the same time, I appreciate the desire to get testing with 3.14 and that PyO3 should not be a blocker for this. See #4662 (comment) We also have an undocumented environment variable If you are testing PyO3 with 3.14 you may need to make patches to |
To investigate:
|
@davidhewitt Might need your set of eyeballs, something has changed significantly in regards to 3.14 with strings representation(?) |
This is a great example of the kind of churn which premature 3.14 support will burden us with. The layout of Personally I'm disinterested in fixing it for an alpha which might yet churn again, I suggest just disabling this API on 3.14 with a note that we would prefer not to reintroduce it. |
It's great to test in alphas, but PyO3 probably isn't one of the projects that should do that, at least for the non-stable APIs; we mess with the internals all the time in alphas. I'd wait until the beta freeze (or to at least get close to it) before trying to test 3.14. I also personally don't think PyO3 support for it would help catch too many bugs:
All that said, it might be worth trying to support 3.14 early for the limited API only, because we have to ensure stability for that anyway. I don't know how feasible that is for you guys, though. |
4c94698
to
ab16e04
Compare
CI passes, might need to consolidate commits |
e3447aa
to
df9a455
Compare
CodSpeed Performance ReportMerging #4811 will not alter performanceComparing Summary
|
Good news! I was able to fix I got part way through updating the FFI for the GIL-disabled build but there have been some pretty substantial changes to refcounting (at least, there might be more) and it's requiring a bit more of a fine-toothed perusal of the headers to make sure everything is right. I'll go ahead and push just the updates for the GIL-enabled build and will come back another day with the GIL-disabled updates as well as some substantial refactoring to reflect upstream refactoring and changes. |
btw, you may want to redo this PR so it isn't from the |
Looking again, I think it's mostly just a refactor? So maybe not so bad. |
(accidentally posted this on the issue instead of here, sorry for the double notification) The last pushes fix ffi-check on the free-threaded build but unfortunately it breaks it on the GIL-enabled build. @davidhewitt it looks like CPython made a change that breaks ffi-check, similar to how it was broken in Python 3.12 - See here inside CPython for what I'm talking about. In ffi-check there's this code you wrote in 2023: pyo3/pyo3-ffi-check/macro/src/lib.rs Lines 148 to 155 in 241080a
Do you happen to know how I should I modify it now that there's an inline struct in the union as well? |
Go ahead |
8cef308
to
471bd35
Compare
Any tips on handling no-GIL specifics within the FFI check? |
You'll need to install a free-threaded Python build and run ffi-check locally. See https://py-free-threading.github.io/installing_cpython/. Any discrepancies flagged by the tests relative to the headers should be corrected. That said, it looks like everything passed on the last run? The free-threaded tests run ffi-check. |
Sorry, I mean to clarify how to handle them within the macro itself... |
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.
Thanks for all the work here.
I think the HangThread
change is correct; to me it looks like other changes in CPython have made it possible to crash the interpreter when allocating a new thread state during finalization. I reported this in python/cpython#132948
I think it would be good to merge this branch and then iterate further on main
there's just a small number of corrections that need to be made, also the check-feature-powerset
job has correctly identified the abi3-py314
feature needs to be added to the root Cargo.toml
.
pyo3-build-config/Cargo.toml
Outdated
abi3-py313 = ["abi3"] | ||
abi3-py314 = ["abi3"] |
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.
abi3-py313 = ["abi3"] | |
abi3-py314 = ["abi3"] | |
abi3-py313 = ["abi3-py314"] | |
abi3-py314 = ["abi3"] |
pyo3-ffi/Cargo.toml
Outdated
@@ -35,6 +35,7 @@ abi3-py310 = ["abi3-py311", "pyo3-build-config/abi3-py310"] | |||
abi3-py311 = ["abi3-py312", "pyo3-build-config/abi3-py311"] | |||
abi3-py312 = ["abi3-py313", "pyo3-build-config/abi3-py312"] | |||
abi3-py313 = ["abi3", "pyo3-build-config/abi3-py313"] |
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.
abi3-py313 = ["abi3", "pyo3-build-config/abi3-py313"] | |
abi3-py313 = ["abi3-py314", "pyo3-build-config/abi3-py313"] |
pyo3-ffi/src/cpython/code.rs
Outdated
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.
We can hopefully just revert the changes in this file now that #5064 is merged
pyo3-ffi/src/cpython/dictobject.rs
Outdated
pub ma_version_tag: u64, | ||
#[cfg(Py_3_14)] | ||
pub _ma_watcher_tag: u64, |
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.
Let's make this private field inaccessible unless we have good reason to allow it.
pub _ma_watcher_tag: u64, | |
_ma_watcher_tag: u64, |
pyo3-ffi/src/cpython/initconfig.rs
Outdated
pub thread_inherit_context: c_int, | ||
#[cfg(Py_3_14)] | ||
pub context_aware_warnings: c_int, | ||
// FIXME: this was backported to 3.13.2 |
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.
And reverted in 3.13.3 :)
// FIXME: this was backported to 3.13.2 |
pyo3-ffi/src/refcount.rs
Outdated
use std::sync::atomic::Ordering::Relaxed; | ||
|
||
#[cfg(Py_3_14)] | ||
pub const _Py_STATICALLY_ALLOCATED_FLAG: c_int = 1 << 7; |
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.
pub const _Py_STATICALLY_ALLOCATED_FLAG: c_int = 1 << 7; | |
const _Py_STATICALLY_ALLOCATED_FLAG: c_int = 1 << 7; |
(or at most pub(crate)
)
pyo3-ffi/src/refcount.rs
Outdated
#[cfg(Py_GIL_DISABLED)] | ||
pub const _Py_IMMORTAL_REFCNT_LOCAL: u32 = u32::MAX; | ||
|
||
#[cfg(Py_GIL_DISABLED)] | ||
pub const _Py_REF_SHARED_SHIFT: isize = 2; |
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.
#[cfg(Py_GIL_DISABLED)] | |
pub const _Py_IMMORTAL_REFCNT_LOCAL: u32 = u32::MAX; | |
#[cfg(Py_GIL_DISABLED)] | |
pub const _Py_REF_SHARED_SHIFT: isize = 2; | |
#[cfg(Py_GIL_DISABLED)] | |
const _Py_IMMORTAL_REFCNT_LOCAL: u32 = u32::MAX; | |
#[cfg(Py_GIL_DISABLED)] | |
const _Py_REF_SHARED_SHIFT: isize = 2; |
pyo3-ffi/src/refcount.rs
Outdated
((6 as c_long) << (28 as c_long)) as Py_ssize_t; | ||
|
||
#[cfg(all(Py_3_14, Py_GIL_DISABLED))] | ||
pub const _Py_IMMORTAL_INITIAL_REFCNT: Py_ssize_t = c_uint::MAX as Py_ssize_t; |
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.
pub const _Py_IMMORTAL_INITIAL_REFCNT: Py_ssize_t = c_uint::MAX as Py_ssize_t; | |
const _Py_IMMORTAL_INITIAL_REFCNT: Py_ssize_t = c_uint::MAX as Py_ssize_t; |
@clin1234 hope you don't mind - I'm going to apply David's comments and try to get this into a mergeable state today. |
* Introspection: add function signatures No annotations or explicit default values yet Fixes an issue related to object identifiers path * Better default value * Refine arguments struct * Introduce VariableLengthArgument * Adds pyfunctions tests * Adds some serialization tests
In the meantime, what should we do here to avoid a segfault when the tests finish? |
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 think for now let's just mark the pytests step as continue-on-error for dev versions, like we do for the whole job? That way we'll still upload coverage.
Also I think we should probably move ffi-check
to run before all testing, because I would think if the ffi definitions are broken then ffi-check would help stop segfaults caused our end.
pytests/pytest.ini
Outdated
@@ -0,0 +1,3 @@ | |||
# see gh-5094 for details |
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.
Maybe use a PyO3 issue URL, I think I might get confused with a cpython issue otherwise.
Alright, I think this is ready now. |
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.
Thank you both for doing so much hard work here!
* Add news item * Move news file * Fix version limit check in noxfile.py * Bump Python version for testing debug builds * 3.14 is available from GH's setup-python action * Bump maximum supported CPython version in pyo3-ffi * Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14 Due to python/cpython#128196, data types within `PyASCIIObject.state` have changed, resulting in test failures when building against 3.14. * Run `cargo fmt --all` * Actually add Py_3_14 as a legitimate macro When `rustc` is invoked, the macro is included with the `--check-cfg` flag, but not with the `--cfg` flag. This caused errors about duplicate definitions to spew out when building with stable Rust toolchains. * Revert "Actually add Py_3_14 as a legitimate macro" This reverts commit 5da57af. * Fix version macro placement for 3.14-specific getters and setters * Import 'c_ushort' only if compiling against CPython 3.14 or later * Add wrapper functions for the statically_allocated field * Remove unused libc::c_ushort * Add (hopefully) final version-specific macros * Port 3.14-specific 64-bit code of Py_INCREF * Don't expose PyDictObject.ma_version_tag when building against 3.14 or later * fix ffi-check on the GIL-enabled ABI * fix older pythons * fix ffi-check on older pythons * WIP: update for 3.14t * fix ffi-check on the free-threaded build * fix clippy * fix clippy on older python versions * fix cargo check on the MSRV * fix ffi-check on 3.13t * fix CI which is using 3.13.1 * fix copy/paste error in noxfile * update ffi bindings for the latest changes in 3.14 * update layout of refcnt field on gil-enabled build * delete unused HangThread struct * fix ffi-check on GIL-enabled build * Revert "delete unused HangThread struct" This reverts commit 3dd439d. * config-out HangThread * fix 3.13 ffi-check * fix debug python build error * fix graalpy build * Ignore DeprecationWarnings from the pytest_asyncio module in tests * Add abi3-py314 * fix free-threading issue in `test_coroutine` (PyO3#5069) * Introspection: add function signatures (PyO3#5025) * Introspection: add function signatures No annotations or explicit default values yet Fixes an issue related to object identifiers path * Better default value * Refine arguments struct * Introduce VariableLengthArgument * Adds pyfunctions tests * Adds some serialization tests * respond to david's code review * add comment and fix test failure * fix check-feature-powerset * fix clippy * fix wasip1 clippy * fix 32 bit python 3.14 bug * mark test-py step continue-on-error for dev python builds * use github issue URL * run ffi-check before running tests * fix ffi-check for 3.14.0a7 --------- Co-authored-by: Nathan Goldbaum <[email protected]> Co-authored-by: David Hewitt <[email protected]> Co-authored-by: Thomas Tanon <[email protected]>
This is to mainly silence this abominable error: