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

New model PR needs green (slow tests) CI #30341

Merged
merged 23 commits into from
Apr 24, 2024
Merged

New model PR needs green (slow tests) CI #30341

merged 23 commits into from
Apr 24, 2024

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Apr 19, 2024

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]).

@HuggingFaceDocBuilderDev

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.

@ydshieh ydshieh force-pushed the new_model_pr_need_ci branch 3 times, most recently from c2ac266 to 60098f0 Compare April 19, 2024 19:20
@ydshieh ydshieh closed this Apr 19, 2024
@ydshieh ydshieh reopened this Apr 19, 2024
@ydshieh ydshieh force-pushed the new_model_pr_need_ci branch 2 times, most recently from ab16223 to 6f25154 Compare April 19, 2024 19:54
@ydshieh ydshieh marked this pull request as ready for review April 22, 2024 08:31
Comment on lines 113 to 123
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;
Copy link
Collaborator Author

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.

@ydshieh ydshieh changed the title [Draft] New model PR needs green CI New model PR needs green CI Apr 22, 2024
@ydshieh ydshieh changed the title New model PR needs green CI New model PR needs green (slow tests) CI Apr 22, 2024
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

🤗

.github/workflows/self-new-model-pr-caller.yml Outdated Show resolved Hide resolved
.github/workflows/self-new-model-pr-caller.yml Outdated Show resolved Hide resolved
Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
Copy link
Collaborator

@amyeroberts amyeroberts 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 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

utils/check_if_new_model_added.py Show resolved Hide resolved
@ydshieh ydshieh marked this pull request as draft April 23, 2024 06:17
@ydshieh
Copy link
Collaborator Author

ydshieh commented Apr 23, 2024

Convert to draft to check if I can using some github action events to control the CI trigger more precisely.

Hi @amyeroberts @ArthurZucker

I think we can use issue_comment and github.event.comment.body to trigger the CI with a comment on the pull request.

However, issue_comment event (this contain the pull request comment, and can be configured to pull-only comment) will only trigger a workflow run if the workflow file is on the default branch, so I am not able to test it while this PR is still not merged to main yet.

I will have to test it in my own repo. first.

@ydshieh ydshieh marked this pull request as ready for review April 23, 2024 06:20
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')
Copy link
Collaborator Author

@ydshieh ydshieh Apr 23, 2024

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

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@ydshieh ydshieh merged commit fc34f84 into main Apr 24, 2024
11 checks passed
@ydshieh ydshieh deleted the new_model_pr_need_ci branch April 24, 2024 07:52
This was referenced Apr 29, 2024
itazap pushed a commit that referenced this pull request May 14, 2024
* You should not pass

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
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.

None yet

4 participants