Fix mergeability check for ghstack PRs #118258

izaitsevfb · 2024-01-25T02:11:58Z

Changes

introduce --check-mergeability trymerge flag that attempts to merge PR locally, using the same merge logic as the mergebot, but requires just a read-only GITHUB_TOKEN and git repo.
change mergeability workflow to utilize the new --check-mergeability logic

Alternatives considered

Rewrite https://github.com/pytorch/test-infra/actions/workflows/pr-dependencies-check.yml to correctly support partially merged ghstacks.

That would be a slightly better approach, but ROI is lower, as it requires reimplementing trymerge logic and additional effort to consolidate the codebase (trymerge lives in pytorch repo).

pr-dependencies-check.yml still produces human-readable results for partially merged ghstack prs (even if it falsely reports them as non-mergeable).

Instead of introducing new trymerge flag, use existing flags, including --dry-run.

That didn't work, as no combination of existing flags skips the rule checks and ROCKSET lookups.

Testing

Manual testing trymerge.py --check-mergeability on the regular and ghstack PRs:

export GITHUB_TOKEN=
export GIT_REPO_DIR=`pwd`
export GITHUB_REPOSITORY=pytorch/pytorch
export GIT_REMOTE_URL=https://github.com/pytorch/pytorch


# Test 1 (2 prs, 1 is closed)
python3 ../pytorch/.github/scripts/trymerge.py --check-mergeability  117862
Skipping 1 of 2 PR (#117859) as its already been merged

echo $?
0

# Test 2 (3 prs, 1 is closed)
python3 ../pytorch/.github/scripts/trymerge.py --check-mergeability  118125
Skipping 1 of 3 PR (#117859) as its already been merged

echo $?
0

# Test 3 (3 prs, intentional conflicts introduced into `main`):

python3 ../pytorch/.github/scripts/trymerge.py --check-mergeability  118125
Skipping 1 of 3 PR (#117859) as its already been merged
stdout:
Auto-merging torch/_inductor/ir.py
Auto-merging torch/_inductor/lowering.py
CONFLICT (content): Merge conflict in torch/_inductor/lowering.py
error: could not apply 66ba5b8792f... Realize inputs to DynamicScalar before unwrapping 
...
RuntimeError: Command `git -C /Users/ivanzaitsev/pytorch2 cherry-pick -x 66ba5b8792fa076c4e512d920651e5b6b7e466f4` returned non-zero exit code 1

Workflow run:
https://github.com/pytorch/pytorch/actions/runs/7660736172/job/20878651852?pr=118258

pytorch-bot · 2024-01-25T02:12:01Z

🔗 Helpful Links

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

📄 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 the bot commands wiki or our office hours

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

✅ No Failures

As of commit 38636cf with merge base b10b082 ():
💚 Looks good so far! There are no failures yet. 💚

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

… PR locally, using the same rules as the mergebot * change mergeability workflow to utilize the new --check-mergeability logic

PaliC

Some naming nits, but otherwise lgtm

.github/workflows/check_mergeability_ghstack.yml

izaitsevfb · 2024-01-26T00:29:14Z

@pytorchbot merge

pytorchmergebot · 2024-01-26T00:31:10Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

huydhn · 2024-01-26T02:13:32Z

.github/scripts/trymerge.py

-        gh_remove_label(
-            org, project, args.pr_num, MERGE_IN_PROGRESS_LABEL, args.dry_run
-        )
+        if not args.check_mergeability:


From what I see, it would always reach this part with not args.check_mergeability, so the if statement is kind of redundant?

Good catch. For some reason I thought the try block starts earlier.

huydhn · 2024-01-26T02:21:12Z

.github/workflows/check_mergeability_ghstack.yml

+          # and check if the following files were changed:
+          # .github/workflows/check_mergeability_ghstack.yml
+          # .github/scripts/trymerge.py
+          FILES_CHANGED=$(curl -s -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" \


This looks fine, but in case you need more complex logic (in bash :P), there are many GHA ready made to get the list of change files in a PR, i.e. https://github.com/tj-actions/changed-files

I already tried that action :)

Its description is a bit confusing, what it actually does, is checking the locally modified files, rather than the changest of the PR.

huydhn · 2024-01-26T02:52:25Z

.github/scripts/trymerge.py

+        pr.merge_changes(
+            repo,
+            branch="main",
+            skip_mandatory_checks=True,


Nit: IMO, having 2 params called skip_mandatory_checks and skip_all_rule_checks is a bit weird as the latter's name implies that it subsumes the former. For example, in this call, setting skip_mandatory_checks to True or False doesn't matter. I don't want it to give the impression that skip_all_rule_checks is an option to bring in a bigger force merge hammer. It's more like a different kind of --dry-run like what you call out in the description. So, may be a different name is more apt, i.e. check_mergability_mode

…st (#4907) In pytorch/pytorch#118258 mergeability check job was renamed to `ghstack-mergeability-check`. This PR updates flakiness exclusion accordingly.

malfet · 2024-01-29T15:04:57Z

.github/scripts/trymerge.py

+            get_ghstack_prs(repo, pr)  # raises error if out of sync
+        pr.merge_changes(
+            repo,
+            branch="main",


Please please please don't hardcode default branch name, as pr has get_default_branch() property

izaitsevfb requested a review from a team as a code owner January 25, 2024 02:11

pytorch-bot bot added the topic: not user facing topic category label Jan 25, 2024

izaitsevfb marked this pull request as draft January 25, 2024 02:12

izaitsevfb force-pushed the fix-ghstack-mergeability-check branch 6 times, most recently from e60bd69 to 026aab8 Compare January 25, 2024 21:34

izaitsevfb marked this pull request as ready for review January 25, 2024 21:50

izaitsevfb requested review from PaliC and huydhn January 25, 2024 21:50

* introduce --check-mergeability trymerge flag that attempts to merge…

2aab17d

… PR locally, using the same rules as the mergebot * change mergeability workflow to utilize the new --check-mergeability logic

izaitsevfb force-pushed the fix-ghstack-mergeability-check branch from 026aab8 to 2aab17d Compare January 25, 2024 23:28

PaliC approved these changes Jan 25, 2024

View reviewed changes

address review comments

38636cf

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jan 26, 2024

pytorchmergebot added the merging label Jan 26, 2024

izaitsevfb mentioned this pull request Jan 26, 2024

[ez] Update mergeability check job name in the flakiness exclusion list pytorch/test-infra#4907

Merged

huydhn reviewed Jan 26, 2024

View reviewed changes

huydhn approved these changes Jan 26, 2024

View reviewed changes

huydhn reviewed Jan 26, 2024

View reviewed changes

pytorchmergebot added the Merged label Jan 26, 2024

pytorchmergebot closed this in b599f56 Jan 26, 2024

pytorchmergebot removed the merging label Jan 26, 2024

malfet reviewed Jan 29, 2024

View reviewed changes

izaitsevfb mentioned this pull request Feb 12, 2024

"Check mergeability and dependencies for ghstack prs / pr-dependencies-check / check" fails when stack is based off of merged PR #117646

Closed

github-actions bot deleted the fix-ghstack-mergeability-check branch February 29, 2024 02:04

Fix mergeability check for ghstack PRs #118258

Fix mergeability check for ghstack PRs #118258

Uh oh!

Conversation

izaitsevfb commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Alternatives considered

Testing

Uh oh!

pytorch-bot bot commented Jan 25, 2024 • 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/118258

✅ No Failures

Uh oh!

PaliC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

izaitsevfb commented Jan 26, 2024

Uh oh!

pytorchmergebot commented Jan 26, 2024

Merge started

Uh oh!

huydhn Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

izaitsevfb Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

huydhn Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izaitsevfb Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huydhn Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malfet Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

izaitsevfb commented Jan 25, 2024 •

edited

Loading

pytorch-bot bot commented Jan 25, 2024 •

edited

Loading

huydhn Jan 26, 2024 •

edited

Loading

izaitsevfb Jan 26, 2024 •

edited

Loading

huydhn Jan 26, 2024 •

edited

Loading