You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/06/03 21:39:12 UTC
[GitHub] [tvm] driazati opened a new pull request, #11569: [ci][mergebot] De-duplicate GitHub Actions tests
driazati opened a new pull request, #11569:
URL: https://github.com/apache/tvm/pull/11569
A lot of the GitHub Actions automation counts as a job on a PR (e.g. the
tag teams workflow) and they run multiple times. This would lead to
skipped/cancelled workflows showing up as errors when trying to merge,
which this PR fixes by only looking at the most recent run of each
action.
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] driazati commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r894795138
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +433,10 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
return [
job
for job in self.ci_jobs()
- if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED"}
+ if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED", "NEUTRAL"}
Review Comment:
ah good point, I think for now the best way would be to ignore jobs that aren't prefixed with `CI /` (since those are all the ones that don't matter for merge status)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
Mousius commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r895013525
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +435,16 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
+ def is_important_job(job):
+ # Ignore GitHub workflows that operate on PRs but don't run tests
+ if job["source"] != "github actions":
+ return True
+ return job["name"].startswith("CI /")
Review Comment:
I might be missing the obvious here, but can you explain how this is tested @driazati ? I've looked through all the fixtures and can't see the string `CI /`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r894839912
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +435,16 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
+ def is_important_job(job):
Review Comment:
maybe could find a better word than "important" here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] github-actions[bot] commented on pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #11569:
URL: https://github.com/apache/tvm/pull/11569#issuecomment-1164270657
It has been a while since this PR was updated, @Mousius @areusch please leave a review or address the outstanding comments. @driazati if this PR is still a work in progress, please [convert it to a draft](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request#converting-a-pull-request-to-a-draft) until it is ready for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
Mousius commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r892363287
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +433,10 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
return [
job
for job in self.ci_jobs()
- if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED"}
+ if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED", "NEUTRAL"}
Review Comment:
My concern would be if we had some window where a previous job had been cancelled but not yet restarted by GitHub, is that a possibility?
Also, I think we need an additional test with the new status in it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] driazati commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r896076873
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +435,16 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
+ def is_important_job(job):
+ # Ignore GitHub workflows that operate on PRs but don't run tests
+ if job["source"] != "github actions":
+ return True
+ return job["name"].startswith("CI /")
Review Comment:
It's the full name of the GitHub actions jobs, e.g. in the signals box on a PR, so its the workflow name + the job name:
<img width="980" alt="image" src="https://user-images.githubusercontent.com/9407960/173432916-c7393d49-ca9b-4643-b587-9132aed59c8b.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
Mousius commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r891477683
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +433,10 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
return [
job
for job in self.ci_jobs()
- if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED"}
+ if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED", "NEUTRAL"}
Review Comment:
Do we want cancelled jobs (`NEUTRAL`) to be considered a success?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] driazati commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r894795138
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +433,10 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
return [
job
for job in self.ci_jobs()
- if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED"}
+ if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED", "NEUTRAL"}
Review Comment:
ah good point, I think for now the best way would be to ignore jobs that aren't prefixed with `CI /` (since those are all the ones that don't matter for merge status). pushed this fix + updated the new test to check this as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] driazati closed pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
driazati closed pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
URL: https://github.com/apache/tvm/pull/11569
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
Mousius commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r897711830
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +435,16 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
+ def is_important_job(job):
+ # Ignore GitHub workflows that operate on PRs but don't run tests
+ if job["source"] != "github actions":
+ return True
+ return job["name"].startswith("CI /")
Review Comment:
Argh, it's this difficult to read formatting:
https://github.com/apache/tvm/pull/11569/files#diff-1cf1d75bfab53cc02abbcf96c356bfdbd477fb9d76ed5558d49e426a641c53f8R292-R301
Missed the `+`s instead of `,` in the item, that's what lead to me not finding the string 😿
To follow up, I'm not clear if de-duplication is no longer tested, or if this is not tested - looking at the de-duplication fixture the jobs are these:
| name | checkSuite.workflowRun.workflow.name | status | completedAt | conclusion | url | state | context | targetUrl |
| ------------ | ------------------------------------ | --------- | -------------------- | ---------- | --------------------------------------------- | ------- | -------------- | ------------------------------------------------------------- |
| cc-reviewers | PR | COMPLETED | 2022-06-01T15:32:23Z | CANCELLED | https://github.com/apache/tvm/runs/6692618718 | | | |
| tag-teams | Teams | COMPLETED | 2022-06-01T15:32:21Z | CANCELLED | https://github.com/apache/tvm/runs/6692618705 | | | |
| MacOS | CI | COMPLETED | 2022-05-28T00:52:07Z | SUCCESS | https://github.com/apache/tvm/runs/6633016151 | | | |
| maybe-merge | Merge | COMPLETED | 2022-06-01T20:08:01Z | SUCCESS | https://github.com/apache/tvm/runs/6697027810 | | | |
| cc-reviewers | PR | COMPLETED | 2022-05-27T23:21:28Z | SUCCESS | https://github.com/apache/tvm/runs/6633016519 | | | |
| cc-reviewers | PR | COMPLETED | 2022-05-27T23:22:11Z | SUCCESS | https://github.com/apache/tvm/runs/6633020322 | | | |
| cc-reviewers | PR | COMPLETED | 2022-06-01T15:34:03Z | SUCCESS | https://github.com/apache/tvm/runs/6692623314 | | | |
| tag-teams | Teams | COMPLETED | 2022-05-27T23:21:23Z | SUCCESS | https://github.com/apache/tvm/runs/6633016525 | | | |
| tag-teams | Teams | COMPLETED | 2022-05-27T23:22:25Z | FAILED | https://github.com/apache/tvm/runs/6633020321 | | | |
| tag-teams | Teams | COMPLETED | 2022-06-01T15:33:09Z | SUCCESS | https://github.com/apache/tvm/runs/6692621919 | | | |
| Windows | CI | COMPLETED | 2022-05-28T00:48:58Z | SUCCESS | https://github.com/apache/tvm/runs/6633016091 | | | |
| Android | CI | COMPLETED | 2022-05-28T00:31:19Z | SUCCESS | https://github.com/apache/tvm/runs/6633016015 | | | |
| | | | | | | SUCCESS | tvm-ci/pr-head | https://ci.tlcpack.ai/job/tvm/job/PR-11455/4/display/redirect |
If we're now filtering it so that anything non-"CI" is ignored, then the above table reduces to:
| name | checkSuite.workflowRun.workflow.name | status | completedAt | conclusion | url | state | context | targetUrl |
| ------------ | ------------------------------------ | --------- | -------------------- | ---------- | --------------------------------------------- | ------- | -------------- | ------------------------------------------------------------- |
| MacOS | CI | COMPLETED | 2022-05-28T00:52:07Z | SUCCESS | https://github.com/apache/tvm/runs/6633016151 | | | |
| Windows | CI | COMPLETED | 2022-05-28T00:48:58Z | SUCCESS | https://github.com/apache/tvm/runs/6633016091 | | | |
| Android | CI | COMPLETED | 2022-05-28T00:31:19Z | SUCCESS | https://github.com/apache/tvm/runs/6633016015 | | | |
| | | | | | | SUCCESS | tvm-ci/pr-head | https://ci.tlcpack.ai/job/tvm/job/PR-11455/4/display/redirect |
Therefore, regardless of de-duplication, this would still pass with the new logic?
I looked at the `badci` fixture and it appears that also only checks a Jenkins job rather than the new GitHub actions checks?
Could you please double check we have test cases for both of these states?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] driazati commented on a diff in pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #11569:
URL: https://github.com/apache/tvm/pull/11569#discussion_r891485537
##########
tests/scripts/github_tvmbot.py:
##########
@@ -417,11 +433,10 @@ def author(self) -> str:
return self.raw["author"]["login"]
def find_failed_ci_jobs(self) -> List[CIJob]:
- # NEUTRAL is GitHub Action's way of saying cancelled
return [
job
for job in self.ci_jobs()
- if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED"}
+ if job["status"] not in {"SUCCESS", "SUCCESSFUL", "SKIPPED", "NEUTRAL"}
Review Comment:
this change is here is because lots of the PR automation (e.g. the cc bot and teams tagging) run on every comment, but new runs cancel old runs, so there are usually a couple cancelled jobs on PRs. We could add a blocklist and just ignore these job names but most users aren't able to cancel jobs anyways and this doesn't require being synced manually if we make any changes to those
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #11569: [ci][mergebot] De-duplicate GitHub Actions tests
Posted by GitBox <gi...@apache.org>.
Mousius commented on PR #11569:
URL: https://github.com/apache/tvm/pull/11569#issuecomment-1157527687
@driazati feel free to ping when this is ready for review again 😸
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org