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

Add free-threaded Python support #2310

Merged
merged 8 commits into from
Nov 25, 2024
Merged

Add free-threaded Python support #2310

merged 8 commits into from
Nov 25, 2024

Conversation

messense
Copy link
Member

@messense messense commented Nov 17, 2024

Implementation details:

  • Free-threaded support is determined by the Py_GIL_DISABLED build flag for runnable Python interpreters
  • We also accept python3.13t when cross compiling

Closes #2298
Closes #2315

@messense
Copy link
Member Author

messense commented Nov 17, 2024

This change is part of the following stack:

Change managed by git-spice.

@davidhewitt
Copy link
Member

For testing it's possible to use https://github.com/Quansight-Labs/setup-python as a drop-in replacement for the GitHub one

@messense messense requested a review from Copilot November 20, 2024 07:48

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (1)

src/python_interpreter/config.rs:131

  • The handling of abiflags for free-threaded Python on Windows is incomplete. This could lead to unexpected behavior if free-threaded Python is used on Windows.
// FIXME: windows abiflags for free-threaded python?
@messense messense force-pushed the free-threaded-python branch 3 times, most recently from ffbf35a to ba5b918 Compare November 20, 2024 13:11
@ngoldbaum
Copy link
Contributor

Happy to help with this if needed. The windows free-threaded packaging situation is "fun".

@messense messense force-pushed the free-threaded-python branch from dd2379a to ae896ff Compare November 21, 2024 13:53
@messense
Copy link
Member Author

Happy to help with this if needed.

Thanks, feel free to send PR to this branch!

@messense messense force-pushed the free-threaded-python branch from ae896ff to 22add66 Compare November 21, 2024 14:29
@ngoldbaum
Copy link
Contributor

It looks like a few of the tests hardcode using abi3:

○  rg "abi3" **/Cargo.toml
pyo3-pure/Cargo.toml
10:pyo3 = { version = "0.22.0", features = ["abi3-py37", "extension-module", "generate-import-lib"] }

pyo3-abi3-without-version/Cargo.toml
2:name = "pyo3-abi3-without-version"
8:pyo3 = { version = "0.22.4", features = ["abi3", "extension-module"] }
11:name = "pyo3_abi3_without_version"

workspace-inverted-order/path-dep-with-root/Cargo.toml
13:pyo3 = { version = "0.22.5", features = ["abi3", "abi3-py38", "extension-module"] }

pyo3-ffi-pure/Cargo.toml
7:pyo3-ffi = { version = "0.18.1", features = ["abi3-py37", "extension-module"] }

Since free-threading and abi3 are incompatible, would it be better to skip over the tests that use these crates? For the ones that are testing whether the limited api support is working correctly I think skipping them makes sense, but some of these seem to be more generic integration tests for maturin that happen to use the limited API.

I don't see an easy way to optionally turn on and off the abi3 feature in the test crates. Am I missing something? If not, any guidance?

@davidhewitt
Copy link
Member

We should probably try to support this case because cryptography probably hard-codes the abi3 build in the same way. It's useful to do that because then the whole system limits you to the limited API.

My gut says that it'd be good enough for now if Py_GIL_DISABLED being set caused pyo3-build-config to no-op the whole lot of abi3 features. So maybe there's a PyO3 fix required here? 🤔

@ngoldbaum
Copy link
Contributor

My gut says that it'd be good enough for now if Py_GIL_DISABLED being set caused pyo3-build-config to no-op the whole lot of abi3 features. So maybe there's a PyO3 fix required here?

Hmm, that's sort of in the vein of PyO3/pyo3#4719, but maybe what I'm doing there making more things hard errors is bad and it would be better to warn and just turn off limited API and set the ABI to Py_3_13 if someone is trying to build for the free-threaded ABI but asks for an earlier ABI.

@alex
Copy link

alex commented Nov 21, 2024

We should probably try to support this case because cryptography probably hard-codes the abi3 build in the same way. It's useful to do that because then the whole system limits you to the limited API.

We have a convoluted setup where Cargo.toml specifies the abi3 feature, pyproject.toml specifies the pyo3/abi3-py37 feature, and then some individual wheel building jobs specify higher abi3 ABIs.

@davidhewitt
Copy link
Member

👍 It seems to me that we probably don't want cryptography to have to remove abi3 feature from Cargo.toml to build against the freethreaded build, so silently overriding (like we do for PyPy & Graalpy) seems more correct. I guess we can probably also make that work for the abi3-py37 too, I'll check how that works...

@davidhewitt
Copy link
Member

Yep, if I add abi3-py37 to Cargo.toml and then try to build with PyPy, the build succeeds (with a warning about PyPy not supporting abi3), I guess we should match here.

@ngoldbaum
Copy link
Contributor

It looks like three of the crates use old PyO3 APIs and need to be updated:

test-crates/sdist_with_target_path_dep/src/lib.rs
18:    let pool = ::pyo3::GILPool::new();
116:    let pool = ::pyo3::GILPool::new();

test-crates/lib_with_path_dep/src/lib.rs
18:    let pool = ::pyo3::GILPool::new();
116:    let pool = ::pyo3::GILPool::new();

test-crates/sdist_with_path_dep/src/lib.rs
18:    let pool = ::pyo3::GILPool::new();
116:    let pool = ::pyo3::GILPool::new();

Other errors from trying to build one of these crates:

   Compiling sdist_with_path_dep v0.1.0 (/Users/goldbaum/Documents/maturin/test-crates/sdist_with_path_dep)
error[E0432]: unresolved import `pyo3::derive_utils`
   --> src/lib.rs:113:15
    |
113 |     use pyo3::derive_utils::ModuleDef;
    |               ^^^^^^^^^^^^ could not find `derive_utils` in `pyo3`

error[E0433]: failed to resolve: could not find `GILPool` in `pyo3`
  --> src/lib.rs:18:24
   |
18 |     let pool = ::pyo3::GILPool::new();
   |                        ^^^^^^^ could not find `GILPool` in `pyo3`

error[E0433]: failed to resolve: could not find `derive_utils` in `pyo3`
  --> src/lib.rs:26:47
   |
26 |                 const PARAMS: &'static [pyo3::derive_utils::ParamDescription] = &[
   |                                               ^^^^^^^^^^^^ could not find `derive_utils` in `pyo3`

error[E0433]: failed to resolve: could not find `derive_utils` in `pyo3`
  --> src/lib.rs:27:27
   |
27 |                     pyo3::derive_utils::ParamDescription {
   |                           ^^^^^^^^^^^^ could not find `derive_utils` in `pyo3`

error[E0433]: failed to resolve: could not find `derive_utils` in `pyo3`
  --> src/lib.rs:32:27
   |
32 |                     pyo3::derive_utils::ParamDescription {
   |                           ^^^^^^^^^^^^ could not find `derive_utils` in `pyo3`

error[E0433]: failed to resolve: could not find `derive_utils` in `pyo3`
  --> src/lib.rs:41:46
   |
41 |                 let (_args, _kwargs) = pyo3::derive_utils::parse_fn_args(
   |                                              ^^^^^^^^^^^^ could not find `derive_utils` in `pyo3`

error[E0433]: failed to resolve: could not find `derive_utils` in `pyo3`
  --> src/lib.rs:53:44
   |
53 |                         .map_err(|e| pyo3::derive_utils::argument_extraction_error(_py, "x", e))?,
   |                                            ^^^^^^^^^^^^ could not find `derive_utils` in `pyo3`

error[E0433]: failed to resolve: could not find `derive_utils` in `pyo3`
  --> src/lib.rs:61:44
   |
61 |                         .map_err(|e| pyo3::derive_utils::argument_extraction_error(_py, "y", e))?,
   |                                            ^^^^^^^^^^^^ could not find `derive_utils` in `pyo3`

error[E0433]: failed to resolve: could not find `callback` in `pyo3`
  --> src/lib.rs:85:17
   |
85 |         ::pyo3::callback::callback_error()
   |                 ^^^^^^^^ could not find `callback` in `pyo3`

error[E0433]: failed to resolve: could not find `derive_utils` in `pyo3`
  --> src/lib.rs:89:27
   |
89 |     args: impl Into<pyo3::derive_utils::PyFunctionArguments<'a>>,
   |                           ^^^^^^^^^^^^ could not find `derive_utils` in `pyo3`

error[E0433]: failed to resolve: could not find `GILPool` in `pyo3`
   --> src/lib.rs:116:24
    |
116 |     let pool = ::pyo3::GILPool::new();
    |                        ^^^^^^^ could not find `GILPool` in `pyo3`

error[E0433]: failed to resolve: could not find `callback` in `pyo3`
   --> src/lib.rs:137:17
    |
137 |         ::pyo3::callback::callback_error()
    |                 ^^^^^^^^ could not find `callback` in `pyo3`

error[E0433]: failed to resolve: could not find `callback` in `pyo3`
  --> src/lib.rs:22:17
   |
22 |         ::pyo3::callback::convert(_py, {
   |                 ^^^^^^^^ could not find `callback` in `pyo3`

error[E0433]: failed to resolve: could not find `callback` in `pyo3`
   --> src/lib.rs:120:17
    |
120 |         ::pyo3::callback::convert(_py, MODULE_DEF.make_module("", pyo3_pure))
    |                 ^^^^^^^^ could not find `callback` in `pyo3`

warning: use of deprecated type alias `pyo3::PyMethodType`: PyO3 implementation detail
  --> src/lib.rs:97:22
   |
97 |         pyo3::class::PyMethodType::PyCFunctionWithKeywords(__pyo3_raw_add),
   |                      ^^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

error[E0061]: this function takes 3 arguments but 5 arguments were supplied
   --> src/lib.rs:94:5
    |
94  |     pyo3::types::PyCFunction::internal_new(
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
95  |         name,
    |         ---- expected `Python<'_>`, found `&CStr`
96  |         doc,
    |         --- expected `&PyMethodDef`, found `&CStr`
97  |         pyo3::class::PyMethodType::PyCFunctionWithKeywords(__pyo3_raw_add),
98  |         pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS,
    |         -------------------------------------------------- unexpected argument #4 of type `i32`
99  |         args.into(),
    |         ----------- unexpected argument #5
    |
    = note: expected reference `&pymethods::PyMethodDef`
               found reference `&CStr`
note: expected `Option<&Bound<'_, ...>>`, found `PyMethodType`
   --> src/lib.rs:97:9
    |
97  |         pyo3::class::PyMethodType::PyCFunctionWithKeywords(__pyo3_raw_add),
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: expected enum `Option<&pyo3::Bound<'_, pyo3::types::PyModule>>`
               found enum `PyMethodType`
note: associated function defined here
   --> /Users/goldbaum/Documents/pyo3/src/types/function.rs:151:12
    |
151 |     pub fn internal_new<'py>(
    |            ^^^^^^^^^^^^
help: remove the extra arguments
    |
95  ~         /* Python<'_> */,
96  ~         /* &pymethods::PyMethodDef */,
97  ~         /* Option<&pyo3::Bound<'_, pyo3::types::PyModule>> */,
    |

error[E0308]: mismatched types
   --> src/lib.rs:94:5
    |
90  |   ) -> pyo3::PyResult<&'a pyo3::types::PyCFunction> {
    |        -------------------------------------------- expected `Result<&'a pyo3::types::PyCFunction, PyErr>` because of return type
...
94  | /     pyo3::types::PyCFunction::internal_new(
95  | |         name,
96  | |         doc,
97  | |         pyo3::class::PyMethodType::PyCFunctionWithKeywords(__pyo3_raw_add),
98  | |         pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS,
99  | |         args.into(),
100 | |     )
    | |_____^ expected `Result<&PyCFunction, PyErr>`, found `Result<Bound<'_, ...>, ...>`
    |
    = note: expected enum `Result<&'a pyo3::types::PyCFunction, _>`
               found enum `Result<pyo3::Bound<'_, pyo3::types::PyCFunction, >, _>`

error[E0277]: the trait bound `Result<&pyo3::types::PyCFunction, PyErr>: IntoPyCallbackOutput<'_, Py<PyAny>>` is not satisfied
   --> src/lib.rs:104:7
    |
104 |     m.add_wrapped(&__pyo3_get_function_add)?;
    |       ^^^^^^^^^^^ the trait `IntoPyCallbackOutput<'_, Py<PyAny>>` is not implemented for `Result<&pyo3::types::PyCFunction, PyErr>`
    |
    = help: the trait `IntoPyCallbackOutput<'py, U>` is implemented for `Result<T, E>`
note: required by a bound in `add_wrapped`
   --> /Users/goldbaum/Documents/pyo3/src/types/module.rs:311:12
    |
309 |     fn add_wrapped<T>(&self, wrapper: &impl Fn(Python<'py>) -> T) -> PyResult<()>
    |        ----------- required by a bound in this associated function
310 |     where
311 |         T: IntoPyCallbackOutput<'py, PyObject>;
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `PyModuleMethods::add_wrapped`

Some errors have detailed explanations: E0061, E0277, E0308, E0432, E0433.
For more information about an error, try `rustc --explain E0061`.
warning: `sdist_with_path_dep` (lib) generated 1 warning
error: could not compile `sdist_with_path_dep` (lib) due to 17 previous errors; 1 warning emitted

I'm confused why the update to PyO3 0.23 last week didn't break tests using these crates on CI.

@messense messense force-pushed the free-threaded-python branch from 9c7dc83 to 417eb38 Compare November 24, 2024 14:08
@messense messense marked this pull request as ready for review November 24, 2024 14:11
@messense messense requested a review from Copilot November 24, 2024 14:42

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated no suggestions.

Files not reviewed (3)
  • src/auditwheel/audit.rs: Evaluated as low risk
  • src/build_options.rs: Evaluated as low risk
  • src/cross_compile.rs: Evaluated as low risk
@messense
Copy link
Member Author

I'm confused why the update to PyO3 0.23 last week didn't break tests using these crates on CI.

Some of these tests are not built but only used to test generating source distribution, will fix them later in a separate PR.

@messense messense merged commit 65404af into main Nov 25, 2024
35 of 38 checks passed
@messense messense deleted the free-threaded-python branch November 25, 2024 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support building free-threaded wheels Support for 3.13t interpreter "version" in preparation for PyO3 v0.23
4 participants