In order to improve the round-trip times as well as the user experience of filing Pull Requests against MXNet, this process could be monitored and assisted by a bot which takes care of finding appropriate reviewers, reminding them and also generating statistics to find areas of improvement. This design is inspired by the Kubernetes GitHub community.
| Table of Contents |
|---|
Link to dev List discussion
...
The MXNet community faces a high round-trip due to the big volume of Pull Requests paired with the lack of reviewers. This creates frustration on the contributors' side since they get no or late feedback in a majority of the cases. The lack of reviewers is partially caused by the fact that people are not aware of Pull Requests that are in their area of expertise and non-committers being unable to use the CODEOWNERS feature to be notified. Frequent reviewers on the other hand don't get rewarded or incentivised for their activity since there's no active metric that tracks these endeavours.
...
Whenever a new Pull Request gets created, it gets set to "Work in progress" (WIP). As soon as the contributor considers their PR final and ready for review, they issue a command to move it to the triage stage. This stage is like the "first level support" which addresses general topics. After the initial review is done, the subject matter expert (SME) is engaged to give a more thorough review. In case the subject matter expert was not a committer, an additional committer review gets requested in order to get them to merge the PR after their quick review.
Whenever a review was given, the PR moves into the "Requested changes" state to signal the contributor that there is outstanding feedback as well as to show the reviewers that there's no action required from their side. As soon as the feedback has been processed, the contributor can issue a command to move it back into the "Ready for review" state.
The bot determines who to ask for review in every stage through a config-file in the repository. In there, people can add themselves in the same style as the CODEOWNERS file to
Whenever a new Pull Request gets created, the bot analyses the changed files in the same style as the CODEOWNERS feature. By having a configuration file in the MXNet repository, we can assign certain folders and files to certain people. The configuration would allow the following variants of responsibility:
MXNet Java Inference API#ProposedApproach
MXNet Java Inference API#ClassDiagram
MXNet Java Inference API#SequenceDiagram
UserExperience
Goals/Usecases
MXNet Java Inference API#Goals
Open Questions
Addition of New APIs
Backward compatibility
Performance Considerations
Test Plan
Alternative Approaches
MXNet Scala API Usability Improvement#AlternativeApproachconsidered
Technical Challenges
MXNet Scala API Usability Improvement#TechnicalChallenges
Milestones
...
In order to determine the appropriate reviewer, we can start categorizing the source directories into modules as follows (using a config files in the root dir):
| Code Block | ||
|---|---|---|
| ||
/julia: Julia
/src/operator: Operator
/src: Backend
/ci: CI
/docker: Docker
/src/operator/nn/mkldnn: MKL |
In another configuration file, a contributor could then define themselves as subject matter experts for specific modules in the following style:
| Code Block | ||
|---|---|---|
| ||
marcoabreu@:
- CI
- Docker
- CD
- Jenkins
elephantmaster9000@:
- Operator
- CUDA
- Backend |
The last configuration file will contain people who are ready to review the first triaging stage as well as the last optional committer stage. Since not all committers are still active, we maintain an explicit opt-in list:
| Code Block | ||
|---|---|---|
| ||
Triage:
- marcoabreu@
Committer:
- marcoabreu@
- elephantmaster9000@ |
When a PR left the WIP state for the first time, the system will pick a few (3?) contributors who signed up as triage reviewers and inform them. Once the PR passed the triage stage, it will use the modules to find the most appropriate SMEs (maybe 2?) while also load-balancing a bit to avoid the same people being constantly engaged and overloading them. If the PR enters the committer stage, it will use the same logic as during the triage stage to find committers.
In case somebody would like to request a review from a specific person, they can issue the bot to manually add a certain reviewer using "/assign marcoabreu@".
To recognize people for their reviews, we can create a summary on a periodic base to call out the TOP X reviewers on dev@ together with average statistics like processing times of various stages and modules.
UserExperience
As soon as a user opens a Pull Request, a bot will write a comment which will be used as a kind of sticky post that will be updated as the PR progresses through the stages. It will contain usage instructions as well assigned reviewers. The first action is to place the Pull Request in the "Work in Progress" state.
Once the PR is ready, the user writes "/pr-status ready" which will move it into the "Triage review" stage, assign the "Ready for review" state and update the sticky post with the name of the Triage reviewers. Once a review has been submitted, the PR will be placed in the "Requested changes" state. The user can write "/status ready" to place it back in the "Ready for review" state.
As soon as a triage reviewer has given their approval, the PR will automatically move into the "SME review" stage, assign the "Ready for review" state and update the sticky post with the name of the SME reviewers. Same process is applied here.
As soon as the SME reviewer has given their approval, the PR will automatically move into the "Committer review" stage, assign the "Ready to merge" state and updated the sticky post with the name of the committers while also using the GitHub feature to assign directly. If they would like to wait for another SMEs review, they can move it back by issuing "/request-review sme"
In case no triage is required, users can issue the command "/skip-reviewer triage". In case no SME is required, users can issue the command "/skip-reviewer sme".
In case a PR sits in the "Ready for review" state for more than 3 business days, they will be reminded and the system adds more reviewers. After 7 business days, it will receive the "lifecycle:lonely" state and the same action as after 3 days is executed. This can be reset by issuing "/lifecycle remove lonely". If a PR sits in the "Ready for review" state for more than 10 business days, it will be marked as "lifecycle:forgotten" and the escalation path is engaged: All committers are being messaged and an email is sent to dev@.
In case a PR sits in the "Work in progress" state for more than 10 business days, it will be marked as "stale". This can be removed by issuing the command "/lifecycle remove stale". If a PR sits in the "lifecycle:stale" state for more than 10 business days, it will be closed automatically, marked with the "lifecycle:rotten" state and the bot will issue an explaining comment. A user can reopen the PR by issuing the command "/lifecycle remove rotten" (a PR cannot be reopened by a user after being closed by a committer, hence this step).
Goals/Usecases
This proposal attempts to solve the following use-cases:
- Being unable to assign PRs to non-committers: Bot holds its own list
- Trouble finding appropriate reviewer: Subject matter expert config
- Reviewers not following up: Automatic reminders, increasing the number of reviewers, escalation to dev@
- PR authors abandoning their PRs: Automatic closing
- Lack of recognition for reviewing PRs: dev@ summary
- Lack of statistics to track round-trip times: dev@ summary
Open Questions
- Should SMEs always be assigned as reviewers as soon as their module is changed? Or is it better to switch between the available SMEs for each module?
- Is it sufficient if one SME has given their approval or can a PR only move forward if SMEs for every module gave their approval?
- Who should be able to decide whether a stage can be skipped? Only assigned reviewers and committers or everybody?
Future ideas
- CODEOWNERS evaluation: Process the CODEOWNERS file to allow people to always get notified in case a file changes. This does not mean that they have to review it; they are just informed.
- Size: Assign sizes (XS, S, M, L, XL) to PRs, depending on the number of changed files and lines to give the reviewers an estimate of the required effort.
References
Kubernetes Pull Requests: https://github.com/kubernetes/kubernetes/pulls