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

ci: remove workaround for free-threaded build w. maturin #172

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

davidhewitt
Copy link
Collaborator

Should hopefully just work on the new maturin 1.7.5 🤞

Copy link

codspeed-hq bot commented Nov 26, 2024

CodSpeed Performance Report

Merging #172 will improve performances by 12.1%

Comparing dh/simplify-ci (afea1e9) with main (2b67dd0)

Summary

⚡ 1 improvements
✅ 72 untouched benchmarks

Benchmarks breakdown

Benchmark main dh/simplify-ci Change
sentence_jiter_iter 7.9 µs 7 µs +12.1%

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.74%. Comparing base (2b67dd0) to head (afea1e9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #172   +/-   ##
=======================================
  Coverage   88.74%   88.74%           
=======================================
  Files          13       13           
  Lines        2203     2203           
  Branches     2203     2203           
=======================================
  Hits         1955     1955           
  Misses        153      153           
  Partials       95       95           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b67dd0...afea1e9. Read the comment docs.

@davidhewitt
Copy link
Collaborator Author

davidhewitt commented Nov 26, 2024

cc @messense, two issues here:

  • the windows builds are failing to "detect platform" (?) and making wheels named e.g. cp313-none when this should be cp313-cp313t (I think - from comparing pydantic-core wheels to numpy wheels this might have been a problem for a while). I'll try to debug.
  • maturin build -i 3.13t still fails, need to pass maturin build -i python3.13t - it might be nice if just 3.13t was accepted, I'll open a maturin PR for this to discuss

EDIT: I think "failing to detect platform" is a red herring.

@davidhewitt
Copy link
Collaborator Author

I opened PyO3/maturin#2324 and PyO3/maturin#2325 to resolve.

messense pushed a commit to PyO3/maturin that referenced this pull request Nov 28, 2024
xref pydantic/jiter#172 (comment)

At the moment the windows builds are hard coded to use the `none` abi
tag (unless they are abi3). I think this is probably technically
incorrect, though in practice has worked fine historically because there
was only one build on Windows... until 3.13t has come along with
free-threading as a second option :)

It's important that we do emit proper abi tags (e.g. `cp313`, `cp313t`)
because otherwise the two 3.13 wheels end up with the same name and
can't be uploaded to PyPI. I think I also saw some install failures on
`jiter` CI where a Python 3.13 build ended up installing the other
build's wheel (I think `pip` detected an incorrect lib name and rejected
it).

I suspect I might need to fix some tests...
@davidhewitt
Copy link
Collaborator Author

davidhewitt commented Nov 28, 2024

@messense looks like there is no way to run maturin-action with a git branch of maturin? I guess I can temporarily fork.

@davidhewitt davidhewitt marked this pull request as draft November 28, 2024 09:49
@messense
Copy link

looks like there is no way to run maturin-action with a git branch of maturin

Yes, but for Windows and macOS you can just pip install from a git branch of maturin to test as there are no Docker containers involved.

@davidhewitt
Copy link
Collaborator Author

Ok, it looks like maturin 1.7.6 builds the wheels totally fine, however the metadata-version bump to 2.4 broke twine. pypa/twine#1146

I can deal with that separately, at least for the non-abi3 wheels I ship it seems like maturin is now ready, and PyO3/maturin#2333 hopefully fixes the abi3 case 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants