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

bump jiter to 0.8.2, PyO3 to 0.23.3 #1570

Merged
merged 1 commit into from
Dec 4, 2024
Merged

bump jiter to 0.8.2, PyO3 to 0.23.3 #1570

merged 1 commit into from
Dec 4, 2024

Conversation

davidhewitt
Copy link
Contributor

Change Summary

... I think this might fix the aarch64 build failure on main...

Related issue number

N/A

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@davidhewitt davidhewitt added the Full Build cause CI to do a full build label Dec 2, 2024
Copy link

codspeed-hq bot commented Dec 2, 2024

CodSpeed Performance Report

Merging #1570 will not alter performance

Comparing dh/jiter-0.8.1 (5fd6ead) with main (be87e05)

Summary

✅ 155 untouched benchmarks

@davidhewitt
Copy link
Contributor Author

@messense the windows aarch64 failures here seem unfortunate, I think probably maturin has located the x64 (host) interpreters and passed those to PyO3. Then neither project has noticed that we're building for aarch64 and the libs from the host interpreters can't be used to link.

We do have the generate-import-lib enabled, so I think maybe PyO3 is doing the wrong thing here? Or do you think maturin should be doing something different? I will try to push a git branch of PyO3 later, perhaps.

@messense
Copy link
Contributor

messense commented Dec 3, 2024

🐍 Found CPython 3.11 at C:\hostedtoolcache\windows\Python\3.11.9\x64\python.exe, CPython 3.12 at C:\hostedtoolcache\windows\Python\3.12.7\x64\python.exe, CPython 3.13 at C:\hostedtoolcache\windows\Python\3.13.0\x64\python.exe

Yeah this looks wrong, previously it was

🐍 Found CPython 3.11, CPython 3.12, CPython 3.13

@davidhewitt
Copy link
Contributor Author

Heh, looks like that was probably my maturin patch to "fix" the generate-import-lib stuff for abi3 not being quite right then, sorry 🙈

@davidhewitt
Copy link
Contributor Author

(I can try to fix that later 👍)

@messense
Copy link
Contributor

messense commented Dec 3, 2024

I think it may be related to https://github.com/PyO3/maturin/pull/2353/files#diff-b465b575a327d11d450c4994e874d0fdd40b90890e43089530c3d148d0017a33R9, is_cross_compiling changed to false because the native MSVC compiler can target aarch64, but we also need to account for python architectures.

@messense
Copy link
Contributor

messense commented Dec 3, 2024

I think it may be related to https://github.com/PyO3/maturin/pull/2353/files#diff-b465b575a327d11d450c4994e874d0fdd40b90890e43089530c3d148d0017a33R9, is_cross_compiling changed to false because the native MSVC compiler can target aarch64, but we also need to account for python architectures.

PyO3/maturin#2359 should fix this.

@davidhewitt
Copy link
Contributor Author

Thanks, I guess we can merge that and try a beta release here?

@messense
Copy link
Contributor

messense commented Dec 3, 2024

A beta release sounds good.

@messense
Copy link
Contributor

messense commented Dec 3, 2024

v1.7.8-beta.1 is out.

@messense
Copy link
Contributor

messense commented Dec 3, 2024

I think you need to use maturin-version: 1.7.8-beta.1 in the action input, it does not support translating from 1.7.8b1 to 1.7.8-beta.1 yet (it downloads maturin executable from github release)

@davidhewitt
Copy link
Contributor Author

Thanks, looks like that beta release works 🎉

@davidhewitt
Copy link
Contributor Author

davidhewitt commented Dec 3, 2024

The aarch64 failure is a definite issue which looks like it's caused by a new interaction between PyO3 0.23 and maturin. Somewhere in the system it seems to not be recompiling when changing Python version.

See output with PyO3 0.22.6 when finishing 3.8 and then compiling for 3.9:

📦 Built wheel for CPython 3.8 to dist/pydantic_core-2.27.1-cp38-cp38-musllinux_1_1_x86_64.whl
warning: `/root/.cargo/config` is deprecated in favor of `config.toml`
note: if you need to support cargo 1.38 or earlier, you can symlink `config` to `config.toml`
   Compiling pyo3-build-config v0.22.6
   Compiling pyo3-macros-backend v0.22.6
   Compiling pyo3-ffi v0.22.6
   Compiling pyo3 v0.22.6
   Compiling jiter v0.7.1
   Compiling pydantic-core v2.27.1 (/home/runner/work/pydantic-core/pydantic-core)
   Compiling pyo3-macros v0.22.6
    Finished `release` profile [optimized] target(s) in 54.07s
🖨  Copied external shared libraries to package pydantic_core.libs directory:
    /usr/local/musl/x86_64-unknown-linux-musl/lib/libgcc_s.so.1
📦 Built wheel for CPython 3.9 to dist/pydantic_core-2.27.1-cp39-cp39-musllinux_1_1_x86_64.whl

Same output with PyO3 0.23:

📦 Built wheel for CPython 3.8 to dist/pydantic_core-2.27.1-cp38-cp38-musllinux_1_1_x86_64.whl
warning: `/root/.cargo/config` is deprecated in favor of `config.toml`
note: if you need to support cargo 1.38 or earlier, you can symlink `config` to `config.toml`
    Finished `release` profile [optimized] target(s) in 0.06s
🖨  Copied external shared libraries to package pydantic_core.libs directory:
    /usr/local/musl/x86_64-unknown-linux-musl/lib/libgcc_s.so.1
📦 Built wheel for CPython 3.9 to dist/pydantic_core-2.27.1-cp39-cp39-musllinux_1_1_x86_64.whl

... I'll try to debug, this looks pretty serious and probably also explains samuelcolvin/watchfiles#313 - I think we're compiling wheels against the wrong Python API version.

@davidhewitt
Copy link
Contributor Author

Yep, pretty clear what's going on. In PyO3/pyo3#4497 I mistakenly changed PYO3_CONFIG_FILE env var to be read in a way that we don't emit a cargo:rerun-if-changed block for it. And so all builds shipped using 0.23 in combination with the PYO3_CONFIG_FILE mechanism (like maturin uses) are potentially broken.

I think this is a pretty major soundness bug, will have to get a PyO3 fix out asap.

@davidhewitt davidhewitt changed the title bump jiter to 0.8.1 bump jiter to 0.8.2, PyO3 to 0.23.3 Dec 4, 2024
@davidhewitt
Copy link
Contributor Author

@messense with the unfortunate PyO3 compile bug in PyO3/pyo3#4757 I think there's going to be a lot of projects recompiling in the coming days. Could I possibly ask you to publish maturin 1.7.8 please so that they won't hit the same windows aarch64 compile issue?

I think we're then in a good place with PyO3 / maturin hopefully fully compatible with the new free-threaded builds for various configurations, thank you for all your help 🙏

@messense
Copy link
Contributor

messense commented Dec 4, 2024

I can do a new release after work, alternatively please feel free to cut a a new release by yourself, it's basicially updating version and push a new tag, then trigger a version manifest update in maturin-action after it's released to github releases.

@davidhewitt
Copy link
Contributor Author

Happy to do it and save you the time, I'll do that now. Thanks.

@davidhewitt
Copy link
Contributor Author

All green! 🎉

@davidhewitt davidhewitt merged commit 707b440 into main Dec 4, 2024
65 checks passed
@davidhewitt davidhewitt deleted the dh/jiter-0.8.1 branch December 4, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Full Build cause CI to do a full build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants