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

Bot to automatically generate cherry-picking PRs #8

Closed
brettcannon opened this Issue Dec 10, 2016 · 27 comments

Comments

Projects
None yet
8 participants
@brettcannon
Member

brettcannon commented Dec 10, 2016

As pulled from PEP 512:

Since the decision has been made to work with cherry-picks instead of forward merging of branches, it would be convenient to have a bot that would generate pull requests based on cherry-picking for any pull requests that affect multiple branches. The most likely design is a bot that monitors merged pull requests with key labels applied that delineate what branches the pull request should be cherry-picked into. The bot would then generate cherry-pick pull requests for each label and remove the labels as the pull requests are created (this allows for easy detection when automatic cherry-picking failed).

@soltysh

This comment has been minimized.

Show comment
Hide comment
@soltysh

soltysh Dec 21, 2016

Collaborator

I'm interested in helping with this one 😄

Collaborator

soltysh commented Dec 21, 2016

I'm interested in helping with this one 😄

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Dec 21, 2016

Member

Yeah, I think this will be one of those high-impact improvements that will definitely make PR acceptance more pleasurable compared to the hg workflow. I think this should be number 2 on the TODO list once #6 is dealt with (as I see the NEWS file being the stickiest point in accepting PRs and it also makes this issue easier by lowering potential merge conflicts).

Member

brettcannon commented Dec 21, 2016

Yeah, I think this will be one of those high-impact improvements that will definitely make PR acceptance more pleasurable compared to the hg workflow. I think this should be number 2 on the TODO list once #6 is dealt with (as I see the NEWS file being the stickiest point in accepting PRs and it also makes this issue easier by lowering potential merge conflicts).

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Dec 22, 2016

Member

Note that I already have a working bot (tested it on https://github.com/fayton/cpython), but I didn't had time to clean it up and make it open source in the past months.

My plan is to work on bug fixes for 3.5.3, 3.6.1 and python.org over the holiday. I can help you after mid-January if you had time to create a prototype during the holiday (or you can help me to clean up my bot if none of us can work on a prototype before mid-January -- I codenamed it "Duke Nukem Forever" :))

Member

berkerpeksag commented Dec 22, 2016

Note that I already have a working bot (tested it on https://github.com/fayton/cpython), but I didn't had time to clean it up and make it open source in the past months.

My plan is to work on bug fixes for 3.5.3, 3.6.1 and python.org over the holiday. I can help you after mid-January if you had time to create a prototype during the holiday (or you can help me to clean up my bot if none of us can work on a prototype before mid-January -- I codenamed it "Duke Nukem Forever" :))

@soltysh

This comment has been minimized.

Show comment
Hide comment
@soltysh

soltysh Dec 22, 2016

Collaborator

👍 @berkerpeksag we can sync and I'd love to help with that.

Collaborator

soltysh commented Dec 22, 2016

👍 @berkerpeksag we can sync and I'd love to help with that.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Feb 8, 2017

Member

Just an FYI, I've finally cleaned-up my bot and set up on my test repository. There is a missing feature though: it doesn't create a new pull request; it automatically pushes the cherry-picked commit into maintenance branches.

See fayton/cpython#2 for an example. Note that it also automatically fills the committer field by inspecting the "core developers" team created on the organization.

Member

berkerpeksag commented Feb 8, 2017

Just an FYI, I've finally cleaned-up my bot and set up on my test repository. There is a missing feature though: it doesn't create a new pull request; it automatically pushes the cherry-picked commit into maintenance branches.

See fayton/cpython#2 for an example. Note that it also automatically fills the committer field by inspecting the "core developers" team created on the organization.

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Feb 8, 2017

Member

What about if the cherry-pick fails either due to testing or the code not patching cleanly? And how do you keep track of what has (not) successfully been cherry-picked (I'm assuming some label on the original PR)?

Member

brettcannon commented Feb 8, 2017

What about if the cherry-pick fails either due to testing or the code not patching cleanly? And how do you keep track of what has (not) successfully been cherry-picked (I'm assuming some label on the original PR)?

@brettcannon brettcannon added the bot label Feb 8, 2017

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 8, 2017

Contributor

What about if the cherry-pick fails either due to testing or the code not patching cleanly?

The other case I have seen is auto backport of an already backport patch. So the 2 regular failure of one of my bot is either:

  • empty commit after cherry-picking.
  • conflict.
  • Other (remote commit disappeared in the meantime/ GitHub down/ API rate limited.).

Seeing that now with github you can edit in the UI, you could try to commit and push the conflict on another branch, and resolve it in the GitHub UI as well.

Contributor

Carreau commented Feb 8, 2017

What about if the cherry-pick fails either due to testing or the code not patching cleanly?

The other case I have seen is auto backport of an already backport patch. So the 2 regular failure of one of my bot is either:

  • empty commit after cherry-picking.
  • conflict.
  • Other (remote commit disappeared in the meantime/ GitHub down/ API rate limited.).

Seeing that now with github you can edit in the UI, you could try to commit and push the conflict on another branch, and resolve it in the GitHub UI as well.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Feb 9, 2017

Member

What about if the cherry-pick fails either due to testing or the code not patching cleanly?

It reverts to the current state of upstream branch and adds a comment to the original pull request.

And how do you keep track of what has (not) successfully been cherry-picked (I'm assuming some label on the original PR)?

I'm using a simple sqlite database to keep track of the state [1], but we can add a label to the original PR too.

[1] This way it's also easy to quit early if the commit has already been cherry-picked.

Member

berkerpeksag commented Feb 9, 2017

What about if the cherry-pick fails either due to testing or the code not patching cleanly?

It reverts to the current state of upstream branch and adds a comment to the original pull request.

And how do you keep track of what has (not) successfully been cherry-picked (I'm assuming some label on the original PR)?

I'm using a simple sqlite database to keep track of the state [1], but we can add a label to the original PR too.

[1] This way it's also easy to quit early if the commit has already been cherry-picked.

@WhyNotHugo

This comment has been minimized.

Show comment
Hide comment
@WhyNotHugo

WhyNotHugo Feb 10, 2017

Since the decision has been made to work with cherry-picks instead of forward merging of branches, it would be convenient to have a bot that would generate pull requests based on cherry-picking for any pull requests that affect multiple branches.

Out of curiosity, why was this decision taken? PEP-0512 doesn't seem to explain any of the benefits of doing this.

WhyNotHugo commented Feb 10, 2017

Since the decision has been made to work with cherry-picks instead of forward merging of branches, it would be convenient to have a bot that would generate pull requests based on cherry-picking for any pull requests that affect multiple branches.

Out of curiosity, why was this decision taken? PEP-0512 doesn't seem to explain any of the benefits of doing this.

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Feb 10, 2017

Member

@hobarrera couple of reasons. One is people instinctively create PRs against master on GitHub and merge-forward wouldn't like that. It also allows you to accept commits on master while any discussion about backporting happens. Finally it makes the 2.7 no longer the odd-ball branch that never merges anywhere.

Member

brettcannon commented Feb 10, 2017

@hobarrera couple of reasons. One is people instinctively create PRs against master on GitHub and merge-forward wouldn't like that. It also allows you to accept commits on master while any discussion about backporting happens. Finally it makes the 2.7 no longer the odd-ball branch that never merges anywhere.

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Feb 15, 2017

Member

Just so no one feels too much pressure for getting a bot up, we could also write a Python script to automate this locally as a hold-over. I would expect a CLI like backport --commit <hash> --versions [comma-separated Python versions] [--upstream <remote for python/cpython>] [--fork <remote to personal fork>]. This would do:

git fetch {upstream}
for version in versions:
    git branch {version} backport-{version}-{commit}
    if git cherry-pick ...:
        git push {fork} backport-{version}-{commit}
    else:
        print(f"cherry-pick for {version} failed", file=sys.stderr)

Could even get really fancy and if someone provides GitHub credentials have the PR also be automatically created.

Member

brettcannon commented Feb 15, 2017

Just so no one feels too much pressure for getting a bot up, we could also write a Python script to automate this locally as a hold-over. I would expect a CLI like backport --commit <hash> --versions [comma-separated Python versions] [--upstream <remote for python/cpython>] [--fork <remote to personal fork>]. This would do:

git fetch {upstream}
for version in versions:
    git branch {version} backport-{version}-{commit}
    if git cherry-pick ...:
        git push {fork} backport-{version}-{commit}
    else:
        print(f"cherry-pick for {version} failed", file=sys.stderr)

Could even get really fancy and if someone provides GitHub credentials have the PR also be automatically created.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 16, 2017

Contributor

https://github.com/minrk/ghpro may be of help:

backport-pr apply 4.x 1234

Made to work with merge commit, but I'm guessing that shouldn't be hard to change.

Contributor

Carreau commented Feb 16, 2017

https://github.com/minrk/ghpro may be of help:

backport-pr apply 4.x 1234

Made to work with merge commit, but I'm guessing that shouldn't be hard to change.

@ncoghlan

This comment has been minimized.

Show comment
Hide comment
@ncoghlan

ncoghlan Feb 18, 2017

Contributor

Having just done the backports for python/cpython#149 I don't think this will work very well until after Misc/NEWS is dealt with :)

Contributor

ncoghlan commented Feb 18, 2017

Having just done the backports for python/cpython#149 I don't think this will work very well until after Misc/NEWS is dealt with :)

@Mariatta

This comment has been minimized.

Show comment
Hide comment
@Mariatta

Mariatta Mar 5, 2017

Member

I started working on my own cherry-picking script 😛
It's here in case anyone wants to try it out: https://github.com/mariatta/chic_a_cherry_picker
It does not handle failures or merge conflicts :(
And ... I tested it in production 😅
python/cpython#474, python/cpython#475, and python/cpython#476 were cherry-picked using this script.

Member

Mariatta commented Mar 5, 2017

I started working on my own cherry-picking script 😛
It's here in case anyone wants to try it out: https://github.com/mariatta/chic_a_cherry_picker
It does not handle failures or merge conflicts :(
And ... I tested it in production 😅
python/cpython#474, python/cpython#475, and python/cpython#476 were cherry-picked using this script.

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Mar 6, 2017

Member

@Mariatta as long as it errors out when a cherry-pick fails due to merge conflicts I think that's acceptable (especially for simple cases like doc updates that don't touch Misc/NEWS).

Member

brettcannon commented Mar 6, 2017

@Mariatta as long as it errors out when a cherry-pick fails due to merge conflicts I think that's acceptable (especially for simple cases like doc updates that don't touch Misc/NEWS).

Mariatta added a commit that referenced this issue Mar 15, 2017

@Mariatta

This comment has been minimized.

Show comment
Hide comment
@Mariatta

Mariatta Apr 17, 2017

Member

Hi, I'm just starting to look into turning cherry_picker.py into a bot.
Currently, each of us push the backport branch into our own fork of CPython.
When implemented as a bot, where should the backport branch be pushed to?
Will it need to have it's own GitHub repo and account?
Thanks 😃

Member

Mariatta commented Apr 17, 2017

Hi, I'm just starting to look into turning cherry_picker.py into a bot.
Currently, each of us push the backport branch into our own fork of CPython.
When implemented as a bot, where should the backport branch be pushed to?
Will it need to have it's own GitHub repo and account?
Thanks 😃

@ncoghlan

This comment has been minimized.

Show comment
Hide comment
@ncoghlan

ncoghlan Apr 17, 2017

Contributor

The bot will definitely need its own account (as with the Knights and Sir Bedevere), but won't necessarily needs its own repo. Specifically, we have a free choice between two architectures:

  1. The bot has its own dedicated fork, and submits PRs from there
  2. The bot makes its working branches directly in the main CPython repo, and deletes them when the PR is merged

The main argument in favour of (1) is that it makes the bot's workflow consistent with the manual workflow we're currently using, which means we can share the automation scripts. It also has the advantage of continuing to keep PR working branches in the main repo to a minimum.

I don't actually see any strong arguments in favour of (2) - we can use that model ourselves as maintainers, but we don't, in order to keep our maintenance workflow as similar as we can to the external contributor workflow.

Contributor

ncoghlan commented Apr 17, 2017

The bot will definitely need its own account (as with the Knights and Sir Bedevere), but won't necessarily needs its own repo. Specifically, we have a free choice between two architectures:

  1. The bot has its own dedicated fork, and submits PRs from there
  2. The bot makes its working branches directly in the main CPython repo, and deletes them when the PR is merged

The main argument in favour of (1) is that it makes the bot's workflow consistent with the manual workflow we're currently using, which means we can share the automation scripts. It also has the advantage of continuing to keep PR working branches in the main repo to a minimum.

I don't actually see any strong arguments in favour of (2) - we can use that model ourselves as maintainers, but we don't, in order to keep our maintenance workflow as similar as we can to the external contributor workflow.

@ncoghlan

This comment has been minimized.

Show comment
Hide comment
@ncoghlan

ncoghlan Apr 17, 2017

Contributor

Naming Tangent: can we call the cherry picking bot Dennis?

Rationale: http://www.intriguing.com/mp/_scripts/peasant.php :)

Contributor

ncoghlan commented Apr 17, 2017

Naming Tangent: can we call the cherry picking bot Dennis?

Rationale: http://www.intriguing.com/mp/_scripts/peasant.php :)

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Apr 17, 2017

Member

It would be great if we all could stop working on our own bot at the same time.

Member

berkerpeksag commented Apr 17, 2017

It would be great if we all could stop working on our own bot at the same time.

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Apr 17, 2017

Member

@berkerpeksag @soltysh @Mariatta who has code for a bot and where is it?

Member

brettcannon commented Apr 17, 2017

@berkerpeksag @soltysh @Mariatta who has code for a bot and where is it?

@Mariatta

This comment has been minimized.

Show comment
Hide comment
@Mariatta

Mariatta Apr 17, 2017

Member

Sorry I haven't actually started working on any bot 🙁
Please continue on 😃

Member

Mariatta commented Apr 17, 2017

Sorry I haven't actually started working on any bot 🙁
Please continue on 😃

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Apr 25, 2017

Member

Sorry, I missed this earlier. I have code and a test repo on GitHub. I still need to publish the source code on GitHub, but I can only work on it weekends and I usually don't have a reliable internet connection on weekends. I hopefully will do it this weekend. Thanks for the ping!

Member

berkerpeksag commented Apr 25, 2017

Sorry, I missed this earlier. I have code and a test repo on GitHub. I still need to publish the source code on GitHub, but I can only work on it weekends and I usually don't have a reliable internet connection on weekends. I hopefully will do it this weekend. Thanks for the ping!

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 25, 2017

Contributor

I do have this which I need to convert to gitgethub, and we are using to backport for IPython/Jupyter.

Contributor

Carreau commented Apr 25, 2017

I do have this which I need to convert to gitgethub, and we are using to backport for IPython/Jupyter.

@Mariatta

This comment has been minimized.

Show comment
Hide comment
@Mariatta

Mariatta Sep 5, 2017

Member

Hi,

I created the repository to host the cherry-picker bot at https://github.com/python/miss-islington

The bot's account has also been created at https://github.com/miss-islington

I made an initial PR: python/miss-islington#1

According to Wikipedia, Miss Islington is the name of the witch in Monty Python and the Holy Grail sketch. Both Brett and Guido were consulted in choosing the name, and they gave me their +1s :)

The workflow:

  • PR that has "needs backport to X.Y" is merged
  • @miss-islington leaves a comment saying it will work on backporting it to X.Y
  • backport PR created
  • If there are errors, @miss-islington comments back saying it couldn't do the backport.

Some things to note:

  • In this workflow, @miss-islington will receive the credit for backporting the PR. In other words, @miss-islington will appear as a contributor to CPython. Misc/Acks entry is not needed.

  • What about CLA?

  • the "needs backport to X.Y" label must already been applied before you merge the PR on master.

  • if you don't want PRs to be automatically backported by @miss-islington, maybe don't apply the "needs backport to X.Y" label? Or only apply it after you merge? I think it's still useful to have this label to remind ourselves that this PR still needs to be backported somehow.

Any thoughts? Anyone up to reviewing the PR?

Member

Mariatta commented Sep 5, 2017

Hi,

I created the repository to host the cherry-picker bot at https://github.com/python/miss-islington

The bot's account has also been created at https://github.com/miss-islington

I made an initial PR: python/miss-islington#1

According to Wikipedia, Miss Islington is the name of the witch in Monty Python and the Holy Grail sketch. Both Brett and Guido were consulted in choosing the name, and they gave me their +1s :)

The workflow:

  • PR that has "needs backport to X.Y" is merged
  • @miss-islington leaves a comment saying it will work on backporting it to X.Y
  • backport PR created
  • If there are errors, @miss-islington comments back saying it couldn't do the backport.

Some things to note:

  • In this workflow, @miss-islington will receive the credit for backporting the PR. In other words, @miss-islington will appear as a contributor to CPython. Misc/Acks entry is not needed.

  • What about CLA?

  • the "needs backport to X.Y" label must already been applied before you merge the PR on master.

  • if you don't want PRs to be automatically backported by @miss-islington, maybe don't apply the "needs backport to X.Y" label? Or only apply it after you merge? I think it's still useful to have this label to remind ourselves that this PR still needs to be backported somehow.

Any thoughts? Anyone up to reviewing the PR?

@terryjreedy

This comment has been minimized.

Show comment
Hide comment
@terryjreedy

terryjreedy Sep 5, 2017

Member

Thank you for persisting with this. New 'autobackport 3.6' and 'autobackport 2.7' labels would a) let you merge the PR without being sure it is perfect, and b) let people volunteer to test it -- live -- or not. Having asked for it, I would. The only way I can 'review' the patch, beyond a cursory reading for apparent sanity, is to test it in action. As long as you are sure that the PR will not push anything to cpython, or otherwise do damage, and think it should work, I am willing to try it.

Member

terryjreedy commented Sep 5, 2017

Thank you for persisting with this. New 'autobackport 3.6' and 'autobackport 2.7' labels would a) let you merge the PR without being sure it is perfect, and b) let people volunteer to test it -- live -- or not. Having asked for it, I would. The only way I can 'review' the patch, beyond a cursory reading for apparent sanity, is to test it in action. As long as you are sure that the PR will not push anything to cpython, or otherwise do damage, and think it should work, I am willing to try it.

@Mariatta

This comment has been minimized.

Show comment
Hide comment
@Mariatta

Mariatta Sep 5, 2017

Member

Reopening the issue since the bot is not deployed yet 😅

Member

Mariatta commented Sep 5, 2017

Reopening the issue since the bot is not deployed yet 😅

@Mariatta Mariatta reopened this Sep 5, 2017

@Mariatta

This comment has been minimized.

Show comment
Hide comment
@Mariatta

Mariatta Sep 5, 2017

Member

@miss-islington made her first backport PR! python/cpython#3369 🌮 🎉
Closing this issue. Any further problem can be reported in https://github.com/python/miss-islington

Member

Mariatta commented Sep 5, 2017

@miss-islington made her first backport PR! python/cpython#3369 🌮 🎉
Closing this issue. Any further problem can be reported in https://github.com/python/miss-islington

@Mariatta Mariatta closed this Sep 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment