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

Updated cibuildwheel settings to generate apple silicon wheels #1673

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

Conversation

reinecke
Copy link
Collaborator

@reinecke reinecke commented Oct 24, 2023

Fixes #1672

This is a WIP PR for enabling Apple Silicon Support.

Currently, this PR simply enables cibuildwheel for x86_64 and arm64.

TODO:

  • Decide which wheels we feel like we need (is there a demand for universal2 or are the optimized wheels fine?)
  • Figure out how to do testing on Apple Silicon since arm64 wheels can't be tested on x86_64

@codecov-commenter
Copy link

Codecov Report

Merging #1673 (dc3ff98) into main (745e614) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1673   +/-   ##
=======================================
  Coverage   79.84%   79.84%           
=======================================
  Files         197      197           
  Lines       21767    21767           
  Branches     4354     4354           
=======================================
  Hits        17379    17379           
  Misses       2234     2234           
  Partials     2154     2154           
Flag Coverage Δ
py-unittests 79.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 745e614...dc3ff98. Read the comment docs.

@JeanChristopheMorinPerso
Copy link
Member

Decide which wheels we feel like we need (is there a demand for universal2 or are the optimized wheels fine?)

Maybe look at https://cibuildwheel.readthedocs.io/en/stable/faq/#what-to-provide?

Figure out how to do testing on Apple Silicon since arm64 wheels can't be tested on x86_64

I hear through the branches that it's coming. See https://github.blog/2023-10-02-introducing-the-new-apple-silicon-powered-m1-macos-larger-runner-for-github-actions/#how-to-use-the-runner and https://github.com/AcademySoftwareFoundation/wg-ci/blob/main/meetings/2023-10-11.md?plain=1#L89. I'll check to see if we could have access.

@reinecke
Copy link
Collaborator Author

I spot checked the Apple Silicon python 3.11 wheel from the first build.

I unzipped that dir into /tmp/otiowheels and installed from it using the following command:

python -m pip install --find-links /tmp/otiowheels opentimelineio==0.16.0.dev1

I was then able to use make ci-postbuild to run tests against the installed wheel.

Our CI currently has a py_build_test job and a package_wheels job.
These redundantly build OTIO on all the platforms but py_build_test just does a pip install .[dev] -v whereas package_wheels uses cibuildwheel to make the final artifacts.

I think a good refactor might be to use have one job that just runs the lint and check-manifest in a single platform (like we do with package_sdist), then a dependent job that runs package_wheels to generate the artifacts, and finally make a dependent job that pulls down the artifacts, installs the right one for the platform, and runs tests (basically use these steps, put a step in to pull down the built wheels and install from them, and then run these steps).

This would have some specific benefits:

  • We would only do the OTIO libaray builds once
  • We would be testing the exact artifact that will deliver to PyPI and might be more likely to catch things like dynamic linker pathing issues that could come up
  • We can limit what is run on Apple Silicon to the bare minimum

The one downside I see is that cibuildwheel seems to take ~5m to build whereas pip install . is taking closer to ~2m in many instances. I suspect this is because cibuildwheel downloads and installs it’s own python. My personal feeling is that the benefits outweigh the extra few minutes.

@reinecke
Copy link
Collaborator Author

Another consideration - universal binaries are a bit slower to import possibly - what's the tax of this?

From TSC meeting: let's go with x86_64 + universal2 archs

@reinecke reinecke added this to the Public Beta 16 milestone Oct 26, 2023
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.

Add Apple Silicon/ARM wheels
3 participants