Skip to content

Conversation

@ReeceHumphreys
Copy link
Contributor

@ReeceHumphreys ReeceHumphreys commented Aug 25, 2025

  • Tickets addressed: bsk-1081
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

This PR adds a new workflow which uses cibuildwheel to build wheels for multiple python versions across a variety of operating systems.

Verification

I temporarily had the workflow execute on pushes to this brach for testing. Here is a link which shows the bundled wheels for each os:

https://github.com/AVSLab/basilisk/actions/runs/17392301611

NOTE: Some of these wheel distributions do not include all of our "supported" Python versions. For example, we technically support Python 3.8 on macOS but the ARM builds of wheels require at least python 3.9.

Documentation

N/A

Future work

Publish these wheels whenever we release a new version to PyPI.

@ReeceHumphreys ReeceHumphreys added enhancement New feature or request ci Continuous integration dont merge Waiting on other changes before merging labels Aug 25, 2025
@ReeceHumphreys ReeceHumphreys force-pushed the feature/bsk-xxxx-build-wheels branch 2 times, most recently from 4fa4f41 to 2915edb Compare August 29, 2025 18:07
@ReeceHumphreys ReeceHumphreys marked this pull request as ready for review August 29, 2025 18:56
@ReeceHumphreys ReeceHumphreys requested a review from a team as a code owner August 29, 2025 18:56
@ReeceHumphreys
Copy link
Contributor Author

Only commit 2915edb should be reviewed. The other two commits are from PR #1073. PR #1073 is required to be merged before this PR as we leverage the refactor to make this PR simpler.

@ReeceHumphreys ReeceHumphreys requested a review from schaubh August 29, 2025 19:11
@dpad
Copy link
Contributor

dpad commented Aug 31, 2025

Hi @ReeceHumphreys ,

Apologies for jumping in on this review, but thank you very much for your efforts on this!

I wanted to just recommend the following option, since in my previous experience it greatly reduced the size of the compiled Basilisk wheel file for Linux (down to ~200MB):

[tool.cibuildwheel.linux]
repair-wheel-command = [
  # Minimise the wheel size by stripping unnecessary symbols from the compiled objects.
  "auditwheel repair --strip -w {dest_dir} {wheel}"
]

Also, you're probably already aware, but you can run tests directly after building each wheel to ensure they install and run correctly. For example:

[tool.cibuildwheel]
# NOTE: You'll need to ensure pytest is installed, e.g. in an optional dependency group using `test-extras`
test-command = [
  "cd {package}/src && pytest -v . -m \"not scenarioTest\""
]

@schaubh
Copy link
Contributor

schaubh commented Aug 31, 2025

Howdy @dpad , good to hear from you. @ReeceHumphreys just joined my lab this fall and has excellent software development skills. He is helping me get these pip wheels onto PyPi and improve the CI test build scripts. We appreciate any feedback or suggestions you want to share.

@ReeceHumphreys
Copy link
Contributor Author

ReeceHumphreys commented Aug 31, 2025

Hi @ReeceHumphreys ,

Apologies for jumping in on this review, but thank you very much for your efforts on this!

I wanted to just recommend the following option, since in my previous experience it greatly reduced the size of the compiled Basilisk wheel file for Linux (down to ~200MB):

[tool.cibuildwheel.linux]
repair-wheel-command = [
  # Minimise the wheel size by stripping unnecessary symbols from the compiled objects.
  "auditwheel repair --strip -w {dest_dir} {wheel}"
]

Hello @dpad!

Thanks for the suggestion! I did notice that the Linux wheels were fairly large during testing, but I haven’t looked into it yet since I’ve mainly been testing on macOS and Windows. Once I move on to Linux testing, I’ll incorporate your suggestion and verify everything still works as expected.

Also, you're probably already aware, but you can run tests directly after building each wheel to ensure they install and run correctly. For example:

[tool.cibuildwheel]
# NOTE: You'll need to ensure pytest is installed, e.g. in an optional dependency group using `test-extras`
test-command = [
  "cd {package}/src && pytest -v . -m \"not scenarioTest\""
]

This is essentially how I have been testing the wheels myself locally! I am not sure yet if it needs to be a part of our CI/CD pipeline as we already test all of the code across all OSes as part of another workflow. Once the wheel builds are working fully as expected adding this to the CI would do is be verifying is that cibuildwheel does indeed produce correct wheels.

This feels like we would be verifying cibuildwheels functionality more than anything. Im not necessarily opposed to adding it as a check though.

@ReeceHumphreys ReeceHumphreys force-pushed the feature/bsk-xxxx-build-wheels branch 2 times, most recently from 72352ab to 9dd7f07 Compare September 2, 2025 03:23
@ReeceHumphreys
Copy link
Contributor Author

ReeceHumphreys commented Sep 2, 2025

Here is a test of the workflow that I triggered. Looks like all of the wheels are much more reasonable in size after an optimization pass. Linux is now around 75 MB per wheel:

https://github.com/AVSLab/basilisk/actions/runs/17571574748

@ReeceHumphreys ReeceHumphreys force-pushed the feature/bsk-xxxx-build-wheels branch 15 times, most recently from 33d0a3a to 2b5a741 Compare September 8, 2025 05:19
@ReeceHumphreys ReeceHumphreys force-pushed the feature/bsk-xxxx-build-wheels branch from 2b5a741 to e07c87d Compare September 8, 2025 05:20
@ReeceHumphreys ReeceHumphreys removed the dont merge Waiting on other changes before merging label Sep 8, 2025
@ReeceHumphreys ReeceHumphreys moved this to 👀 In review in Basilisk Sep 8, 2025
@ReeceHumphreys ReeceHumphreys force-pushed the feature/bsk-xxxx-build-wheels branch 6 times, most recently from 280516e to 3ea0483 Compare September 9, 2025 04:14
Copy link
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

Great changes. I like how you refactored the commit to make the review much easier. Good work :-)

System setup needs to occur both for building bsk when
peforming PR testing AND when building wheels. To avoid
code duplication this setup has been moved to its own
composite action.
Create an sdist and add a step to publish wheels in the workflow

    - Tags starting with v* (e.g., v2.3.1) publish to PyPI’s main
      index.
    - Tags starting with test* publish to PyPI’s test index.

The version bump script was also updated to export the new
version to GitHub runners, enabling automatic tagging on patch
releases. This ensures PyPI releases are triggered on merge.
The built windows wheels did not contain all of the necessary
dlls to run. This commit ensures the built wheels contains them.
PyPi requires that files are below 100 MB. If we include docs,
examples, and tests we will exceed this limit. As such, we
remove these files from the sdist artifact as they are not
necessary for performing a build of bsk.
Update test to source ONNX files relative to repo root instead
of the Basilisk executable. Using the executable path was
fragile, since BSK wheels do not ship tests or helpers.
@ReeceHumphreys ReeceHumphreys force-pushed the feature/bsk-xxxx-build-wheels branch from 3ea0483 to 15ea56a Compare September 11, 2025 03:26
@ReeceHumphreys ReeceHumphreys self-assigned this Sep 11, 2025
@ReeceHumphreys ReeceHumphreys merged commit bdfef0f into develop Sep 11, 2025
22 of 23 checks passed
@ReeceHumphreys ReeceHumphreys deleted the feature/bsk-xxxx-build-wheels branch September 11, 2025 06:05
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Basilisk Sep 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Continuous integration enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants