Change Review Process#3659
Conversation
| - **Expert reviewers should review within 1 business day.** Message the assigned reviewer if it is taking longer. The reviewer either needs to review the PR or suggest an alternate reviewer. | ||
| - If the reviewer is not responding after 2 business days, escalate to the reviewer's manager. | ||
| - Add the “Final Review” label after experts approve | ||
| - If the reviewer is not responding after 2 business days, escalate to the reviewer’s manager. |
There was a problem hiding this comment.
Who is this note for? Maybe it’s obsolete nowadays?
There was a problem hiding this comment.
I.e. how do we do that if we cannot reach out to everyone? Maybe via MCore-oncall?
There was a problem hiding this comment.
That's the idea, the oncall is supposed to reach out directly to reviewer.
|
/claude review |
|
/claude review |
| GH_TOKEN: ${{ secrets.PAT }} | ||
| run: | | ||
| gh pr ready --undo ${{ github.event.pull_request.number }} --repo ${{ github.repository }} | ||
|
|
||
| - name: Add comment explaining draft policy | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
/claude review |
|
/ok to test f5b8205 |
|
/claude review |
|
/claude review |
|
Code Review Bug (inline comment posted): TypeError in auto_reminder_github.py See inline comment on line 253. days_since(ready_date) returns None for pre-existing non-draft PRs that lack a ready_for_review event, and the subsequent "stage_days > 3" comparison raises TypeError in Python 3. The new label-agnostic query (is:open is:pr -is:draft) returns PRs that the old label-filtered query never would, exposing this path during migration. Potential gap: milestone checkbox removed from PR template The "I want this PR in a versioned release and have added the appropriate Milestone" checklist item was removed from pull_request_template.md. The auto-reminder bot queries PRs by milestone -- PRs without a milestone are silently skipped. If milestone assignment is now handled another way that is fine, but worth confirming PRs cannot fall through reminder coverage. Missing tests for refactored swap_pr_labels.py The label-swap logic was significantly rewritten into a 3-stage state machine (_handle_expert_review_stage, _handle_final_review_stage, _handle_approved_stage). No tests were added or updated. Given the edge cases (revert paths, race-condition guard, needs_final_review logic), unit tests covering the main state transitions would help catch regressions. |
…ip79/Megatron-LM into philip/change-review-process
|
/claude review |
| # Assign to PR author | ||
| reviewer_emails = [self.get_user_email(pr.user.login)] | ||
| action_message = "All Expert Reviewers approved the PR. Please attach the Final Review label to proceed with the review." | ||
| action_message = "All Expert Reviewers have approved the PR." |
There was a problem hiding this comment.
This message fires whenever pending_reviewers is empty in the EXPERT_REVIEW stage. With the query now including all non-draft milestone PRs (not just labeled ones), this will also trigger for a PR that just became ready-for-review before CODEOWNERS/oncall assignment completes — resulting in a spurious "All Expert Reviewers have approved" message to the author.
swap_pr_labels.py already handles this race condition with a has_any_reviewers guard (step 8). A similar guard is needed here, e.g.:
if not (pending_individuals or pending_teams_slugs or approvers or non_approving_reviewers):
logger.info(f"PR #{pr.number} has no reviewers yet. Skipping.")
return [], ""|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/22700334343 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/22703252875 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/22705408467 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/22709214191 |
What does this PR do ?
Changing the review process to:
Before this PR is merged in I will change all open PRs that do not have the "Expert Review" or "Final Review" process to draft PRs.
Contribution process
flowchart LR A[Pre-checks] --> B[PR Tests] subgraph Code Review/Approval C1[Expert Review] --> C2[Final Review] end B --> C1 C2 --> D[Merge]Pre-checks
Core 0.8)Code review
The following process is enforced via the CODEOWNERS file for changes into
megatron/core. For changes outside ofmegatron/core, it is up to the PR author whether or not to tag the Final Reviewer team.For MRs into `main` branch
Feel free to message or comment the @mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
(Step 1): Add PR label
Expert Review(Step 2): Collect the expert reviewers reviews
Expert Reviewlabel when your PR is ready for review.Final Review might get declined if these requirements are not fulfilled.
(Step 3): Final Review
Final Reviewlabel(Optional Step 4): Cherry-pick into release branch
If this PR also needs to be merged into
core_r*release branches, after this PR has been merged, selectCherry-pickto open a new PR into the release branch.For MRs into `dev` branch
The proposed review process for `dev` branch is under active discussion.MRs are mergable after one approval by either
eharper@nvidia.comorzijiey@nvidia.com.Merging your PR
Any member of core-adlr and
core-nemowill be able to merge your PR.