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

TST: migrate macOS CI to arm64, move x86 to cron #16321

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

Conversation

neutrinoceros
Copy link
Contributor

@neutrinoceros neutrinoceros commented Apr 22, 2024

Description

In the wake of #16319 and #16320, it seems we can no longer assume that both macOS' taget archs (x86, arm64) behave similarly enough that we can just test one.

arm64 is the only arch targeted by macOS these days, but it still makes sense to test x86 which wan discountinued in Jan 2023 according to Wikipedia, so I'm adding an env rather than changing the one we have.

Also note that jobs relying on the moving tag macos-latest are being migrated to amr64, so I'm setting macos-12 and macos-14 explicitly (which is the documented way to request x86 and amr64 runners, respectively).

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

Copy link

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Copy link

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@neutrinoceros
Copy link
Contributor Author

neutrinoceros commented Apr 22, 2024

We're getting two failures, which are exactly #16319 and #16320 so this job already proved useful. I'm going to keep this as a draft until we know how to stabilize both.

@pllim pllim added this to the v7.0.0 milestone Apr 22, 2024
@pllim pllim requested a review from a team April 22, 2024 16:12
@pllim
Copy link
Member

pllim commented Apr 22, 2024

We were not even able to test OSX ARM64 wheels until recently, but now that we can, I guess does not hurt. But I would want @astrofrog and/or @saimn's inputs on whether a new CI job is really necessary here. As it stands, I think we would have caught this in the nightly wheel build (the scheduled ones, e.g., https://github.com/astropy/astropy/actions/runs/8778293486) but given transient nature, maybe that is not often enough, but then again, is scipy BLAS issues really our problem?


- name: Python 3.11 with all optional dependencies (MacOS X, x86)
macos: py311-test-alldeps
posargs: --durations=50 --run-slow
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't need --durations=50 --run-slow on both ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll update it when we know wether your idea of moving x86 to cron gets traction. Thanks !

@saimn
Copy link
Contributor

saimn commented Apr 22, 2024

Moving to the arm64 job is a good thing I think since that's what osx users are more and more using, and it's a different architecture. Not sure if we need to keep the x86 one here though, maybe it could be moved to cron jobs ?

@neutrinoceros
Copy link
Contributor Author

is scipy BLAS issues really our problem ?

Yes and no:

  • I'm not yet sure were the problem is originated (BLAS is only my primary suspect)
  • this particular instance is probably symptomatic of an edge case that is exercised in astropy's test suite and not in scipy's. Ultimately the (reduced) test should be upstream, but that's one of the reasons we need to test how our dependencies integrate: we can't expect them to already cover 100% of our own use case in their test suite (maybe in an ideal world though).
  • not having this (non exotic) arch part of CI means that we're effectively relying on devs and users to discovers these issues, and I would really like CI to catch such defects before users (or I) do.

Not sure if we need to keep the x86 one here though, maybe it could be moved to cron jobs ?

sounds reasonable to me.

@pllim
Copy link
Member

pllim commented Apr 22, 2024

Swapping out x86 with arm64 without changing the test name would LGTM. If we change the name, then I have to go update the branch protection rule and all PRs have to rebase (again). Thanks!

@pllim pllim changed the title TST: add macOS-amr64 to test matrix TST: add macOS-arm64 to test matrix Apr 22, 2024
@neutrinoceros
Copy link
Contributor Author

good point, I didn't think of that. Thanks !

@neutrinoceros
Copy link
Contributor Author

Looks like we just went through the migration process, so macos-latest is now arm64 by default. Since we still have a couple failures, I'll open another PR to move back to x86 to stabilize main, and I'll rebase this one.

@pllim
Copy link
Member

pllim commented Apr 23, 2024

I merged #16330 which would have to be reverted here along with fixes for if still necessary:

I think we should wait for scipy to sort it out first. Also see #16329 for an unmerged attempt.

@neutrinoceros
Copy link
Contributor Author

For now I'm working on producing a useful reproducer for scipy folks (the full-fledge astropy test involves several thousands lines of code so it's basically impossible to debug). I'm getting there !

@neutrinoceros
Copy link
Contributor Author

I rebased to fix conflicts and resolve existing suggestions. CI is still expected to fail.

@neutrinoceros neutrinoceros changed the title TST: add macOS-arm64 to test matrix TST: migrate macOS CI to arm64, move x86 to cron Apr 24, 2024
@neutrinoceros
Copy link
Contributor Author

Unless the fix in scipy is backported, this will wait for scipy 1.14.0. See https://discuss.scientific-python.org/t/proposed-release-schedule-for-scipy-1-14-0/ for the proposed release schedule

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.

None yet

3 participants