[RFC] Allow merging via PR comments

Development pre-RFC
#1

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. See examples:

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.

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.

2 Likes
Commit Message Guideline
#2

This sounds good to me. AFAIK, PyTorch also leverages this approach so it might not be that “non-standard” :stuck_out_tongue_winking_eye:

1 Like
#3

This sounds great! Thanks @driazati!!

#4

Overall I’m supportive. It looks like there is also prior art 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 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 @junrushao @tqchen @jroesch @kparzysz @manupa-arm @ramana-arm @Mousius

#5

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.

#6

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.

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.

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 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

Recent CI Improvements
#7

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 Commit Message Guideline work that @gromero is doing.

We also need a cooler bot name than tvm-bot to be competitive in the merge bot market though.

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.

Is there an “off-the-shelf” version here that works as we’d like? As @comaniac suggested, this is fairly common in other projects.

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.

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.

#8

@driazati Hi

As we discussed today on TVM Community Meeting, the second part of your proposal about taking the final commit message from the PR’s body, in my view, is a prerequisite for the Commit Message Guideline we’re discussing.

It seems the community likes / uses / is used to the additional commits (i.e. commits pushed to the branch AFTER/on top of the initial commit(s) used to create the PR, usually created as a consequence of the review process, when reviewers / maintainers request changes), which is fair, but because of Github’s squash-and-merge mechanism there is no way to land a good commit message since the additional commits will anyways get squashed and merged into the final commit message, ruining to a large extent the essence of the Commit Message Guideline (not even using the fixup! tag to make a linter, e.g. Gitlint, ignore the additional commits messages won’t help because Github will ignore them anyways).

Hence, thumb up from my side :slight_smile:

Btw @areusch raised a good point that changing the PR’s body will re-trigger the CI, but if I got it right there is probably way to set it off so if the author wants to do a last minute change in the PR’s body message (after the PR is reviewed and the CI is already green) that’s not a big deal (e.g. CI won’t re-trigger)?

2 Likes
#9

Discussion from the TVM Community Meeting today:

  • This is good since it’s the only way to enforce rules we define as part of Commit Message Guideline
    • We may want to run linters such as gitlint, we can do this as a part of the bot
  • Should we check the PR body/title in Jenkins rather than when a merge is requested?
    • This might be better since it is 1 less system for people to have to satisfy, though the message when the merge is requested might not be the same as when it was checked in Jenkins
  • Can reviewers tag other reviewers as required? e.g. @tvm-bot require review from @someone in a review
    • We can certainly add this and it fits with common development patterns where someone will review a PR but delegate other parts of the PR to experts
    • We may need a timeout on these requests in case a reviewer never responds
  • Will PRs require a review on the most recently pushed commit?
    • Requiring reviews on the most recent commit may require more committer engagement and disincentivize fix-up type changes
    • Maybe a good middle ground is requiring an accept on the latest commit for PR authors that are not committers
    • GitHub has a setting to auto-dismiss stale reviews as well

This is a bug in how we set up Jenkins, I’m hoping it will be fixed with https://github.com/apache/tvm/pull/10778 along with some config changes (so only pushing new commits would trigger CI)

1 Like