-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Change Review Process #3659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Change Review Process #3659
Changes from all commits
6b9a080
e7bf973
d8339e9
f5b8205
b78b767
bad9c26
9b4c2b8
8a9e108
529166f
f510396
d376362
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
|
|
||
| name: Force Draft PR | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened] | ||
| branches: | ||
| - main | ||
|
|
||
| permissions: | ||
| pull-requests: write | ||
|
|
||
| jobs: | ||
| force-draft: | ||
| runs-on: ubuntu-latest | ||
| if: ${{ !github.event.pull_request.draft && github.repository == 'NVIDIA/Megatron-LM' }} | ||
| steps: | ||
| - name: Convert PR to draft | ||
| env: | ||
| 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 }} | ||
|
Comment on lines
+21
to
+27
This comment was marked as resolved.
Sorry, something went wrong. |
||
| run: | | ||
| gh pr comment ${{ github.event.pull_request.number }} --repo ${{ github.repository }} --body \ | ||
| "This PR has been automatically converted to **draft** because all PRs must start as drafts. | ||
|
|
||
| When you are ready for review, click **Ready for Review** to begin the review process. This will: | ||
| 1. Add the oncall reviewer (optional reviewer) | ||
| 2. Add required review teams based on your changes | ||
|
|
||
| See the [contribution guide](https://github.com/NVIDIA/Megatron-LM/blob/main/docs/developer/submit.md) for more details." | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,13 +40,13 @@ Below is the checklist that the oncall needs to go through for each PR. | |
| - Do all tests pass? | ||
| - Oncall will need to kick off testing suite for external reviewers | ||
| - Comment “/ok to test commid_id” to kick off testing suite | ||
| - Add the “Expert Review” label | ||
| - Select an expert reviewer from each expert group as a reviewer. If you’re unsure who to select, pick a “maintainer” or manager. | ||
| - Expert reviewers are notified after the PR is marked “Ready for Review” | ||
| - **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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who is this note for? Maybe it’s obsolete nowadays?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I.e. how do we do that if we cannot reach out to everyone? Maybe via MCore-oncall?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the idea, the oncall is supposed to reach out directly to reviewer. |
||
| - For `megatron/core` PRs, the “Final Review” label is applied automatically once all expert reviewers approve | ||
| - Final reviewers should review within 1 business day. Message the assigned reviewer if it is taking longer. | ||
| - If the reviewer is not responding after 2 business days, escalate to the reviewer's manager. | ||
| - If the reviewer is not responding after 2 business days, escalate to the reviewer’s manager. | ||
| - The “Approved” label is applied automatically once all required reviewers have approved | ||
|
|
||
| ## Issues and Discussion Questions | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ class Reminder: | |
| class PRReviewTracker: | ||
| EXPERT_REVIEW = "Expert Review" | ||
| FINAL_REVIEW = "Final Review" | ||
| APPROVED = "Approved" | ||
| EXCLUDED_TEAMS = {"core-adlr", "core-nemo"} | ||
|
|
||
| def __init__( | ||
|
|
@@ -127,6 +128,11 @@ def get_label_date(self, pr, label: str): | |
| ] | ||
| return max(dates) if dates else None | ||
|
|
||
| def get_ready_for_review_date(self, pr): | ||
| """Get the date a PR was marked as ready for review.""" | ||
| dates = [e.created_at for e in pr.as_issue().get_events() if e.event == "ready_for_review"] | ||
| return max(dates) if dates else None | ||
|
|
||
| def days_since(self, date): | ||
| """Calculate days since given date.""" | ||
| if not date: | ||
|
|
@@ -138,11 +144,22 @@ def days_since(self, date): | |
| def get_stage(self, pr): | ||
| """Get current review stage.""" | ||
| labels = {l.name for l in pr.labels} | ||
| return self.FINAL_REVIEW if self.FINAL_REVIEW in labels else self.EXPERT_REVIEW | ||
| if self.APPROVED in labels: | ||
| return self.APPROVED | ||
| if self.FINAL_REVIEW in labels: | ||
| return self.FINAL_REVIEW | ||
| return self.EXPERT_REVIEW | ||
|
|
||
| def get_reviewers(self, pr): | ||
| """Get filtered reviewer emails who haven't approved yet.""" | ||
| stage = self.get_stage(pr) | ||
|
|
||
| if stage == self.APPROVED: | ||
| return ( | ||
| [self.get_user_email(pr.user.login)], | ||
| "All reviewers have approved. Please merge the PR.", | ||
| ) | ||
|
|
||
| org = self.github.get_organization(self.repo.organization.login) | ||
|
|
||
| # 1. Get the latest review state for everyone who has submitted a review | ||
|
|
@@ -207,17 +224,27 @@ def get_reviewers(self, pr): | |
| # 8. Handle the original edge cases | ||
| if len(reviewer_emails) == 0: | ||
| if stage == self.EXPERT_REVIEW: | ||
| # No reviewer activity yet — assignment hasn't completed (e.g. PR just became | ||
| # ready-for-review). Don't fire a spurious "all approved" message. | ||
| has_reviewer_activity = bool( | ||
| approvers | ||
| or non_approving_reviewers | ||
| or pending_individuals | ||
| or pending_teams_slugs | ||
| ) | ||
| if not has_reviewer_activity: | ||
| return [], "Waiting for reviewers to be assigned." | ||
| # 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This message fires whenever
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 [], "" |
||
| elif stage == self.FINAL_REVIEW: | ||
| # Assign to mcore-reviewers who approved | ||
| try: | ||
| mcore_team = org.get_team_by_slug("mcore-reviewers") | ||
| mcore_members = {m.login for m in mcore_team.get_members()} | ||
| valid_approvers = approvers & mcore_members | ||
| reviewer_emails = sorted([self.get_user_email(u) for u in valid_approvers]) | ||
| action_message = "All Final Reviewers approved the PR. Please ping an Expert or Final Reviewer to merge the PR." | ||
| action_message = "All Final Reviewers approved the PR. Please ping the @mcore-oncall to merge the PR." | ||
|
|
||
| except Exception as e: | ||
| logger.warning( | ||
|
|
@@ -229,7 +256,14 @@ def get_reviewers(self, pr): | |
| def create_reminder(self, pr): | ||
| """Create reminder for PR.""" | ||
| stage = self.get_stage(pr) | ||
| stage_days = self.days_since(self.get_label_date(pr, stage)) | ||
| ready_date = self.get_ready_for_review_date(pr) | ||
Phlip79 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if stage == self.EXPERT_REVIEW: | ||
| stage_days = self.days_since(ready_date) | ||
| elif stage in (self.FINAL_REVIEW, self.APPROVED): | ||
| stage_days = self.days_since(self.get_label_date(pr, stage)) | ||
| else: | ||
| stage_days = 0 | ||
| total_review_days = self.days_since(ready_date) | ||
| author_email = self.get_user_email(pr.user.login) | ||
| reviewer_emails, action_message = self.get_reviewers(pr) | ||
| escaped_title = html.escape(pr.title, quote=False) | ||
|
|
@@ -241,7 +275,7 @@ def create_reminder(self, pr): | |
| author=self.get_slack_user_id(author_email), | ||
| priority="P0" if stage_days > 3 else "P1" if stage_days >= 1 else "P2", | ||
| review_stage=stage, | ||
| total_review_time=self.days_since(self.get_label_date(pr, self.EXPERT_REVIEW)), | ||
| total_review_time=total_review_days, | ||
| current_stage_time=stage_days, | ||
| reviewers=[self.get_slack_user_id(email) for email in reviewer_emails], | ||
| action_message=action_message, | ||
|
|
@@ -256,12 +290,11 @@ def generate_reminders(self): | |
|
|
||
| reminders = [] | ||
| for milestone in milestones: | ||
| # Find issues with the 'Expert Review' or 'Final Review' label | ||
| # Find all open non-draft PRs with this milestone | ||
| query = ( | ||
| f'repo:"{self.repo.full_name}" ' | ||
| f'milestone:"{milestone.title}" ' | ||
| f'is:open is:pr -is:draft ' | ||
| f'label:"{self.EXPERT_REVIEW}","{self.FINAL_REVIEW}"' | ||
| f'is:open is:pr -is:draft' | ||
| ) | ||
| try: | ||
| # Use search_issues for a more direct query instead of get_issues + filtering | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.