Skip to content

CI Introduces macOS arm64 wheel building with Cirrus CI #25048

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

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Nov 25, 2022

This PR introduces macOS arm64 wheel building with Cirrus CI. Here is an example of this job running: https://cirrus-ci.com/task/6614267152039936, which takes ~ 4 minutes to run.

I made adjustments to build_tools/circle/build_test_arm.sh so it can be configured to run on macOS and Linux arm64. In the future, we can move some of the Linux ARM tests and wheel building to Cirrus CI as well.

There are limits to Cirrus CI as detailed here: https://cirrus-ci.org/faq/#are-there-any-limits. macOS can only have 1 VM at a time which means if there are many PRs there can be a long queue. The complete run does only take ~ 4 minutes so it may not be too bad.

The situation is nicer for Linux arm64, because we can spin up to 16 CPUs in total. For example, we can have 4 jobs with 4 CPUS each on Linux arm64.

@glemaitre
Copy link
Member

macOS can only have 1 VM

Since we only use it for nightly build and [ci build], it might not be a problem.

@thomasjpfan
Copy link
Member Author

Since we only use it for nightly build and [ci build], it might not be a problem.

Currently this PR adds macOS arm64 to our test suite, which means all PRs will run the test.

If we only want to use Cirrus CI for building macOS arm64 wheels, then we can make it nightly and trigger with [cd build]. I'm okay with this alternative.

@glemaitre
Copy link
Member

If we only want to use Cirrus CI for building macOS arm64 wheels, then we can make it nightly and trigger with [cd build]. I'm okay with this alternative.

My reasoning here is that we did not get into trouble by not checking every PR on ARM64. I am thinking that limiting to the nightly build could allow us to save some computation.

@thomasjpfan thomasjpfan marked this pull request as draft November 29, 2022 12:48
@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2022

I agree with @glemaitre's suggestion to focus its use wheel building and testing (for both nightly and release builds).

Thanks for the PR @thomasjpfan!

@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2022

+1 to also try to build the linux arm wheels on this platform so as to not rely on travis anymore.

@thomasjpfan thomasjpfan changed the title CI Introduces macOS arm64 testing with Cirrus CI CI Introduces macOS arm64 wheel building with Cirrus CI Dec 6, 2022
@thomasjpfan thomasjpfan marked this pull request as ready for review December 6, 2022 20:16
@thomasjpfan
Copy link
Member Author

I updated this PR to build macOS wheels arm64 using cibuildwheel and enabled Cirrus CI in the repo. The Cirrus CI results can be seen in the list of checks and is available here.

Follow up PRs include:

  • Uploading artifacts to the nightly or staging index
  • Opening an tracking issue if the CirrusCI job fails.
  • Add configuration for Linux ARM64

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM.

Thank you for bringing this new support for macOS arm64, @thomasjpfan.

@jjerphan
Copy link
Member

@ogrisel: can we merge this?

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I did another pass of review and it looks good to me. It's great that test runs are so fast.

+1 for follow-up PRs to build release / nightly artifacts with this and to also build linux wheels for the arm64 platform on this infra.

@ogrisel ogrisel merged commit 743fe8e into scikit-learn:main Dec 12, 2022
@lesteve
Copy link
Member

lesteve commented Dec 14, 2022

Should we go back to building the doc on CircleCI i.e. revert #21137? I am happy to do the PR.

It seems like #21137 was part of making room on CircleCI #20711 to move towards building the arm64 wheels on CircleCI but this does not make sense anymore since we are using Cirrus.

#21137 introduced additional complexity and a ~5-10 minutes latency (store artifact on github, get the github artifact on CircleCI) when we are waiting to see the result on the doc build.

Vincent-Maladiere pushed a commit to Vincent-Maladiere/scikit-learn that referenced this pull request Dec 14, 2022
@thomasjpfan
Copy link
Member Author

I agree the latency is annoying. I propose:

  1. doc-min-dependencies on GitHub Actions, but do not upload to CircleCI. I do not think we look at this rendering often. The job is usually for catching incompatible changes with our minimum dependencies when updating the examples.
  2. The regular doc goes back to CircleCI. CircleCI focuses on building the version of the docs that is actually shown to users.

I admit this is more complex compared to having everything on CircleCI, but our doc building scripts already work with both, so we will not be adding more complexity.

@lesteve What do you think?

@lesteve
Copy link
Member

lesteve commented Dec 14, 2022

I guess that could get rid of the 5-10 minutes latency, but what is the downside of moving both doc builds back to CircleCI?

Your proposal does not add complexity to what we currently have but still is quite more complex compared to having both builds on CircleCI.

@thomasjpfan
Copy link
Member Author

thomasjpfan commented Dec 14, 2022

I guess that could get rid of the 5-10 minutes latency, but what is the downside of moving both doc builds back to CircleCI?

Ultimately, I want the setup with the fastest turn around time. Even with the latency, using GitHub Actions + CircleCI was faster to render back when #21137 was developed. Since then, we have gotten access to 60 workers on GitHub Actions.

The last time I checked, CircleCI can only run 2 workers concurrently, but when I look at the docs now we can run 30 workers concurrently. If we can run 30 workers concurrently and have enough compute on CircleCI, then I do prefer moving both doc builds back to CircleCI.

@lesteve
Copy link
Member

lesteve commented Dec 14, 2022

I also see 30 workers if I look at https://app.circleci.com/settings/plan/github/scikit-learn/overview.

About compute time on CircleCI, this is not super clear, the page above mentions 30,000 free credits per month, but also 400,000 credits:
image

You can also check our historical consumption in https://app.circleci.com/settings/plan/github/scikit-learn/usage and in March 2022, we consumed 600,000 credits so I am not sure CircleCI is actually enforcing these limits (or maybe the rules were different at the time?) ...

jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 3, 2023
@thomasjpfan
Copy link
Member Author

@lesteve Yea, I can see that sometimes we sometimes went over the 400k, but CircleCI did not stop working. We always go over their storage limit and that also takes some credits. For example this is for November 2022:

nov-2022

I think it comes down to how much we want to rely on CircleCI being okay with us going over sometimes. Historically, they have been lenient.

jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 23, 2023
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.

5 participants