[Github] Action to Auto-assign PR Reviewers (that respects CODEOWNERS.md and overrides) #5630

venkywonka · 2025-07-01T01:15:58Z

venkywonka · 2025-07-02T06:08:02Z

.github/workflows/module-owners.json

picked up these module-owners mappings from this internal google sheet

CONTRIBUTING.md

.github/module-paths.json

.github/workflows/module-owners.json

.github/scripts/assign_reviewers.py

tburt-nv · 2025-07-02T16:14:55Z

.github/scripts/assign_reviewers.py

+        # Get team reviewers
+        team_result = subprocess.run(


For team reviewers, it would probably be helpful to randomly assign one reviewer in the team. That may also help automate the round-robin-style review balancing.

i believe the CODEOWNERS already does this tyler, my action does not deal with the github teams (although the same information is present in module-owners.json)

I don't think so: GitHub assigns the whole team as a reviewer when the team is in the CODEOWNERS list, and then GitHub optionally sends a email notification to the whole team when the team is assigned as a reviewer. I'm not aware of any built-in mechanism to send a notification to a single person in that team.

I think it might even be appropriate to replace each team reviewer with a single person in the team. The CODEOWNERS requirement would still allow any member of the team to approve.

We can leave that as a future improvement if you'd prefer, I'll approve this MR for now

Ah I didn't realize this setting existed; I suppose each team can decide how they want to balance their own reviews this way. The built-in teams probably would be sufficient instead of this action if we were able to mix required and not-required CODEOWNERS.

but imagine a scenario where the CODEOWNERS has very specific module mappings, each with a team/individual. If a PR touches multiple of these, then all these teams will get tagged as required codeowner approvals. It is possible that some module-mappings are so specific that its probably just 1-2 people. And they will effectivley block the approval and merge of the PR.
If the person that's randomly selected by the CODEOWNER routing algorithm happens to be OOTO, then i believe it will block the merge too as the PR author won't have the ability to remove such a person.

but the github action can alleviate that.
It does seem like a trade-off between getting specific module-owner mappings vs not imposing too many codeowner-approvals. Ideally a "coarse" CODEOWNERS and a "fine" github action would be the balance right?

the above assumes that the CODEOWNERS-enabled assignments are rigid (and hence blocking).

What do you think @tburt-nv ? also tagging @juney-nvidia @zeroepoch @kevinch-nv @atrifex @hlu1 @MartinMarciniszyn

If the person that's randomly selected by the CODEOWNER routing algorithm happens to be OOTO, then i believe it will block the merge too as the PR author won't have the ability to remove such a person.

Other members of the team in that case can still be manually assigned to the PR after the fact and their approval would unblock the PR.

It comes down to what the goals are for enforcing reviews and approvals.

If the goal is that module owners must explicitly approve every PR that touches their module, then using CODEOWNERs is the better solution since Github has built-in options requiring their approval, and we can guarantee the right individual or team is auto-assigned per PR.

If module owner approval is optional, then it makes sense to have this system in place to auto-assign a few reviewers for visibility on changes, as IIRC no default reviewers are assigned if the code changes are outside the limited modules covered in CODEOWNERs.

Sorry, I thought I had already resolved this thread. I'm personally ok with the flexibility of maintaining both the required CODEOWNERs with native GitHub features and the optional module-owners.json with this script. I only wish GitHub supported both natively.

Thanks @kevinch-nv / @tburt-nv .
@kevinch-nv , with regards to the 2 goals you detailed above, i believe we should lean towards (1) over (2).

So that means the module-owners must explicitly approve every PR (atleast 1 of them) that touches their module. This can potentially blow up the required approvals (like in this PR you mentioned), but that's okay/necessary.

BUT if, for any reason, none of the module owners of specific CODEOWNERS rule/mapping are available for approval, we'll need an unblocking mechanism.
And I believe that is an override-approval by an administrator. I think that's okay, because the likelihood that none of the owners inside a team is available is very low, and in such a case we anyway should not ignore it.

I should also correct my original comment - the auto-routing done per-team, when the team is pulled by the CODEOWNERS for a particular change, does respect OOTO/Busy statuses (here).

so, if the following things are done properly, CODEOWNERS + team settings are alone enough:

CODEOWNERS is fleshed out carefully, with the module-mappings in the right heirarchical order: This means more-specific paths come below less-specific ones. (/tensorrt_llm/_torch/, followed by /tensorrt_llm/_torch/models/ followed by /tensorrt_llm/_torch/models/modeling_qwen3.py )

Appropriate granularity of github teams need to be created, by expanding existing teams. For example, @NVIDIA/trt-llm-torch-devs maps to /tensorrt_llm/_torch, but we need another @NVIDIA/trt-llm-torch-custom-ops-devs created as child team to @NVIDIA/trt-llm-torch-devs. This means for the /tensorrt_llm/_torch/custom_ops files, @NVIDIA/trt-llm-torch-devs need not be spammed.

To avoid notifying all of the members of the coarser github teams (like @NVIDIA/trt-llm-torch-devs, ~23 members), we need to enable auto-routing and notifying only 1-2 people from that team (in a load-balanced way).

In mappings where one needs to notify an individual, but not necessarily be blocked by that person, one should include both the teams + individuals to take advantage of notifying reviewers in a specific + randomized way:

/tensorrt_llm/_torch/modules/fused_moe.py @hlu1 @dongxuy04 @zongfeijing @HuiGao-NV @NVIDIA/trt-llm-torch-devs

I have another draft PR #5977 that expands CODEOWNERS, but basically takes care of the ordering and heirarchy, but maintains the same coverage as @kevinch-nv 's reverted #4105 . I think that should be a good starting point.

venkywonka · 2025-07-03T22:47:27Z

/bot run

tensorrt-cicd · 2025-07-03T22:52:39Z

PR_Github #10878 [ run ] triggered by Bot

tensorrt-cicd · 2025-07-04T00:40:17Z

PR_Github #10878 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #8041 completed with status: 'FAILURE'

venkywonka · 2025-07-07T15:00:30Z

/bot run

tensorrt-cicd · 2025-07-07T15:05:54Z

PR_Github #11161 [ run ] triggered by Bot

tensorrt-cicd · 2025-07-07T17:43:58Z

PR_Github #11161 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #8254 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

kevinch-nv · 2025-07-08T17:04:45Z

.github/scripts/assign_reviewers.py

+
+        # Get modules and reviewers for module-mapped files
+        modules = get_modules_from_files(module_files, module_paths)
+        reviewers, modules_without_owners = get_reviewers_for_modules(


From my understanding of this code, all the reviewers for all the modules are placed in a set and randomly chosen.
If that's the case, it's possible that the final chosen reviewers do not span all the modules, thus leading to potential missed coverage.

yea that is correct, it is a risk because choosing a reviwer randomly per-module can be done but might exceed reviewer_limit if the modules touched are arbitrarily high - and that will anyways cause missed coverage.

i guess instead of having a coarse reviewer_limit, we can have a per-module reviewer_limit (maybe 2?) so we ensure module coverage and yet stick to an upper bound.

i can include that in this PR too, does the above make sense @kevinch-nv ?

These are just review assignments that doesn't block merge right? In that case can we make sure that at least one person from each module is assigned?

have resolved this in the latest push. now we have a per_module_reviewer_limit and ensures module coverage (that accounts for overlapping module-owners and overlapping modules)

have verified it works with this comment

Copilot

Pull Request Overview

This PR introduces a GitHub Action and supporting scripts to automatically assign PR reviewers based on module ownership, while respecting CODEOWNERS and manual overrides.

Adds assign_reviewers.py script with module-paths and module-owners JSON mappings
Defines a auto-assign-reviewers.yml workflow with dry-run and force-assign options
Updates documentation and adds end-to-end tests covering various assignment scenarios

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
CONTRIBUTING.md	New section describing the auto-assignment feature
.github/module-paths.json	New mapping of repository paths to logical modules
.github/module-owners.json	New mapping of modules to owner lists
.github/workflows/module-owners.json	Extended list of module owners for CI workflows
.github/workflows/auto-assign-reviewers.yml	Workflow definition for automatic reviewer assignment
.github/scripts/assign_reviewers.py	Script to detect changed files, map modules, and assign reviewers
.github/scripts/tests/test_assign_reviewers.py	Comprehensive tests for `assign_reviewers.py`

Comments suppressed due to low confidence (2)

.github/workflows/auto-assign-reviewers.yml:6

The workflow_dispatch inputs block does not include an input for pr_author, but the script relies on PR_AUTHOR. Consider adding a pr_author input or using github.actor to populate the PR author in manual dispatch.

    inputs:

CONTRIBUTING.md:108

The docs mention REVIEWER_LIMIT but the code and workflow use PER_MODULE_REVIEWER_LIMIT. Update the documentation to reference the correct environment variable name.

* **Limits reviewer count**: Randomly samples up to 3 reviewers if more are eligible (configurable via `REVIEWER_LIMIT`)

Copilot · 2025-07-08T21:48:09Z

.github/scripts/assign_reviewers.py

+
+            # Log if there were multiple matches (for debugging)
+            if len(matches) > 1:
+                matches[0][1]


This standalone expression is a no-op and appears to be leftover debugging code. It can be removed to improve readability.

Suggested change

matches[0][1]

venkywonka · 2025-07-09T00:40:07Z

/bot run

tensorrt-cicd · 2025-07-09T00:45:48Z

PR_Github #11359 [ run ] triggered by Bot

tensorrt-cicd · 2025-07-09T05:03:01Z

PR_Github #11359 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #8407 completed with status: 'SUCCESS'

poweiw

LGTM

tburt-nv · 2025-07-10T00:24:31Z

.github/scripts/assign_reviewers.py

+        # Get team reviewers
+        team_result = subprocess.run(


We can leave that as a future improvement if you'd prefer, I'll approve this MR for now

- Add logic to check existing reviewers before assignment - Skip auto-assignment if reviewers already exist (unless --force-assign) - Improve logging and error handling with status messages - Add force-assign option for manual override - Switch back to pull_request trigger for proper access - Fix workflow parameter handling for force_assign option Signed-off-by: Venky Ganesh <[email protected]>

Add comprehensive documentation about the GitHub action for automatic PR reviewer assignment, including its behavior with CODEOWNERS, module-based assignment, and existing reviewer respect. Signed-off-by: Venky Ganesh <[email protected]>

Signed-off-by: Venky Ganesh <[email protected]>

- Replace broad 'CI/CD' with 5 granular modules - Separate test concerns from pipeline logic - Move module-owners.json to .github/ for consistency Signed-off-by: Venky Ganesh <[email protected]>

…s and modules - Modified map_modules() to return both modules and unmapped files - Enhanced gather_reviewers() to track modules without owners - Added comprehensive feedback when no reviewers are assigned: - Warns about files with no module mapping - Warns about modules with no owners - Explains specific reasons for no assignment - Provides actionable guidance for fixing coverage gaps - Updated test cases to cover new functionality - Added test cases for unmapped files and modules without owners Signed-off-by: Venky Ganesh <[email protected]>

Signed-off-by: Venky Ganesh <[email protected]>

venkywonka · 2025-08-27T18:51:22Z

closing this as auto-assignment has been taken care of by CODEOWNERS itself.

venkywonka changed the base branch from main to user/venky/auto-assign-pr-test July 1, 2025 01:19

venkywonka changed the base branch from user/venky/auto-assign-pr-test to main July 1, 2025 01:19

venkywonka force-pushed the user/venky/auto-assign-pr-test branch from aa6184e to 0cc40e7 Compare July 2, 2025 02:42

venkywonka marked this pull request as ready for review July 2, 2025 02:43

venkywonka requested review from chzblych, Copilot, kaiyux, poweiw, syuoni, tburt-nv and yuanjingx87 July 2, 2025 02:43

This comment was marked as outdated.

Sign in to view

venkywonka commented Jul 2, 2025

View reviewed changes

venkywonka self-assigned this Jul 2, 2025

venkywonka added Infra <NV>automated tests, build checks, github actions, system stability & efficiency. Ease of Use Items about improving or complaints about TRTLLM ease of use labels Jul 2, 2025

tburt-nv reviewed Jul 2, 2025

View reviewed changes

venkywonka force-pushed the user/venky/auto-assign-pr-test branch from 0cc40e7 to 63339cd Compare July 3, 2025 17:14

venkywonka force-pushed the user/venky/auto-assign-pr-test branch from 87c4ea9 to c0e3ec0 Compare July 7, 2025 15:00

venkywonka requested review from MartinMarciniszyn and kevinch-nv July 8, 2025 01:50

kevinch-nv reviewed Jul 8, 2025

View reviewed changes

venkywonka force-pushed the user/venky/auto-assign-pr-test branch from c0e3ec0 to 2597ad5 Compare July 8, 2025 21:45

venkywonka requested a review from kevinch-nv July 8, 2025 21:47

venkywonka requested review from Copilot and tburt-nv July 8, 2025 21:47

Copilot AI reviewed Jul 8, 2025

View reviewed changes

poweiw approved these changes Jul 9, 2025

View reviewed changes

tburt-nv approved these changes Jul 10, 2025

View reviewed changes

venkywonka added 7 commits July 10, 2025 17:34

add comprehensive testing, change to pull_request_target

dd95efe

Signed-off-by: Venky Ganesh <[email protected]>

fix: Major module mapping overhaul for auto-assign reviewers

e435dc2

- Replace broad 'CI/CD' with 5 granular modules - Separate test concerns from pipeline logic - Move module-owners.json to .github/ for consistency Signed-off-by: Venky Ganesh <[email protected]>

fix 'first-match-wins' bug and test it

8c6f689

Signed-off-by: Venky Ganesh <[email protected]>

implement per-module reviewer assignment for better coverage, test it

bb53b5b

Signed-off-by: Venky Ganesh <[email protected]>

venkywonka force-pushed the user/venky/auto-assign-pr-test branch from 2597ad5 to bb53b5b Compare July 11, 2025 00:35

venkywonka closed this Aug 27, 2025

[Github] Action to Auto-assign PR Reviewers (that respects CODEOWNERS.md and overrides) #5630

[Github] Action to Auto-assign PR Reviewers (that respects CODEOWNERS.md and overrides) #5630

Conversation

venkywonka commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

venkywonka Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venkywonka Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinch-nv Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venkywonka Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venkywonka commented Jul 3, 2025

Uh oh!

tensorrt-cicd commented Jul 3, 2025

Uh oh!

tensorrt-cicd commented Jul 4, 2025

Uh oh!

venkywonka commented Jul 7, 2025

Uh oh!

tensorrt-cicd commented Jul 7, 2025

Uh oh!

tensorrt-cicd commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

venkywonka commented Jul 9, 2025

Uh oh!

tensorrt-cicd commented Jul 9, 2025

Uh oh!

tensorrt-cicd commented Jul 9, 2025

Uh oh!

poweiw left a comment

venkywonka commented Jul 1, 2025 •

edited

Loading

venkywonka Jul 2, 2025 •

edited

Loading

venkywonka Jul 11, 2025 •

edited

Loading

kevinch-nv Jul 11, 2025 •

edited

Loading

venkywonka Jul 12, 2025 •

edited

Loading