-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
New model PR needs green (slow tests) CI #30341
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
c2ac266
to
60098f0
Compare
ab16223
to
6f25154
Compare
84a2e01
to
a022298
Compare
result: | ||
runs-on: ubuntu-22.04 | ||
name: Check test status | ||
needs: [run_test] | ||
if: always() | ||
steps: | ||
- name: Check test status | ||
shell: bash | ||
run: | | ||
echo "${{ needs.run_test.result }}" | ||
if [ "${{ needs.run_test.result }}" = "failure" ]; then echo "failure"; exit -1; else echo "pass"; fi; |
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.
We want to block the merge button if a new model PR has any failing slow test. But since the previous job run_test
may have a skipped
status (if there is no new model), we still need a job (i.e. this one) to indicate it as successful
so we can still show the merge button in this case.
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.
🤗
Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
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.
Thanks for adding!
As @ArthurZucker says - running for all commits for maintainers isn't ideal. If necessary, having [run-slow]
flag in the commit message seems OK, but would be great to have this as something which is necessary for merge but we can also control from within the github web environment
I think we can use issue_comment and However, I will have to test it in my own repo. first. |
fe7bd63
to
6073cd8
Compare
check_for_new_model: | ||
runs-on: ubuntu-22.04 | ||
name: Check if PR is a new model PR | ||
if: contains(github.event.pull_request.labels.*.name, 'single-model-run-slow') |
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.
After looking into the GitHub Actions doc and online discussion, I decide to add this condition.
Only after the PR has been labeled with single-model-run-slow
, the subsequent pull_request
events (like push more commits to the PR) will trigger the workflow.
Note that, the push after being labeled by single-model-run-slow
is still necessary to trigger the CI.
I avoid to trigger the CI by using issue_comment
(as this is way too frequent, despite we can filter it out later).
So please ask the contributors to push an empty commit after add this label. For external contributors' PR, we still need to click Approve and run
button.
We can explore the pull_request: labeled
event (this is probably less frequent I think) in the future.
cc @amyeroberts and @ArthurZucker
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.
Only after the PR has been labeled with single-model-run-slow
Just to make sure I've understood correctly how to trigger, we add the label single-model-run-slow
label via the Labels
section on the PR e.g. like New model
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.
add the label single-model-run-slow
This is the requirement, but it won't actually trigger the CI. The contributor has to push a commit (even empty) to trigger it.
But we can explore to use another event together where when we add a label (pull_request: labeled
). In that event, the CI will be triggered (if the added label is single-model-run-slow
).
Why we don't just use the above: because the PR might need multiple CI runs before it is ready to be merged. And we don't want the reviewers to add label / remove it / re-add label to achieves this.
3d02064
to
93522c4
Compare
What does this PR do?
For a new model PR, let's trigger and run the slow tests of that model.
Question: if we want to trigger the slow tests for each commit in a PR, or we prefer to trigger it toward the end of the PR (i.e. almost ready to be merged to main)? In the later case, we can use the commit message (i.e. having a prefix like
[run_slow]
).