[mergebot] Flatten workflows into jobs, fix bugs #92097
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/92097
Note: Links to docs will display an error until the docs builds have been completed. ❌ 6 FailuresAs of commit 8da3a25: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
clee2000
changed the title
clee2000
force-pushed
the
csl/trymergechanges
branch
from
d48377c to
c691d1d
Compare
clee2000
force-pushed
the
csl/trymergechanges
branch
from
c691d1d to
536f655
Compare
| required_checks = [] | ||
| failed_rule_message = None | ||
| try: | ||
| find_matching_merge_rule(pr, repo) |
There was a problem hiding this comment.
If I read the code correctly, this would raise MandatoryChecksMissingError when there are pending jobs. So how are the 2 branches of failed_rule_message is not None run on line 1511?
I guess the question is about the exceptions raised by find_matching_merge_rule (also calls get_combined_checks_from_pr_and_land_validation inside) v.s. the exceptions raising here in merge. They looks duplicated. But then trymerge logic is complicated, so I wonder what I miss here
There was a problem hiding this comment.
not entirely sure if i understand the first question, but some None checks are just in case.
For the exception raising duplication, let's say a pull job is pending, but a trunk job has failed. Then find_matching_merge_rule doesn't see the failing trunk job because it only checks the mandatory jobs (none of which include trunk at the moment). Then it raises MandatoryChecksMissingError which previously would get caught at the end of the loop, and we would wait 5 minutes and run the loop again. By catching the MandatoryChecksMissingError earlier, we can also check the non mandatory checks (trunk) and raise RuntimeError, which would cause the trymerge script to exit early.
get_combined_checks_from_pr_and_land_validation does get called twice but get_checkrun_conclusions does check for self.conclusions first, so at least its not rerunning all the code.
There was a problem hiding this comment.
Got it, so find_matching_merge_rule is for mandatory checks while merge will handle both mandatory and non-mandatory ones. The latter behavior is what I'm familiar with, so I wrongly thought that find_matching_merge_rule did both. This also answers my first question.
There was a problem hiding this comment.
Per discussion with @clee2000, we should consider just get rid of the obsolete distinction between mandatory and non-mandatory checks in trymerge (not as part of this PR though). So the logic could be as simple as:
- Does the PR have the correct approval and labels?
- Yes, continue
- No, the merge process stops and clearly says the reason
- Are pull and trunk run?
- Yes, continue
- No, the merge process stops and clearly says the reason
- No job fails. All is good and the commit will be merged
- Some jobs fail. Are they ignorable?
- Yes, the commit will be merged
- No, the merge process stops and clearly says the reason
There was a problem hiding this comment.
+1 this is the most confusing chunk of our merge rules and would be great to simplify them like that
| required_checks = [] | ||
| failed_rule_message = None | ||
| try: | ||
| find_matching_merge_rule(pr, repo) |
There was a problem hiding this comment.
+1 this is the most confusing chunk of our merge rules and would be great to simplify them like that
|
@pytorchbot merge -f "lint passed, failures are disabled dynamo tests and some other windows thing that ive seen on some other prs" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Uh oh!
There was an error while loading. Please reload this page.