-
Notifications
You must be signed in to change notification settings - Fork 590
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
Added ability to run linux workflows on large runners #6273
base: master
Are you sure you want to change the base?
Conversation
Hello. You may have forgotten to update the changelog!
|
id: runner_group | ||
env: | ||
# We are not able to use \d to check numeric values as bash does not allow them (not POSIX compliant) | ||
RC_BRANCH_FORMAT_REGEX: v[0-9]+\.[0-9]+\.[0-9]+_rc[0-9]? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RC_BRANCH_FORMAT_REGEX: v[0-9]+\.[0-9]+\.[0-9]+_rc[0-9]? | |
RC_BRANCH_FORMAT_REGEX: v[0-9]+\.[0-9]+\.[0-9]+-rc[0-9]? |
We usually use dashes between the version number and "rc0", not underscores
determine_runner: | ||
if: github.event.pull_request.draft == false | ||
name: Determine runner type to use | ||
uses: ./.github/workflows/determine-workflow-runner.yml | ||
with: | ||
default_runner: ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it make sense to potentially use the large runners for running black
and pylint
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you say we should exclude format/docs_check/upload to codecov from large runners?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excluding upload-to-codecov
and format
makes sense to me. I think including docs
has some merit, considering that it's actually very useful for new features or big refactors to verify that the sphinx build passes without issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about it, the more sense it makes to me to use the large runners for the actions above. If a PR is marked as urgent, we should treat all workflows as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite good! Just a couple of questions about where it might make sense to not use large runners.
There is also a syntax error with the |
…n once vs many times)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6273 +/- ##
==========================================
- Coverage 99.59% 99.58% -0.01%
==========================================
Files 443 443
Lines 42255 42273 +18
==========================================
+ Hits 42082 42096 +14
- Misses 173 177 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the test runtimes expected to stay the same even while using the large runners? I don't see noticeable improvements in the runtimes based on the most recent CI run, especially in the core tests. Additionally, looks like the jax tests are quite imbalanced when using the large runners. We might need different durations.json
files for the case where large runners are used.
If that's expected, once the conversation about using large runners with the docs
and format
workflows is resolved, I'm happy to approve.
Context:
Currently the CI gets congested when large amounts of pull requests are being updated simultaneously. This pull request gives PRs an escape hatch and use large runners and use different queue to have CI jobs be picked up.
Description of the Change:
This pull request adds two new features:
urgent
label to any pull request and switch it over to large runnersvX.Y.Z-rcN
Large runners, albeit slightly more powerful than standard runners, can be spawned at a much higher volume than standard runners ... this is because we pay per minute for these runners vs being included on our GitHub Plan.
If a PR needs CI run without waiting for a runner, add the
urgent
label to the pull request.Important Note:
pull_request
and useubuntu
runners.Benefits:
Ability to leverage large runner to have quick time for a runner to pick up a job.
Possible Drawbacks:
Though we dictate the pool size of large runners, it is possible to still saturate it.
Related GitHub Issues:
None. sc-73711