Give better error message when merge fails to find any rules #84160

huydhn · 2022-08-27T00:23:32Z

Fixes #84147 and pytorch/test-infra#421.

If merge rule file is missing or fails to load for whatever reasons:

No rules find to match PR, please [report]{issue_link} this issue to DevX team.

If the list of rules is empty:

Merges are not allowed into repository without a rules.

pytorch-bot · 2022-08-27T00:23:34Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/84160

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit d865825:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

ZainRizvi

Thanks for making this change!

want to request more functionality

malfet

Please do not ask people to file an issue every time merge rules are not found
Nor do advice people to rebase, it should not help

.github/scripts/trymerge.py

ZainRizvi

These changes are great, but it doesn't really address #84147, right?

Perhaps those could go in another PR?

huydhn · 2022-08-29T17:17:00Z

These changes are great, but it doesn't really address #84147, right?

Perhaps those could go in another PR?

We already have most of the features you mention in #84147 I think:

https://github.com/pytorch/pytorch/blob/master/.github/scripts/trymerge.py#L1021-L1042 is to get the list of reviewers required for the PR following the merge rules
https://github.com/pytorch/pytorch/blob/master/.github/scripts/trymerge.py#L1048-L1065 is to tell which checks fail and the links to them on HUD

The only reason #84147 has such cryptic message was that it failed to find the merge rule (fixed by #84084). Thus, the default reject message was shown. So I think we just need to be clearer here

ZainRizvi · 2022-08-29T17:24:22Z

Ah, I see, thanks for the explanation. The code does look like it should have done the right thing.

I'll try to re-merge that PR (which should still fail due to the missing approval) to verify that a better error is shown.

malfet · 2022-08-29T17:36:29Z

@pytorchbot merge

malfet

Perhaps two usecases you want to cover:

if len(merge_rules) == 0 mergebot should print a message saying that "Merges are not allowed into repository without a rules"
Default message should say "No rules find to match PR , please report this issue to ..." (Also, can we specify a default issue message?)

pytorchmergebot · 2022-08-29T17:40:36Z

@pytorchbot successfully started a merge job. Check the current status here and land check progress here.
The merge job was triggered with the land checks (-l) flag. If you did not specify this flag yourself, you are likely enrolled in the land checks rollout. This means that your change will be merged once all checks on your PR and the land checks have passed (ETA 4 Hours). If you need to coordinate lands between different changes and cannot risk a land race, please add the ciflow/trunk label to your PR and wait for signal to complete, and then land your changes in proper order. Having trunk, pull, and Lint pre-run on a PR will bypass land checks and the ETA should be immediate. If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

pytorchmergebot · 2022-08-29T17:40:38Z

Merge failed
Reason: Matched rule Core Maintainers, but PR #84160 has not been reviewed yet
Raised by workflow job If you believe this is an error, you can use the old behavior with @pytorchbot merge -g (optionally with the ciflow/trunk to get land checks) or use @pytorchbot merge -f "some reason here". For more information, see the bot wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

Fixes #84147 and pytorch/test-infra#421. * If merge rule file is missing, the follow error will be printed out with `rules_path` replaced by the actual path like `merge_rules.yaml`. The most likely reason here is that we are doing something with `trymerge` intentionally. ``` merge_rules.yml does not exist, no merge rule can be found. Please run `@pytorchbot rebase -s` to rebase the PR onto PyTorch stable branch before trying to merge again. ``` * If any runtime error occurs or no rule matches, this is very likely an error. So we will provide the link to submit a [bug report](https://github.com/pytorch/pytorch/issues/new?labels=module%3A%20ci&template=bug-report.yml) to DevX ``` PR 12345 does not match any merge rules. This should not happen. Please help file a [bug report]({issue_link}) to PyTorch DevX Team with the PR number. ``` Pull Request resolved: #84160 Approved by:

huydhn · 2022-08-29T18:39:04Z

Perhaps two usecases you want to cover:

if len(merge_rules) == 0 mergebot should print a message saying that "Merges are not allowed into repository without a rules"

Default message should say "No rules find to match PR , please report this issue to ..." (Also, can we specify a default issue message?)

Fix the message. On the other hand, I'll work on the change to not start land validation if the PR hasn't been reviewed. This looks quite different than this one, so I think it would be best to keep that in a separate PR.

malfet · 2022-08-29T19:44:58Z

.github/scripts/trymerge.py

-            print(f"{rules_path} does not exist, returning empty rules")
-            return []
+            reject_reason = (f"{rules_path} does not exist")
+            raise RuntimeError(reject_reason)


This change is not needed with later change, is it?

malfet · 2022-08-29T19:46:48Z

.github/scripts/trymerge.py

+    reject_reason = (f"No rules find to match PR, "
+                     f"please [report]{issue_link} this issue to DevX team")


Don't use f decorator in front of string literals. Also, do not rely on the implicit concatenation, but rather add + operator or just keep it on the single line (or line length is 120 characters, isn't it?)

Suggested change

reject_reason = (f"No rules find to match PR, "

f"please [report]{issue_link} this issue to DevX team")

reject_reason = ("No rules find to match PR, "

f"please [report]{issue_link} this issue to DevX team")

malfet · 2022-08-29T19:48:07Z

.github/scripts/trymerge.py

+    try:
+        rules = read_merge_rules(repo, pr.org, pr.project)
+    except Exception as error:
+        # Any error here means that the script couldn't load the merge rules for
+        # some reasons, and the reject message should say so
+        raise RuntimeError(f"{reject_reason}: {error}")


Please don't having a generic exception catching is a bad practice (i.e. this function should already raise correct errors

Suggested change

try:

rules = read_merge_rules(repo, pr.org, pr.project)

except Exception as error:

# Any error here means that the script couldn't load the merge rules for

# some reasons, and the reject message should say so

raise RuntimeError(f"{reject_reason}: {error}")

rules = read_merge_rules(repo, pr.org, pr.project)

.github/scripts/trymerge.py

ZainRizvi · 2022-08-29T19:44:38Z

.github/scripts/trymerge.py

+        project=pr.project,
+        labels=["module: ci"],
+    )
+    reject_reason = (f"No rules find to match PR, "


nit

Suggested change

reject_reason = (f"No rules find to match PR, "

reject_reason = (f"No rules found to match PR, "

ZainRizvi · 2022-08-29T19:48:35Z

.github/scripts/trymerge.py

+        raise RuntimeError(f"{reject_reason}: {error}")
+
+    if not rules:
+        reject_reason = "Merges are not allowed into repository without a rules."


"Without a rules" is a bit hard to understand. Can we make the suggested action more explicit?

Perhaps:

Suggested change

reject_reason = "Merges are not allowed into repository without a rules."

reject_reason = "No matching merge rules found in merge_rules.yaml. Please fix file contents to ensure we have a valid merge rule are defined."

Perhaps "Rejecting the merge as no rules are defined for the repository."

Also, let's not hardcode file name multiple times in the code, but rather move it into top level variable or something

.github/scripts/test_trymerge.py

.github/scripts/trymerge.py

Co-authored-by: Nikita Shulga <nikita.shulga@gmail.com>

…failed-merge-helper

ZainRizvi

One small nit. Otherwise looking good!

.github/scripts/trymerge.py

Co-authored-by: Zain Rizvi <ZainRizvi@users.noreply.github.com>

huydhn · 2022-08-29T21:16:15Z

@pytorchbot merge -g

pytorchmergebot · 2022-08-29T21:19:46Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

github-actions · 2022-08-30T01:47:11Z

Hey @huydhn.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

…#84160) Summary: Fixes #84147 and pytorch/test-infra#421. * If merge rule file is missing or fails to load for whatever reasons: ``` No rules find to match PR, please [report]{issue_link} this issue to DevX team. ``` * If the list of rules is empty: ``` Merges are not allowed into repository without a rules. ``` Pull Request resolved: #84160 Approved by: https://github.com/ZainRizvi, https://github.com/malfet Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/adc54dc2195fbfe37b2f01649b8788314382a9be Reviewed By: mehtanirav Differential Revision: D39123427 Pulled By: huydhn fbshipit-source-id: 69b02163b76eefac2bd64758713f65bbcb287ac4

Give better error message when merge fails to find any rules

b878625

huydhn requested a review from ZainRizvi August 27, 2022 00:23

huydhn requested a review from a team as a code owner August 27, 2022 00:23

facebook-github-bot added the cla signed label Aug 27, 2022

huydhn self-assigned this Aug 27, 2022

ZainRizvi previously approved these changes Aug 29, 2022

View reviewed changes

malfet requested changes Aug 29, 2022

View reviewed changes

.github/scripts/trymerge.py Outdated Show resolved Hide resolved

.github/scripts/trymerge.py Outdated Show resolved Hide resolved

.github/scripts/trymerge.py Outdated Show resolved Hide resolved

ZainRizvi requested changes Aug 29, 2022

View reviewed changes

malfet reviewed Aug 29, 2022

View reviewed changes

Fix messages

5958f9a

huydhn requested review from ZainRizvi and malfet August 29, 2022 18:43

ZainRizvi mentioned this pull request Aug 29, 2022

random tweak #81893

Closed

malfet reviewed Aug 29, 2022

View reviewed changes

ZainRizvi reviewed Aug 29, 2022

View reviewed changes

huydhn added 2 commits August 29, 2022 13:04

Fix message

2c9832f

Fix typo

b59e185

malfet approved these changes Aug 29, 2022

View reviewed changes

.github/scripts/test_trymerge.py Outdated Show resolved Hide resolved

malfet reviewed Aug 29, 2022

View reviewed changes

.github/scripts/trymerge.py Outdated Show resolved Hide resolved

.github/scripts/trymerge.py Outdated Show resolved Hide resolved

.github/scripts/trymerge.py Outdated Show resolved Hide resolved

huydhn and others added 2 commits August 29, 2022 13:12

Update .github/scripts/trymerge.py

945e248

Co-authored-by: Nikita Shulga <nikita.shulga@gmail.com>

Update .github/scripts/trymerge.py

816a523

Co-authored-by: Nikita Shulga <nikita.shulga@gmail.com>

huydhn added 2 commits August 29, 2022 13:18

Fix messages

2b467ca

Merge branch 'failed-merge-helper' of github.com:huydhn/pytorch into …

ade5ec8

…failed-merge-helper

huydhn requested a review from ZainRizvi August 29, 2022 20:25

ZainRizvi approved these changes Aug 29, 2022

View reviewed changes

.github/scripts/trymerge.py Outdated Show resolved Hide resolved

Update .github/scripts/trymerge.py

d865825

Co-authored-by: Zain Rizvi <ZainRizvi@users.noreply.github.com>

pytorchmergebot added the Merged label Aug 29, 2022

pytorchmergebot closed this in adc54dc Aug 29, 2022

		reject_reason = (f"No rules find to match PR, "
		f"please [report]{issue_link} this issue to DevX team")

	reject_reason = (f"No rules find to match PR, "
	reject_reason = (f"No rules found to match PR, "

	reject_reason = "Merges are not allowed into repository without a rules."
	reject_reason = "No matching merge rules found in merge_rules.yaml. Please fix file contents to ensure we have a valid merge rule are defined."

Give better error message when merge fails to find any rules #84160

Give better error message when merge fails to find any rules #84160

Uh oh!

Conversation

huydhn commented Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/84160

✅ No Failures

Uh oh!

ZainRizvi left a comment

Choose a reason for hiding this comment

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZainRizvi left a comment

Choose a reason for hiding this comment

Uh oh!

huydhn commented Aug 29, 2022

Uh oh!

ZainRizvi commented Aug 29, 2022

Uh oh!

malfet commented Aug 29, 2022

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

pytorchmergebot commented Aug 29, 2022

Uh oh!

pytorchmergebot commented Aug 29, 2022

Uh oh!

huydhn commented Aug 29, 2022

Uh oh!

malfet Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

malfet Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

malfet Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZainRizvi Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

ZainRizvi Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

malfet Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZainRizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huydhn commented Aug 29, 2022

Uh oh!

pytorchmergebot commented Aug 29, 2022

Uh oh!

github-actions bot commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

huydhn commented Aug 27, 2022 •

edited

Loading

pytorch-bot bot commented Aug 27, 2022 •

edited

Loading