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

Set max parallel property for Bazel tests #2710

Merged
merged 1 commit into from
Feb 13, 2021

Conversation

BenHenning
Copy link
Member

We're starting to see significant resource starvation in GitHub Actions, likely due to #1904 leading to >130 tests running in separate actions. This PR sets a max parallelization of 5 which means:

  1. No more than 5 Bazel tests will run simultaneously. At ~15 minutes per test, this means it will take 7-8 hours for the whole suite to run. This actually seems fine to me. In reality, people should not be relying on CI for tests passing/failing, they should be relying on running the tests locally. Further, code reviewers shouldn't hinge their approval on actions passing since GitHub already guards this. In general, the asynchronous nature of code reviews should result in an ~8 hour CI turnaround not causing too much issue. This does mean that more trivial changes might take longer than expected to get in, but as we continue to modularize Bazel this situation will get better (since the number of affected targets will go down for each PR).
  2. Per https://docs.github.com/en/actions/reference/usage-limits-billing-and-administration a maximum of 20 jobs can run simultaneously across the whole project. This change will let us load-balance tests so that new PRs can get linter results & tests started without having to wait ~8 hours for all other workflows to complete.

Reference documentation: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstrategymax-parallel.

@BenHenning
Copy link
Member Author

Note that I am actually going to cancel all existing workflows to let this PR get merged in.

@BenHenning
Copy link
Member Author

Also: note that this PR will not actually demonstrate the parallelization since there are no new tests to run.

Copy link
Contributor

@jcqli jcqli left a comment

Choose a reason for hiding this comment

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

LGTM! Needs another run for the failed check

@BenHenning
Copy link
Member Author

Thanks! The failed check is optional (and a known issue), so this actually can be merged.

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.

2 participants