PyTorch ONNX Exporter Code Reviews and Duty Rotation - pytorch/pytorch GitHub Wiki

Author: garymiguel. Created: 2021-07-16

• Pull request review protocol
  • Authors
  • Reviewers
• Duty rotation
  • Mechanics
  • Rotation duties
  • Review new pull requests
  • Review previously assigned pull requests
  • GitHub issue triage
    • Triage new issues
    • Resolving an issue
  • Review "onnx-needs-info" items

Reviewers and authors should follow Google's Code Review Developer Guide.

Authors and reviewers should feel free to discuss things synchronously (in-person or Teams calls) to avoid a lot of back-and-forth.

PRs that are not ready for review should be marked as Draft so that the on-duty doesn't see them.

There are two options for an author to get a pull request reviewed:

1. Assign reviewer(s) of their choice in the GitHub UI, or
2. Wait for the person on duty to assign themselves.

Some good reasons an author may assign reviewers:

• That reviewer has much more context on the change than anybody else on the team.
• That reviewer is the only person with expertise in the code being changed.

If this PR is for an item whose work cannot proceed until the PR is merged, the author should move the item into the "In Review" column on the Kanban.

All team members should check in daily on all PRs that are assigned to them via https://github.com/pulls/assigned, notifications filtered by reason:assign is:unread, or email notifications (whichever UI they prefer).

If unable to review a ready-for-review PR on a given workday, assignees should comment on the PR saying when they expect to review it.

If an assigned reviewer thinks someone else would be a much better reviewer, or is needed to review a specific part of the PR, they should:

1. Review the PR as best they can. This will help the assigned reviewer learn about a new area.
2. Add a comment suggesting a different reviewer who can finish the review and approve and merge the change.

If the author does not work at Microsoft and has not responded to comments in over a week, add the label "onnx-needs-info", and un-assign yourself.

Before merging, pull requests should be approved by members of the PyTorch ONNX Exporter engineering team at Microsoft. Current Microsoft engineering team members include:

• Ganesan Ramalingam (@gramalingam)
• Xavier Dupré (@xadupre)
• Justin Chu (@justinchuby)
• Ti-Tai Wang (@AllenTiTaiWang)
• Shubham Bhokare (@shubhambhokare1)
• Wei-Sheng Chin (@wschin)

• Attend required meetings.
• Review previously assigned pull requests.
• Review new pull requests.
• GitHub issue triage.
• Review onnx-needs-info items.

• Use the queries below to find unassigned PRs.

  • onnx in title
  • module: onnx

• For each PR...

• Look over the PR briefly.

• If you feel competent to review it at least partially, assign yourself in the GitHub UI and review the PR.

• If you don't feel competent to review it entirely, add a comment explaining what parts you think should be reviewed by someone else. If you know who else should review it, assign that person in the GitHub UI.

• If the PR is against master and has been approved by someone on the PyTorch exporter team, ask someone from Facebook to import it. TODO: figure out the GitHub alias to mention.

• Check appendix for details on setting proper PR descriptions when merging.

Continue any previously assigned pull request reviews according to protocol.

• Use the queries below to find issues that need to be triaged.

  • no assignee

  • assignee:@me

  • onnxruntime

• For each issue...

  • Spend a reasonable amount of time (< 1 hour) to try to understand and reproduce the issue.

  • If possible, edit the title and/or body for clarity, to add repro instructions, etc.

  • If unable to understand or reproduce:

    • Add a comment, @mentioning the reporter and explain that you can't understand or reproduce.
    • Add the label "onnx-needs-info" (for issues in onnxruntime repo, use "status:more-info-needed").
    • Move on to the next issue.

  • If able to understand and reproduce...

    • Add the label "onnx-triaged"
    • If it seems like it will take less than 1 hour to resolve, then attempt to resolve it immediately. See resolving an issue.
    • If it seems like it will (or actually does) take more than 1 hour to resolve, don't do anything else.

If it can be resolved without changes to the PyTorch source code (e.g., it was just a question, or the reporter can change their code), then:

• Add a comment answering the question or explaining how the reporter can change their code. Mention the reporter (using "@<user name>").
• Add the label onnx-needs-info.

If changes to the PyTorch source code are required, in the message of the commit that fixes the issue write "fixes #". Once this commit is in the master branch, the GitHub issue should automatically be closed.

• Use this query and go through each item.
• If there's been a response from the author / reporter, remove the "onnx-needs-info" label and assign the issue / PR to the person who previously added that label.
• If there's been no response in 2 months, close it with the comment:
  • It's been over 2 months, so closing this. Feel free to open another one.