Pin base branch for pull requests

[samestep]

Context: As of #53719 (which replaced an equivalent solution from before), we use the ref input for actions/checkout to checkout the PR tip instead of a merge commit.

• Upside: This means that if the PR author bases off of viable/strict instead of master, the repo checkout for GHA runs on the PR won't pull in stuff from the latest (often-broken) master.
• Downside: This breaks atomicity, because even though the checkout comes from the PR tip, the actual workflow YAML files are still pulled from the merge commit of the PR with master. So if master adds a new workflow and some scripts that that workflow uses, any PR based on a commit behind that will fail in GHA. A couple examples:
  • https://github.com/pytorch/pytorch/runs/2059201673
  • https://github.com/pytorch/pytorch/runs/2141801798

Instead of breaking atomicity by checking out something besides the merge commit, this PR resolves the problem by forcing the merge commit to be the same as the PR head commit.

Specifically, it adds a workflow which listens for activity on PRs against master (such as when they're opened or when new commits are pushed), and updates the PR's base branch to be base/hhhhhhh, where hhhhhhh is the 7-digit prefix of the SHA1 hash of the merge-base of the PR head with pytorch:master. It also adds the "base pinned" label to the PR (if it doesn't have the label already). Because this new workflow is triggered by pull_request_target rather than pull_request, it must be merged before it can take effect. For demonstration purposes, I have manually set the base branch and "base pinned" label for this PR, but those would both be done automatically after it is landed.

Important: Before this PR is merged, a GitHub token called GITHUB_BASE_BRANCH_TOKEN must be added to the pytorch/pytorch repo. The new workflow uses said token to add a label in such a way that it will trigger a followup pull_request workflow run.

The timeline for a typical PR will look something like this:

• user opens PR against master
• pull_request workflows don't run because we restricted them to only run once they're pinned (or if they're ghstack PRs, which are always pinned anyway)
• the new workflow pins the PR's base branch, and adds the "base pinned" label which triggers another pull_request event (this time of type labeled)
• the pull_request workflows now run on that initial commit of the PR, using the PR head as the merge commit since the base branch has been pinned
• user pushes new commits, possibly rebasing onto a different master commit
• the new workflow switches the PR's base branch as necessary (only if the PR was rebased), but doesn't change the labels (assuming no one removed the "base pinned" label)
  • this is important because it ensures that the "Commits" and "Files" tabs only show the changes actually introduced by the PR, and not any that are already in the master history
• on each commit after the initial one, the pull_request workflow runs as normal, with type synchronize, because the base branch matches base/* so even if the user rebases onto a more recent commit from master, the PR head and merge commits will still be exactly the same

A few caveats of this implementation:

• if the user rebases on to a commit older than the one they were previously based on, the pull_request run immediately after that (before the "Pin base branch" workflow has a chance to run again) will use a merge commit different from the PR's head commit
  • this shouldn't usually happen since people usually rebase onto newer commits rather than older, and even if it does happen, it will only be for one commit: if the user pushes more commits after rebasing, the problem will go away because the "Pin base branch" workflow will have already fixed the problem
• if anyone adds a different tag besides "base pinned", CI will be triggered on the PR even though no changes were pushed
  • this shouldn't matter as far as resources thanks to Add GHA to cancel redundant GHA workflows except on master #54689, and might actually be a good thing if we're already planning on using labels to decide which workflows to run on a PR
  • however, one other label that gets added to every PR is "cla signed", so this would add some noise
  • it may be possible to avoid this problem by using if to check the label that was added and cancel jobs if it's not "base pinned", but that would add more boilerplate
• according to this script from Stack Overflow, there are currently 165 commits in PyTorch which require at least 8 digits of the hash to disambiguate, 12 of which actually require 9 digits; is 7 digits OK for now, or should we proactively set this workflow to use 9 or more digits?
  • even if we merge this as-is, there shouldn't really be a downside to increasing the number of digits in the hash later
• this is a big one: the "Import to Phabricator" button currently doesn't work on this PR
  • @bigfootjon has resolved this! 🎉
• a medium one: apparently if you rebase a PR whose base branch is pinned, it adds many code owners as reviewers, who then have to be manually removed

Other questions for reviewers:

• is it OK that now these pull_request workflows are limited to PRs with base branch base/* or gh/**? what about PRs against other branches, such as release branches?

Test plan:

@janeyx99 and I tested this in the same GitHub repo used to test #54685, including with PRs from forks.

[facebook-github-bot]

💊 CI failures summary and remediations

As of commit 364a7e5 (more details on the Dr. CI page):

---

• 1/2 failures possibly* introduced in this PR
  • 1/1 non-scanned failure(s)
• 1/2 broken upstream at merge base 9e6877c from Mar 27 until Mar 30

---

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

• clang-tidy from Mar 27 until Mar 30 (4d4fc02 - fd77e0e)
  • 🔁 rerun

---

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

[facebook-github-bot]

@bigfootjon has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[codecov]

Codecov Report

Merging #54693 (364a7e5) into base/9e6877c (9e6877c) will increase coverage by 0.00%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           base/9e6877c   #54693   +/-   ##
=============================================
  Coverage         77.45%   77.45%           
=============================================
  Files              1894     1894           
  Lines            186403   186403           
=============================================
+ Hits             144374   144383    +9     
+ Misses            42029    42020    -9     

[walterddr]

Looks good to me for this approach. Have a couple of questions and potential follow ups.

The timeline for a typical PR will look something like this:

• user opens PR against master
• pull_request workflows don't run because we restricted them to only run once they're pinned (or if they're ghstack PRs, which are always pinned anyway)

Have we tried this workflow on a ghstack PR. Specifically: Would ghstack PRs have the "base pinned" label added by this workflow? if not do we plan to implement this in ghstack or modify this workflow?

...

• user pushes new commits, possibly rebasing onto a different master commit
• the new workflow switches the PR's base branch as necessary (only if the PR was rebased), but doesn't change the labels (assuming no one removed the "base pinned" label)

will we advance the current base branch tip to the new master commit or do we create a new base branch? (e.g. do we increase # of branches on the pytorch repo?

...
A few caveats of this implementation:

• if the user rebases on to a commit older than the one they were previously based on, the pull_request run immediately after that (before the "Pin base branch" workflow has a chance to run again) will use a merge commit different from the PR's head commit

I dont think we need to worry about this, because a typical git rebase on an older commit will be a no-op as it will rebase all commits newer than the old commit hash onto the new branch if they are both on master.

[samestep]

@walterddr

Have we tried this workflow on a ghstack PR. Specifically: Would ghstack PRs have the "base pinned" label added by this workflow? if not do we plan to implement this in ghstack or modify this workflow?

The "Pin base branch" workflow only runs when the PR's base branch is already master or base/*, so this doesn't affect ghstack PRs (since their base branch always matches gh/**).

will we advance the current base branch tip to the new master commit or do we create a new base branch? (e.g. do we increase # of branches on the pytorch repo?

I don't understand what you mean here; could you clarify?

I dont think we need to worry about this, because a typical git rebase on an older commit will be a no-op as it will rebase all commits newer than the old commit hash onto the new branch if they are both on master.

Agreed.

[walterddr]

The "Pin base branch" workflow only runs when the PR's base branch is already master or base/*, so this doesn't affect ghstack PRs (since their base branch always matches gh/**).

in this case, who would be the one that tags base pinned to ghstack PRs? according to

...

• the new workflow pins the PR's base branch, and adds the "base pinned" label which triggers another pull_request event (this time of type labeled)

This means base pinned tag needs to be added to trigger another pull_request event right?

---

I don't understand what you mean here; could you clarify?

For example currently this PR is based off pytorch:base/6f8328e, if you rebase against a later commit on master, say ( pytorch:base/abcdefg, will you delete pytorch:base/6f8328e?

[samestep]

in this case, who would be the one that tags base pinned to ghstack PRs? according to

...

• the new workflow pins the PR's base branch, and adds the "base pinned" label which triggers another pull_request event (this time of type labeled)

This means base pinned tag needs to be added to trigger another pull_request event right?

The "Pin base branch" workflow adds that label automatically, which is why it requires a special GITHUB_BASE_BRANCH_TOKEN separate from the standard GITHUB_TOKEN (otherwise the followup pull_request event wouldn't trigger another workflow).

For example currently this PR is based off pytorch:base/6f8328e, if you rebase against a later commit on master, say ( pytorch:base/abcdefg, will you delete pytorch:base/6f8328e?

No, because it is possible that there are other PRs still using that same branch as their base (which is one of the reasons @seemethere suggested using hash prefixes instead of PR numbers for this).

[seemethere]

Perhaps a good follow up to this PR would be to create a workflow to clean up these base branches when there’s no longer pull requests associated with them?

[walterddr]

The "Pin base branch" workflow adds that label automatically, which is why it requires a special GITHUB_BASE_BRANCH_TOKEN separate from the standard GITHUB_TOKEN (otherwise the followup pull_request event wouldn't trigger another workflow).

Hmm. I guess i didn't express my question correctly. let me rephrase. as you mentioned,

1. this workflow only runs on master or base/*,
2. this workflow automatically adds the base pinned tags.
3. ghstack bases are always gh/*.

and I further assume based pinned tag is important to trigger other GHA workflows in the future

My question is,

1. since ghstack PRs are based off gh/* base branches, this workflow doesn't automatically add the base pinned tags to those ghstack PRs, is that correct?
2. if Q1 is correct, do we expect ghstack PR authors to add them manually?

[walterddr]

Perhaps a good follow up to this PR would be to create a workflow to clean up these base branches when there’s no longer pull requests associated with them?

yes. but I am not sure if base/x got updated to base/y, we can delete base/x? since base/x might still be used by other PRs?

[samestep]

yes. but I am not sure if base/x got updated to base/y, we can delete base/x? since base/x might still be used by other PRs?

Not necessarily, but there's probably a way to check and see whether or the branch is still being used by other PRs (not sure about race conditions though).

[samestep]

Hmm. I guess i didn't express my question correctly. let me rephrase. as you mentioned,

1. this workflow only runs on master or base/*,
2. this workflow automatically adds the base pinned tags.
3. ghstack bases are always gh/*.

and I further assume based pinned tag is important to trigger other GHA workflows in the future

My question is,

1. since ghstack PRs are based off gh/* base branches, this workflow doesn't automatically add the base pinned tags to those ghstack PRs, is that correct?
2. if Q1 is correct, do we expect ghstack PR authors to add them manually?

Ah, I see what you're saying. The answer is that the point of the "base pinned" tag is just to kick off CI, so we don't need it for ghstack PRs.

[facebook-github-bot]

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[samestep]

Closing this for now (in favor of #54967) because it just introduces too many quirks.