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

GitHub runner updates #388

Merged
merged 4 commits into from
Jan 13, 2024
Merged

GitHub runner updates #388

merged 4 commits into from
Jan 13, 2024

Conversation

sserita
Copy link
Contributor

@sserita sserita commented Jan 11, 2024

This includes several quality-of-life and deprecation runner updates and addresses #387.

  • MacOS runners are oversubscribed and 10x more expensive, and rarely break due to OS specific issues not caught by Linux tests. Therefore, MacOS tests are now only run on beta and master.
  • Bugfix/feature branches now only test the lowest and highest supported Python versions - this should be sufficient to flag any deprecation or missing language feature problems and cuts down the runtime on these branches by 2x. (Combined with no MacOS tests, this takes the number of runners needed from 12 to 4).
  • The runner versions were updated to -latest, bumping our testing environment to Ubuntu 22.04 from 20.04, Windows 2022 from 2019, and MacOS 12 from 11, and will now track updated environments.
  • The GitHub-provided actions were updated from v2 to v4 (or v3 in the case of cache) for node16 updates, which should remove the node12 deprecation warning we've been getting.

As an example, this cuts the current runtime from nearly 2 hours (with the MacOS oversubscription problem) to 13 minutes on this feature branch.

Only run MacOS tests on beta/master,
and only run lowest/highest Python versions on bugfix/feature branches.
Should address #387.
Should keep the README badges working still,
and have better continuity in the Actions tab on GitHub.
@sserita sserita added this to the 0.9.12.1 milestone Jan 11, 2024
@sserita sserita self-assigned this Jan 11, 2024
@sserita sserita linked an issue Jan 11, 2024 that may be closed by this pull request
@sserita sserita marked this pull request as ready for review January 11, 2024 22:47
@sserita sserita requested review from a team as code owners January 11, 2024 22:47
@sserita sserita requested a review from coreyostrove January 11, 2024 22:47
@coreyostrove
Copy link
Contributor

Question for you before I approve this. If I understand everything here correctly the new workflow structure is such that:

  1. main-minimal gets run upon pushes to any branch.
  2. main, which includes python 3.9 and 3.10 on linux and windows, gets run only upon pushes to develop and master (and external PRs).
  3. main-mac which includes python 3.8 through 3.11 on macOS only gets run upon pushes to beta and master (and external PRs).

Is splitting off main and main-mac necessary? In principle beta should track develop in which case there is little difference and these workflows could be merged. In practice this is only true when test_extras passes, though, so I wasn't sure if this was the reason behind that difference.

@sserita
Copy link
Contributor Author

sserita commented Jan 12, 2024

Thanks for taking a look Corey - you understand the proposed workflow correctly.

The reason I split main and main-mac was to avoid the Mac runners stuffing up tests on develop, which I thought was the original intent of #387. Although rereading now, I think you may have just wanted main-minimal and main. The Mac tests rarely catch bugs that the linux tests do not, but we do have cases where develop fails and does not push to beta - in these cases, I thought having the more streamlined no Mac tests would be worthwhile. We do want to test the Mac environment eventually, thus we have main-mac which runs on beta and master just so we have the full complement of tests on those branches.

I don't feel too strongly about this. If you think it is cleaner to merge the workflows and don't mind waiting for the Mac tests when we push to develop, I will change that and remerge those.

Copy link
Contributor

@coreyostrove coreyostrove left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. I'm sold, and this all looks good to me. Merge away!

@sserita sserita merged commit 8eea242 into develop Jan 13, 2024
4 checks passed
@sserita sserita deleted the feature-runner-updates branch January 30, 2024 22:44
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.

Idea: Switch macOS tests to run against beta branch
2 participants