You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tvm.apache.org by driazati via Apache TVM Discuss <no...@discuss.tvm.ai> on 2022/03/02 21:34:44 UTC
[Apache TVM Discuss] [Development/pre-RFC] Allow merging via PR comments
# [RFC] Merge Bot
## Summary
PRs authors must currently wait for changes to be manually merged by a committer clicking the "merge" button. A large amount of time can often elapse between CI going green, a PR being approved, and this merge button being pressed. This RFC proposes a safe, self-service mechanism to allow PR authors to merge their own PRs. This mechanism will also ensure better commit messages than GitHub's UI provides.
## Motivation
The PR process currently looks something like this:
1. Develop and submit code
2. Request and wait for reviews
3. Iterate and address comments
4. Wait for CI
5. Ask a committer to merge the PR
This RFC addresses steps 4 and 5. It is commonplace for TVM developers to have to ask a committer to merge a PR that has already been approved by someone else. Imagine if a simple PR was submitted and a reviewer accepts it minutes after it was posted. A committer will have to revisit this PR in the future, requiring another sync and more time out of their day, to mindlessly merge the PR (since it has already been accepted). It has also happened in the course of development that PRs that need to be monitored after merging (especially CI infrastructure changes) are merged at inopportune times for the author. Allowing users who are not committers to merge their own PRs solves this issue.
TVM's commit messages are also poor. This is due to the fact that GitHub’s squash-and-merge strategy uses a commit message made by concatenating the first lines of all commits in a PR’s branch. These commits are often named irrelevant or nonsense things which do not belong in the history. Meanwhile, the first message in a PR thread often has a thoughtful and descriptive message which is usually more appropriate. This gets lost behind a level of indirection and is unavailable without a connection to GitHub. GitHub has no solution for this as can be seen in [this thread](https://github.community/t/how-to-change-the-default-squash-merge-commit-message/1155). See examples:
- [https://github.com/apache/tvm/commit/dace8b72e3a8173704a689a7074df4c6f36bda4a](https://github.com/apache/tvm/commit/dace8b72e3a8173704a689a7074df4c6f36bda4a)
- [https://github.com/apache/tvm/commit/73cf51b8246f1f0b5c0bff6fa206d154e053199b](https://github.com/apache/tvm/commit/73cf51b8246f1f0b5c0bff6fa206d154e053199b)
## Reference
A bot implemented as a GitHub Actions workflow will listen for comments on PRs that match a pattern similar to:
```
@tvm-bot merge this
```
It will then check that the PR has been accepted by a valid reviewer / committer in its latest version. If so, it will rebase the PR on `main`, using the GitHub PR's body and title as the commit message and `push` to the `main` branch using a committer API key. If this fails (perhaps due to a push to `main` that happened in the meantime), it will re-attempt until successful. It will then leave a comment on the PR:
```
This PR has been merged with commit <commit short hash>.
```
or relevant information for debugging and who to contact if failed. The merge button on PRs could be effectively disabled by restricting the `main` branch to admin-only access.
## Prior Art
PyTorch does this
## Disadvantages
- This would be a non-standard flow, adding another piece of overhead to the TVM PR process that is outside of the typical process one would expect on GitHub.
- Something to maintain - this would be another piece of automation for us to build. Given that we have dedicated resources for the CI though, this is manageable.
## Alternatives
- Do nothing - continue with bad commit messages and slow PR merging. Maintaining the status quo is certainly an option, but given the pains felt by PR authors something probably needs to happen.
- Make more people committers - This should happen anyway given the rising number of TVM contributors but there will always be people that aren't. In addition, the commit message issue persists even if people can merge their own PRs on GitHub.
---
[Visit Topic](https://discuss.tvm.apache.org/t/allow-merging-via-pr-comments/12220/1) to respond.
You are receiving this because you enabled mailing list mode.
To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/64440d86be8d618dd8dc598086a6e899083a4db5de604288c629f5ad7770cdea).
[Apache TVM Discuss] [Development/pre-RFC] Allow merging via PR comments
Posted by "Cody H. Yu via Apache TVM Discuss" <no...@discuss.tvm.ai>.
This sounds good to me. AFAIK, PyTorch also leverages this approach so it might not be that "non-standard" 😜
---
[Visit Topic](https://discuss.tvm.apache.org/t/allow-merging-via-pr-comments/12220/2) to respond.
You are receiving this because you enabled mailing list mode.
To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/b1e28401943017a326dce668360d6fd18c56f24ca2fdd4ec170ab91479939e06).
[Apache TVM Discuss] [Development/pre-RFC] [RFC] Allow merging via PR comments
Posted by Mehrdad Hessar via Apache TVM Discuss <no...@discuss.tvm.ai>.
This sounds great! Thanks @driazati!!
---
[Visit Topic](https://discuss.tvm.apache.org/t/rfc-allow-merging-via-pr-comments/12220/3) to respond.
You are receiving this because you enabled mailing list mode.
To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/fb9bc8d8506ef55e0a3949301c174877dff351a81690983f7bbaf1aeee70b620).
[Apache TVM Discuss] [Development/pre-RFC] [RFC] Allow merging via PR comments
Posted by Christopher Sidebottom via Apache TVM Discuss <no...@discuss.tvm.ai>.
Hi @driazati,
I think this is a great improvement to help empower more people to contribute without having to synchronise with those with certain powers in the project across many timezones and organisations, as well as providing some much needed consistency on commit messages. Could this implement some simple lint rules for commit messages as well to try to support better overall messages? Considering the new https://discuss.tvm.apache.org/t/commit-message-guideline/12334 work that @gromero is doing.
We also need a cooler bot name than `tvm-bot` to be competitive in the merge bot market though.
[quote="driazati, post:1, topic:12220"]
It will then check that the PR has been accepted by a valid reviewer / committer in its latest version.
[/quote]
I *think* this would be just Committers who could give a "merge-able" approval in the ASF? Reviewers would continue to provide assistance to Committers though, potentially future bot commands could help with highlighting that.
[quote="driazati, post:1, topic:12220"]
* Something to maintain - this would be another piece of automation for us to build. Given that we have dedicated resources for the CI though, this is manageable.
[/quote]
Is there an "off-the-shelf" version here that works as we'd like? As @comaniac suggested, this is fairly common in other projects.
[quote="driazati, post:6, topic:12220"]
[quote="areusch, post:4, topic:12220"]
committers have reviewed and one wants to approve the PR but not merge it until the others have looked the PR over.
[/quote]
One of the main reasons of this work would be to explicitly remove this step, though it makes sense to still limit the ability to those that have already interacted with the repo in some way (i.e. they aren’t a first time contributor).
[/quote]
Isn't the issue here that we want a PR to be active for a certain period where people can review it before it being merged due to some people working across different timezones etc? I'd prefer something minimal so as to encourage progress rather than creating new ways to hold up PRs, the Committer takes responsibility for approval either way. We can always follow a PR with another PR using the same bot :smile_cat:
I also don't think we should limit who can ask for a merge given it'll only function if the last commit has a positive review from a Committer - they'll have to start the workflows anyway for a new person in the repo.
[quote="driazati, post:6, topic:12220"]
[quote="areusch, post:4, topic:12220"]
Suppose we want to approve but hold a PR until another PR lands
[/quote]
This is something we just need to rely on the community for, with mostly good faith actors this workflow would work fine with comments like `accepted but please don't merge until #1234 merges` to keep the bot simple and understandable.
[/quote]
This should be an edge case, given patches should be self-contained and work in isolation; we should aim to encourage that level of discipline :smile_cat: If necessary, we should default fallback to human interaction to resolve these more complex scenarios.
---
[Visit Topic](https://discuss.tvm.apache.org/t/rfc-allow-merging-via-pr-comments/12220/7) to respond.
You are receiving this because you enabled mailing list mode.
To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/f436ba706b6fc13a7ba3eed212f20394106a31e49674b58b36006e3392e22384).
[Apache TVM Discuss] [Development/pre-RFC] [RFC] Allow merging via PR comments
Posted by driazati via Apache TVM Discuss <no...@discuss.tvm.ai>.
[quote="areusch, post:4, topic:12220"]
committers have reviewed and one wants to approve the PR but not merge it until the others have looked the PR over.
[/quote]
One of the main reasons of this work would be to explicitly remove this step, though it makes sense to still limit the ability to those that have already interacted with the repo in some way (i.e. they aren't a first time contributor). We could have a way for reviewers to add other required reviewers which would solve this too. Either way even if it's just committers to start with that lets us fix our commit messages which by itself is a big improvement.
[quote="areusch, post:4, topic:12220"]
Suppose we want to approve but hold a PR until another PR lands
[/quote]
This is something we just need to rely on the community for, with mostly good faith actors this workflow would work fine with comments like `accepted but please don't merge until #1234 merges` to keep the bot simple and understandable.
[quote="kparzysz, post:5, topic:12220"]
One question—what should happen if (1) PR is approved and looks non-controvesial, (2) author adds a merge directive, and then (3) some other reviewer requests changes? Should we expect the merge directive to be canceled? I think we should consider what is actually possible to implement in automation, maybe there is no way to handle this scenario
[/quote]
The time from acceptance to merge should be very short (like < 1 min), so this would be pretty rare. In any case, reverts are [quick and simple](https://github.com/apache/tvm/blob/main/docs/contribute/ci.rst#skip-ci-for-reverts) and if a committer has a good reason they should not hesitate to bounce something back to the author. A cancel mechanism is certainly possible but not worth it from a complexity perspective IMO
---
[Visit Topic](https://discuss.tvm.apache.org/t/rfc-allow-merging-via-pr-comments/12220/6) to respond.
You are receiving this because you enabled mailing list mode.
To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/ef5ac42a64eafa320fe0046fc681923113dd3c16693fa2adb092bbdbfdc3a919).
[Apache TVM Discuss] [Development/pre-RFC] [RFC] Allow merging via PR comments
Posted by Krzysztof Parzyszek via Apache TVM Discuss <no...@discuss.tvm.ai>.
I think this is a good idea. In my opinion it would be authors' responsibility not to abuse the new ability: the merge directive should be added only once the PR has reached an agreement. That is a subjective judgment, but it would be hard to codify it.
One question---what should happen if (1) PR is approved and looks non-controvesial, (2) author adds a merge directive, and then (3) some other reviewer requests changes? Should we expect the merge directive to be canceled? I think we should consider what is actually possible to implement in automation, maybe there is no way to handle this scenario.
---
[Visit Topic](https://discuss.tvm.apache.org/t/rfc-allow-merging-via-pr-comments/12220/5) to respond.
You are receiving this because you enabled mailing list mode.
To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/7978914dd3eb5071717f2b42c4c53889d6661748afe767b007386ee70eb639da).
[Apache TVM Discuss] [Development/pre-RFC] [RFC] Allow merging via PR comments
Posted by Andrew Reusch via Apache TVM Discuss <no...@discuss.tvm.ai>.
Overall I'm supportive. It looks like there is also [prior art](https://github.com/jasonkuster/merge-bot) for an ASF project to do something like this (although it's not clear how that bot is run). There is also some mention of [preserving provenance](https://www.mail-archive.com/search?l=dev@community.apache.org&q=subject:%22Re%5C%3A+ASF+wide+policy+on+github%27s+squash+and+merge%5C%3F%22&o=newest&f=1) on the ASF mail archives (e.g. suppose the bot squashes and merges a branch of commits, but not all commits in the branch were authored or committed by the GH contributor).
There is also a question in my mind about who should be allowed to command the bot to merge. The prior art restricts this capability to committers only. I realize this seems like a bit of an annoyance...there are a couple reasons why I could see this being useful:
- Suppose multiple committers have reviewed and one wants to approve the PR but not merge it until the others have looked the PR over. Traditionally we'd do this with a PR comment, but a merge bot couldn't distinguish the difference in this scenario.
- Suppose we want to approve but hold a PR until another PR lands. In this scenario I usually just comment LGTM but don't explicitly approve, so people could just get used to that. But it means people do need to be a bit more intentional about approving things.
Would be great to get some other feedback from the community about this.
cc @junrushao1994 @tqchen @jroesch @kparzysz @manupa-arm @ramana-arm @Mousius
---
[Visit Topic](https://discuss.tvm.apache.org/t/rfc-allow-merging-via-pr-comments/12220/4) to respond.
You are receiving this because you enabled mailing list mode.
To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/59c3432be33bb9e3573791da42ad0dde419dcacff7f9f67331e5c198c2db3956).