mirror of
https://github.com/pytorch/pytorch.git
synced 2025-10-21 05:34:18 +08:00
Further improve mergebot messages (#84283)
Reword the rejection reasons to better match the format mergebot uses to output the message, and repoints the workflow links to point to the commit page in hud instead of github
**Context:** Some of the mergebot messages looked a bit weird. For example, it would claim to be offering a reason for a merge failing, but instead the message would be of a more diagnostic nature.
Example of a weird message ("view failures on hud" is not a reason!):
<img width="960" alt="image" src="https://user-images.githubusercontent.com/4468967/187495195-9f1cf6cc-49e3-42a1-8c29-b1e95027d0e2.png">
The above message would now look like:
<img width="967" alt="image" src="https://user-images.githubusercontent.com/4468967/187726065-ea93dc34-065f-47bd-acd4-78415329e5a6.png">
Pull Request resolved: https://github.com/pytorch/pytorch/pull/84283
Approved by: https://github.com/huydhn
This commit is contained in:
Zain Rizvi
committed by
PyTorch MergeBot
PyTorch MergeBot
parent
c585e149e2
commit
fddfc4488a
59
.github/scripts/trymerge.py
vendored
59
.github/scripts/trymerge.py
vendored
@ -1031,7 +1031,14 @@ def find_matching_merge_rule(pr: GitHubPR,
|
||||
reject_reason = f"Rejecting the merge as no rules are defined for the repository in {MERGE_RULE_PATH}"
|
||||
raise RuntimeError(reject_reason)
|
||||
|
||||
# Used to determine best rejection reason
|
||||
# PRs can fail multiple merge rules, but it only needs to pass one rule to be approved.
|
||||
# If it fails all rules, we need to find the rule that it came closest to passing and report
|
||||
# that to the dev.
|
||||
#
|
||||
# reject_reason_score ranks rules by relevancy. The higher the score, the more relevant the
|
||||
# rule & rejection reason, and we only care about the most relevant rule/reason
|
||||
#
|
||||
# reject_reason_score intrepretation:
|
||||
# Score 0 to 10K - how many files rule matched
|
||||
# Score 10K - matched all files, but no overlapping approvers
|
||||
# Score 20K - matched all files and approvers, but mandatory checks are pending
|
||||
@ -1041,6 +1048,8 @@ def find_matching_merge_rule(pr: GitHubPR,
|
||||
rule_name = rule.name
|
||||
patterns_re = patterns_to_regex(rule.patterns)
|
||||
non_matching_files = []
|
||||
|
||||
# Does this rule apply to all the files?
|
||||
for fname in changed_files:
|
||||
if not patterns_re.match(fname):
|
||||
non_matching_files.append(fname)
|
||||
@ -1048,16 +1057,21 @@ def find_matching_merge_rule(pr: GitHubPR,
|
||||
num_matching_files = len(changed_files) - len(non_matching_files)
|
||||
if num_matching_files > reject_reason_score:
|
||||
reject_reason_score = num_matching_files
|
||||
reject_reason = (f"{num_matching_files} files matched rule {rule_name}, but there are still non-matching files: " +
|
||||
f"{','.join(non_matching_files[:5])}{', ...' if len(non_matching_files) > 5 else ''}")
|
||||
reject_reason = "\n".join((
|
||||
f"Not all files match rule `{rule_name}`."
|
||||
f"{num_matching_files} files matched, but there are still non-matching files:"
|
||||
f"{','.join(non_matching_files[:5])}{', ...' if len(non_matching_files) > 5 else ''}"
|
||||
))
|
||||
continue
|
||||
|
||||
# If rule needs approvers but PR has not been reviewed, skip it
|
||||
if len(rule.approved_by) > 0 and len(approved_by) == 0:
|
||||
if reject_reason_score < 10000:
|
||||
reject_reason_score = 10000
|
||||
reject_reason = f"Matched rule {rule_name}, but PR #{pr.pr_num} has not been reviewed yet"
|
||||
reject_reason = f"PR #{pr.pr_num} has not been reviewed yet (Rule {rule_name})"
|
||||
continue
|
||||
|
||||
# Does the PR have the required approvals for this rule?
|
||||
rule_approvers_set = set()
|
||||
for approver in rule.approved_by:
|
||||
if "/" in approver:
|
||||
@ -1070,34 +1084,43 @@ def find_matching_merge_rule(pr: GitHubPR,
|
||||
if len(approvers_intersection) == 0 and len(rule_approvers_set) > 0:
|
||||
if reject_reason_score < 10000:
|
||||
reject_reason_score = 10000
|
||||
reject_reason = (f"Matched rule {rule_name}, but PR #{pr.pr_num} was not reviewed yet by any of: " +
|
||||
f"{', '.join(list(rule_approvers_set)[:5])}{', ...' if len(rule_approvers_set) > 5 else ''}")
|
||||
reject_reason = "\n".join((
|
||||
f"Approval needed from one of the following (Rule '{rule_name}'):",
|
||||
f"{', '.join(list(rule_approvers_set)[:5])}{', ...' if len(rule_approvers_set) > 5 else ''}"
|
||||
))
|
||||
continue
|
||||
|
||||
# Does the PR pass the checks required by this rule?
|
||||
mandatory_checks = rule.mandatory_checks_name if rule.mandatory_checks_name is not None else []
|
||||
checks = get_combined_checks_from_pr_and_land_validation(pr, land_check_commit)
|
||||
required_checks = filter(lambda x: skip_mandatory_checks is False or "CLA Check" in x, mandatory_checks)
|
||||
[pending_checks, failed_checks] = categorize_checks(checks, required_checks)
|
||||
|
||||
hud_link = f"https://hud.pytorch.org/{pr.org}/{pr.project}/commit/{pr.last_commit()['oid']}"
|
||||
if len(failed_checks) > 0:
|
||||
if reject_reason_score < 30000:
|
||||
reject_reason_score = 30000
|
||||
reject_reason = (
|
||||
f"[View failures on hud](https://hud.pytorch.org/{pr.org}/{pr.project}/commit/{pr.last_commit()['oid']}). "
|
||||
+ f"Refusing to merge as mandatory check(s) {checks_to_str(failed_checks)} failed for "
|
||||
+ f"rule {rule_name}."
|
||||
)
|
||||
reject_reason = "\n".join((
|
||||
f"The following mandatory check(s) failed (Rule `{rule_name}`):",
|
||||
*checks_to_markdown_bullets(failed_checks),
|
||||
"",
|
||||
f"Dig deeper by [viewing the failures on hud]({hud_link})"
|
||||
))
|
||||
continue
|
||||
elif len(pending_checks) > 0:
|
||||
if reject_reason_score < 20000:
|
||||
reject_reason_score = 20000
|
||||
reject_reason = (
|
||||
f"[View pending jobs on hud](https://hud.pytorch.org/{pr.org}/{pr.project}/commit/{pr.last_commit()['oid']}). "
|
||||
+ f"Refusing to merge as mandatory check(s) {checks_to_str(pending_checks)} are pending/not yet run for "
|
||||
+ f"rule {rule_name}."
|
||||
)
|
||||
reject_reason = "\n".join((
|
||||
f"The following mandatory check(s) are pending/not yet run (Rule `{rule_name}`):",
|
||||
*checks_to_markdown_bullets(pending_checks),
|
||||
"",
|
||||
f"Dig deeper by [viewing the pending checks on hud]({hud_link})"
|
||||
))
|
||||
continue
|
||||
|
||||
if not skip_internal_checks and pr.has_internal_changes():
|
||||
raise RuntimeError("This PR has internal changes and must be landed via Phabricator")
|
||||
|
||||
return rule
|
||||
|
||||
if reject_reason_score == 20000:
|
||||
@ -1134,6 +1157,10 @@ def get_land_checkrun_conclusions(org: str, project: str, commit: str) -> Dict[s
|
||||
def checks_to_str(checks: List[Tuple[str, Optional[str]]]) -> str:
|
||||
return ", ".join(f"[{c[0]}]({c[1]})" if c[1] is not None else c[0] for c in checks)
|
||||
|
||||
|
||||
def checks_to_markdown_bullets(checks: List[Tuple[str, Optional[str]]]) -> List[str]:
|
||||
return [f"- [{c[0]}]({c[1]})" if c[1] is not None else f"- {c[0]}" for c in checks]
|
||||
|
||||
def get_combined_checks_from_pr_and_land_validation(
|
||||
pr: GitHubPR,
|
||||
land_check_commit: Optional[str]
|
||||
|
||||
Reference in New Issue
Block a user