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

Use cibuildwheel for all architectures; add support for aarch64 linux; publish musllinux wheels as well as manylinux; move cibuildwheel configuration to pyproject.toml; use pypa action for PyPI upload #118

Merged
merged 88 commits into from
Nov 25, 2024

Conversation

enzbus
Copy link
Contributor

@enzbus enzbus commented Oct 27, 2024

Hello @bodono , testing here to build wheels using cibuildwheel . Not sure how bad it is to reproduce the pipeline for Windows for example, but if it works it should be much easier to maintain.

@enzbus enzbus closed this Oct 27, 2024
@enzbus
Copy link
Contributor Author

enzbus commented Oct 27, 2024

Closing this because I just realized the CI/CD is already running on my fork, will reopen once ready.

@enzbus enzbus changed the title Use cibuildwheel for all architectures; add support for aarch64 linux; move cibuildwheel configuration to pyproject.toml; use pypa action for PyPI upload Use cibuildwheel for all architectures; add support for aarch64 linux; publish musllinux wheels as well as manylinux; move cibuildwheel configuration to pyproject.toml; use pypa action for PyPI upload Oct 27, 2024
@enzbus
Copy link
Contributor Author

enzbus commented Oct 28, 2024

When you have some time @bodono please review. You'll have to set up tokens for trusted publishing in PyPI. This page explains it -> https://docs.pypi.org/trusted-publishers/adding-a-publisher/ , scs-python is registered under your username in PyPI so it's all up to you.

You can see from the runner logs that the Linux job builds manylinux wheels for x86_64 and aarch64, and musllinux images for x86_64. That's why it takes longer than the others. Especially the aarch64 wheels take longer to compile, I suspect it's a combination of running QEMU virtualization inside docker, and openblas lacking optimized kernels for aarch64. Everything compiles and passes pytest, anyways.

I can also add steps to make a Github release on tag, like CLARABEL does -> https://github.com/oxfordcontrol/Clarabel.rs/releases (but including all the wheels too). With this code wheels are built automatically for all current Python versions. One possible extension, but I'm afraid that will require changing the meson code, would be to use ABI3 reduced Python linking, which creates wheels that are forward compatible with future Python versions. That's the strategy used by MOSEK for example -> https://pypi.org/project/Mosek/#files

@enzbus
Copy link
Contributor Author

enzbus commented Oct 28, 2024

PS you can inspect the wheels that are built, in the logs for example for the Linux wheels job there is the link to the artifact under the upload step -> https://github.com/bodono/scs-python/actions/runs/11544095456/artifacts/2109819766. I just tried to install a wheel manually in a test environment and confirm that it works and is linked correctly.

@bodono
Copy link
Owner

bodono commented Nov 1, 2024

Thanks for doing all this @enzbus ! I will take a look asap.

@bodono
Copy link
Owner

bodono commented Nov 15, 2024

Thanks @enzbus, this looks great, and much cleaner! Let's get this landed asap!

A couple of questions I have before that(as I'm not very familiar with all this):

  1. Currently wheels are uploaded to pypi when I create a new tag (see here. It seems like this will now be triggered on releases instead? I'm fine with the change just want to make sure I understand the new workflow.
  2. It currently uses the 'secrets' to manage Pypi uploads, and it looks like I need to switch over to what you linked in order to do that instead, right? I can delete the secrets afterwards presumably.
  3. There is another PR close to landing here that supports python 3.13, it looks like the merge for them shouldn't be too bad. I am happy to take it over and do the merge after that one lands if you don't have time, just let me know.

@enzbus
Copy link
Contributor Author

enzbus commented Nov 15, 2024

Thanks @enzbus, this looks great, and much cleaner! Let's get this landed asap!

A couple of questions I have before that(as I'm not very familiar with all this):

1. Currently wheels are uploaded to pypi when I create a new tag (see [here](https://github.com/bodono/scs-python/blob/master/.github/workflows/build.yml#L225). It seems like this will now be triggered on releases instead? I'm fine with the change just want to make sure I understand the new workflow.

Yes, I'm not too familiar with the Github release system either; please do edit the trigger part of the upload_to_pypi section as you wish. What I've done in another project is upload always with skip-existing = true, see here, it's won't re-write wheels with the same version name.

2. It currently uses the 'secrets' to manage Pypi uploads, and it looks like I need to switch over to what you linked in order to do that instead, right? I can delete the secrets afterwards presumably.

That is actually super easy. I recently tested it with a repository I started 2 weeks ago, pyspqr. Head over to pypi.org, and in the scs management section select your github.com/bodono/scs-python as trusted publisher. You need to tell it the github workflow script path and environment name, it's pypi in the code. Then it will all work without setting any environment variable, tokens, or anything like that.

3. There is another PR close to landing [here](https://github.com/bodono/scs-python/pull/117) that supports python 3.13, it looks like the merge for them shouldn't be too bad. I am happy to take it over and do the merge after that one lands if you don't have time, just let me know.

I saw that. This is a bit heavier, merge whichever you prefer first. This one solves the issue some time ago a user raised about missing aarch64 linux wheels.

Bonus, ABI3 linking (forward-compatible with future CPython versions). I got that working in pyspqr, it's just one macro definition in the C code and some extra checks in cibuildwheel. I followed this repository's instructions, I hope it's not too much worse with meson...

@enzbus
Copy link
Contributor Author

enzbus commented Nov 15, 2024

PS Yes, better if you merge the other first. I'll clean up this one and add ABI3. Please do squash commits when you merge this :)

@enzbus
Copy link
Contributor Author

enzbus commented Nov 16, 2024

Hey Brendan; turns out ABI3 is a dead-end. You're using PyTypeObject, it's not fully part of the limited API

Also, since PyTypeObject is only part of the Limited API as an opaque struct, any extension modules using static types must be compiled for a specific Python minor version.

@bodono
Copy link
Owner

bodono commented Nov 21, 2024

Thanks Enzo, I did the rebase. Can you double check it looks good and if so we can merge this!

@enzbus
Copy link
Contributor Author

enzbus commented Nov 23, 2024

Looks good to merge! Then dependabot will keep cibuildwheel updated and it will add new Python versions as they become available. You'll still have to make new tags to deploy :)

@bodono
Copy link
Owner

bodono commented Nov 25, 2024

Great! Thanks again for doing this @enzbus !

@bodono bodono merged commit 55647d9 into bodono:master Nov 25, 2024
24 checks passed
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.

2 participants