Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Script to ghstack land #6270

Closed
wants to merge 1 commit into from
Closed

Script to ghstack land #6270

wants to merge 1 commit into from

Conversation

kirklandsign
Copy link
Contributor

@kirklandsign kirklandsign commented Oct 16, 2024

Per ghstack tutorial for a PR exported by ghstack, it will have two diff sets:

  • gh/user/1/base <- gh/user/1/head where the PR is created like this
  • main <- gh/user/1/orig

The purpose of this bot is, when the ghstack PR is merged, automatically create another PR to do this merge main <- gh/user/1/orig. Then we just merge the newly created PR so that main has that change.

Missing piece: token from pytorch bot to create PR.

If this goes well, we can either git merge the commit from gh/user/1/orig into main directly, without going through the new PR; or auto approve and merge the PR.

Test: You can test locally with export GITHUB_TOKEN=ghpxyz; python .github/scripts/propose_ghstack_orig_pr.py --pr 6265 --repo pytorch/executorch
Note that between /orig merges, there is never a merge conflict.

The invariant is guaranteed that gh/user/1/orig contains the exact same change between gh/user/1/base <- gh/user/1/head by ghstack tool.

Copy link

pytorch-bot bot commented Oct 16, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 3410698 with merge base 7510f8c (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 16, 2024
@kirklandsign kirklandsign force-pushed the gh/kirklandsign/12/orig branch 6 times, most recently from 12571c1 to e92bf31 Compare October 16, 2024 06:32

python .github/scripts/propose_ghstack_orig_pr.py --pr $PR_NUMBER --repo pytorch/executorch
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huydhn could use your help with getting a bot token for creating the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huydhn Then let's still use GH_PYTORCHBOT_CHERRY_PICK_TOKEN, and add environment: cherry-pick-bot?

name: Propose to merge ghstack orig PRs to main
on:
pull_request:
types: [opened, synchronize, closed]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very cheap job.

@huydhn
Copy link
Contributor

huydhn commented Oct 16, 2024

I need to sit down to review this one, but let me ask @kit1980 to take a look too as granting pull request write permission to a pull_request target seems like a security hole. And he knows that part well.

@huydhn huydhn requested a review from kit1980 October 16, 2024 07:35
@kirklandsign
Copy link
Contributor Author

I need to sit down to review this one, but let me ask @kit1980 to take a look too as granting pull request write permission to a pull_request target seems like a security hole. And he knows that part well.

Thank you! It’s very similar to our cherry pick bot. However if I use cherry pick bot token it doesn’t allow me to create PR (maybe on this branch trigger). GH actions token can’t be used to create PR.

@huydhn
Copy link
Contributor

huydhn commented Oct 16, 2024

@kit1980 Let me hear what you think. From what I see, we could make the PR work as it stands because it only needs to grant write permission to ghstack PRs. And folks need to have write permission to the repo already to be able to use ghstack in the first place. So, there shouldn't be any security gap (if we implement the workflow correctly)


python .github/scripts/propose_ghstack_orig_pr.py --pr $PR_NUMBER --repo pytorch/executorch
env:
GITHUB_TOKEN: ${{ secrets.GH_PYTORCHBOT_CHERRY_PICK_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huydhn @kit1980 Can we accept gh/*/base to trigger the cherry pick bot?

Users already needs permission to push to pytorch:gh/*/base, so this won't grant additional permission

@kit1980
Copy link
Member

kit1980 commented Oct 17, 2024

I need to sit down to review this one, but let me ask @kit1980 to take a look too as granting pull request write permission to a pull_request target seems like a security hole. And he knows that part well.

I don't see the issue. Secrets are not passed to forked PRs on pull_request trigger, so it should be fine.

pull_request:
types: [opened, synchronize, closed]
branches:
- 'gh/*/[0-9]+/base'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you want to choose a smaller group of people to test this out first, you can list them here explicitly, i.e.

    branches:
      - 'gh/kirklandsign/[0-9]+/base'
      - 'gh/guangy10/[0-9]+/base'

It seems like a good idea to try it out first before rolling out to everyone.

@kirklandsign kirklandsign force-pushed the gh/kirklandsign/12/orig branch 7 times, most recently from c88dcc0 to 0734fc6 Compare October 17, 2024 22:27
ghstack-source-id: 441280b30010718a5440390f5de05133f872728f
Pull Request resolved: #6265
@facebook-github-bot
Copy link
Contributor

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

parser.add_argument(
"--pr",
type=int,
help="Number of the PR in the stack to check and create corresponding PR",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify this? Is this an index into the stack, or the PR number itself? And if it's an existing PR, why does it need to create another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the trigger PR. Example: #6346

The bot creates #6347 for merging into main

@facebook-github-bot
Copy link
Contributor

@kirklandsign merged this pull request in 18a7e65.

@kirklandsign kirklandsign deleted the gh/kirklandsign/12/orig branch October 17, 2024 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants