add maximum Python version check #3821

davidhewitt · 2024-02-11T03:20:53Z

This PR seeks to protect users from compiling PyO3 against Python versions not yet supported, and the undefined behaviour that might result from doing so. There are two groups of people who might do this, and how they are protected is slightly different:

People installing an outdated package from PyPI who end up building from source against too new a Python version.
These users will see an error that their Python version is not yet supported by PyO3 and see a hint to use PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1 to opt-in to using the stable ABI's for forward compatibility.

error: failed to run custom build command for `pyo3-ffi v0.21.0-dev (/Users/david/Dev/pyo3/pyo3-ffi)`

Caused by:
  process didn't exit successfully: `/Users/david/Dev/pyo3/target/debug/build/pyo3-ffi-e3e7d0d040c4f908/build-script-build` (exit status: 1)
  --- stdout
  cargo:rerun-if-env-changed=PYO3_PRINT_CONFIG
  cargo:rerun-if-env-changed=PYO3_USE_ABI3_FORWARD_COMPATIBILITY

  --- stderr
  error: the configured Python interpreter version (3.13) is newer than PyO3's maximum supported version (3.12)
  = help: please check if an updated version of PyO3 is available. Current version: 0.21.0-dev
  = help: set PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1 to suppress this check and build anyway using the stable ABI

By setting the environment variable, we will build as if the abi3 feature was enabled.

People testing an in-development Python version and/or PyO3 and willing to accept the risks of broken stuff.
These users can turn off the check with UNSAFE_PYO3_SKIP_VERSION_CHECK=1.

Some worthwhile points of discussion:

Do we need to bother checking this works in CI? I felt it didn't hurt, but I can revert the CI changes if needed.
Should we document UNSAFE_PYO3_SKIP_VERSION_CHECK? I decided to punt on that for now.

davidhewitt · 2024-02-11T03:23:06Z

pyo3-macros-backend/src/utils.rs

+pub(crate) fn is_abi3() -> bool {
+    cfg!(feature = "abi3")
+        || std::env::var_os("PYO3_USE_ABI3_FORWARD_COMPATIBILITY")
+            .map_or(false, |os_str| os_str == "1")
+}


It's a bit unfortunate to have this duplication here in the macro code, but unfortunately we don't depend on pyo3-build-config since #2243. Given the FIXME I added to method.rs I would prefer to not worry about this for now and tidy up in future.

But is it really necessary? Couldn't we use cargo:rust-cfg in the build script to automatically enable the abi3 feature, e.g. by printing cargo:rust-cfg=feature=abi3?

I think that's better, yes, as it ensures things are in sync. I'll change to that, even if we end up reworking this eventually for the FIXME.

Ah, actually, it looks like pyo3-macros-backend does not currently have a build.rs, and it's not clear to me that it'd be correct to try to enable this feature from PyO3's main build.rs. Would you be open to just leaving this as it is, rather than introducing a new build script?

it's not clear to me that it'd be correct to try to enable this feature from PyO3's main build.rs.

This is what I would have gone for. The environment variable is basically a way to add the abi3 to any Cargo.toml referencing pyo3 without being able to edit said Cargo.toml files. This would then do anything else that is happening based on enabling that feature, including properly configuring our macro crates. But I'd say that pyo3's self-configuration is the right place for this.

Right, but quoting the Cargo docs:

Note that this does not affect Cargo’s dependency resolution. This cannot be used to enable an optional dependency, or enable other Cargo features.

So I think that adding cargo:rust-cfg=feature="pyo3-macros-backend/abi3 to the build script won't work as intended?

I think instead what I'll do is push a commit to revert #2243 and pull the configuration from pyo3-build-config again, and centralise it there.

So I think that adding cargo:rust-cfg=feature="pyo3-macros-backend/abi3 to the build script won't work as intended?

I don't think the build script of crate X can directly influence the compilation of crate Y even if Y is a dependency of X. My idea here, was to only emit cargo:rust-cfg=feature=abi and have the feature forwarding from Cargo.toml "just work", but I guess this is my think'o: This is not really enabling the feature from Cargo's point of view, but just passing compiler flags as if the feature were enabled.

I think instead what I'll do is push a commit to revert #2243 and pull the configuration from pyo3-build-config again, and centralise it there.

Sorry that this became a rabbit hole which you actually anticipated and wanted to avoid. But I also think that spooky-action-at-a-distance via environment variables is a good thing to minimise as much as possible, so I would indeed be glad if the above could be implemented. Thanks!

davidhewitt · 2024-02-11T04:21:49Z

Once merged, I would like to ship this in a 0.20.3 patch release. It shouldn't break any valid existing uses, and will mean that packages built on 0.20 will be safe when 3.13 lands.

.github/workflows/ci.yml

adamreichold · 2024-02-11T09:18:29Z

Should we document UNSAFE_PYO3_SKIP_VERSION_CHECK? I decided to punt on that for now.

I think the people who are expected to usefully do this (testing against future CPython versions) can be expected to be able to read the code since they have to do that anyway to figure out any resulting breakage.

pyo3-build-config/src/impl_.rs

codspeed-hq · 2024-02-12T23:56:20Z

CodSpeed Performance Report

Merging #3821 will degrade performances by 16.95%

_{Comparing davidhewitt:version-checks (6a27c08) with main (94b7d7e)}

Summary

⚡ 1 improvements
❌ 2 regressions
✅ 76 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`davidhewitt:version-checks`	Change
❌	`mapping_from_dict`	272.2 ns	327.8 ns	-16.95%
⚡	`not_a_list_via_downcast`	244.4 ns	216.7 ns	+12.82%
❌	`sequence_from_list`	272.2 ns	327.8 ns	-16.95%

davidhewitt · 2024-02-13T00:10:44Z

Ok great, I pushed some additional commits to restore the dependency of pyo3-macros-backend on pyo3-build-config and get everything lined up 👍

pyo3-macros-backend/src/utils.rs

noxfile.py

davidhewitt · 2024-02-13T21:52:41Z

Thanks for the review! I will prepare 0.20.3 shortly, possibly looking into #3722 (comment) first...

* add maximum Python version check * restore dependency of `pyo3-macros-backend` on `pyo3-build-config` * fix clippy-all noxfile job

Seems like the latest pyo3 version (0.20.3 at the date of writing) introduces a maximum version check which can make the our CI to fail. This can be turned off by setting the `abi3` feature, so that's what I did. More info: PyO3/pyo3#3821 btw, why are you reading this commit message? I almost never write commit messages this long, so it an interesting coincidence that you ended up here. Please reach me and tell me how you ended up diggging this, so we can have a laugh together

Summary: NOTE: If your `hg bisect` brings you here & the error you are seeing looks like ` expected Result<&PyAny, PyErr>, found Result<Bound<'_, PyAny>, PyErr>` then see these [migration notes](https://pyo3.rs/v0.21.0/migration.html#from-020-to-021) for the fix or click on `fbcode/security/ace/pyo3/authz.rs` or similar files from bellow to see what the fix is! In order to upgrade `pyo3` to [`0.21.1`](https://github.com/PyO3/pyo3/releases/tag/v0.21.1), the following had to take place: ## [PyO3] * Address [migration notes](https://pyo3.rs/v0.21.0/migration.html#from-020-to-021) for `Bound<'py, T>` * Address [#3595](PyO3/pyo3#3595) - this is done in a crude way for now since there are many call sites depending on `fbcode/dba/rust/common/service_address/py/pyo3_conversion_helper.rs` which would require a more thorough review. * Address [#3821](PyO3/pyo3#3821) - `pyo3-build-config` is now dependent on and used by PyO3 macros. Currently, the only thing that gets checked is `abi3` compatibility. To address that, we introduce a fixup for that automatically generate `pyo3-build-config*.txt` configuration files, given an `fbsource` Python version. We are tryin to stay as close to `pyo3-build-config*.txt` spec as possible even though only a single bool from that file is ever since there is validation for the other fields but also to future proof future changes. By also generating this file ourselves, we prevent PyO3 from attempting to locate a Python interpreter some other way which seems to start leaking into the PyO3 API as an implementation choice already. ## [PyPi + Rust] * Upgrade `orjson` to [`3.10.1`](https://github.com/ijl/orjson/releases/tag/3.10.1) * Added `README.md` notes for future upgrades * Upgrade `py-polars` to [`0.20.22`](https://github.com/pola-rs/polars/releases/tag/py-0.20.22) * Removed `py-polars` and `polars` from `target_os = "windows"`. The `third-party/pypi/polars` Python extension has only been supported for Mac and Linux for a while now so its only natural to do that on the Rust side as well. What is more, `polars-util` is bringing in `stacker = = "0.1.14"` which does not build on Windows mostly because its using a much more recent version of `libc` than we use in `third-party/rust` (see P1228807344) * Upgrade `pydantic-core` to [`2.18.2`](https://github.com/pydantic/pydantic-core/releases/tag/v2.18.2) * Removed old `third-party/pypi/pydantic-core` versions * Upgrade `safetensors` to [`0.4.3`](https://github.com/huggingface/safetensors/releases/tag/v0.4.3) * Patch `third-party/pypi/cryptography/41.0.7` to account for PyO3's [#2975](PyO3/pyo3#2975) (`0.19.0`) where `pyo3::once_cell` was renamed to `pyo3::sync` (see D56826865) * Upgrade `tokenizers` and `tokenizers-python` to [`0.19.1`](https://github.com/huggingface/tokenizers/releases/tag/v0.19.1) * Removed old `third-party/pypi/tokenizers` versions * Fixed `third-party/pypi/tokenizers/BUCK` * Migrated `third-party/pypi/tokenizers/0.19.1/BUCK` to mirror other Python packages that bind to Rust crates e.g. `libcst`, `polars` etc. * Removed Windows support from `third-party/pypi/tokenizers` ## [Rust] * Upgrade `indexmap` to [`2.2.6`](https://github.com/indexmap-rs/indexmap/releases/tag/2.2.6) * Both latest `pydantic-core` and `c2pa` depend on `serde_json > 1.0.112` which brings in `indexmap = 2.2.1`. The latter has deprecated `.take` and `.remove` on both `IndexMap` and `IndexSet` leading to a bunch of errors (see bellow), all addressed: ```bash error: use of deprecated method `indexmap::set::IndexSet::<T, S>::take`: `take` disrupts the set order -- use `swap_take` or `shift_take` for explicit behavior. --> fbcode/hphp/hack/src/package/types.rs:76:16 | 76 | self.0.take(value) | ^^^^ | = note: `-D deprecated` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(deprecated)]` ``` Reviewed By: capickett Differential Revision: D56671179 fbshipit-source-id: 3ae69c069b7f005570c1a06d37194cf056282a18

add maximum Python version check

fe28d3f

davidhewitt commented Feb 11, 2024

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

adamreichold reviewed Feb 11, 2024

View reviewed changes

pyo3-build-config/src/impl_.rs Show resolved Hide resolved

restore dependency of pyo3-macros-backend on pyo3-build-config

93affec

davidhewitt added the CI-build-full label Feb 12, 2024

fix clippy-all noxfile job

fb0260c

davidhewitt force-pushed the version-checks branch from 6a27c08 to fb0260c Compare February 12, 2024 23:23

adamreichold reviewed Feb 13, 2024

View reviewed changes

pyo3-macros-backend/src/utils.rs Show resolved Hide resolved

adamreichold reviewed Feb 13, 2024

View reviewed changes

noxfile.py Show resolved Hide resolved

adamreichold reviewed Feb 13, 2024

View reviewed changes

noxfile.py Show resolved Hide resolved

adamreichold approved these changes Feb 13, 2024

View reviewed changes

davidhewitt added this pull request to the merge queue Feb 13, 2024

Merged via the queue into PyO3:main with commit f5eafe2 Feb 13, 2024
65 of 68 checks passed

davidhewitt deleted the version-checks branch February 13, 2024 22:35

davidhewitt added a commit that referenced this pull request Feb 22, 2024

add maximum Python version check (#3821)

c375c8a

* add maximum Python version check * restore dependency of `pyo3-macros-backend` on `pyo3-build-config` * fix clippy-all noxfile job

davidhewitt added a commit that referenced this pull request Feb 23, 2024

add maximum Python version check (#3821)

5157a45

* add maximum Python version check * restore dependency of `pyo3-macros-backend` on `pyo3-build-config` * fix clippy-all noxfile job

messense mentioned this pull request Feb 26, 2024

error: the configured Python interpreter version (3.13) is newer than PyO3's maximum supported PyO3/maturin#1960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add maximum Python version check #3821

add maximum Python version check #3821

davidhewitt commented Feb 11, 2024

davidhewitt Feb 11, 2024

adamreichold Feb 11, 2024

davidhewitt Feb 11, 2024

davidhewitt Feb 11, 2024

adamreichold Feb 11, 2024

davidhewitt Feb 11, 2024

adamreichold Feb 12, 2024

davidhewitt commented Feb 11, 2024

adamreichold commented Feb 11, 2024

codspeed-hq bot commented Feb 12, 2024 •

edited

davidhewitt commented Feb 13, 2024

davidhewitt commented Feb 13, 2024

add maximum Python version check #3821

add maximum Python version check #3821

Conversation

davidhewitt commented Feb 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt commented Feb 11, 2024

adamreichold commented Feb 11, 2024

codspeed-hq bot commented Feb 12, 2024 • edited

Merging #3821 will degrade performances by 16.95%

Summary

Benchmarks breakdown

davidhewitt commented Feb 13, 2024

davidhewitt commented Feb 13, 2024

codspeed-hq bot commented Feb 12, 2024 •

edited