Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .github/module-owners.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"Generic Runtime": ["funatiq", "pcastonguay", "Shixiaowei02", "MartinMarciniszyn", "schetlur-nv", "dcampora"],
"Triton Backend": ["Tabrizian", "pcastonguay", "schetlur-nv"],
"LLM API/Workflow": ["Superjomn", "syuoni", "nv-guomingz", "litaotju", "QiJune"],
"KV-Cache Management":["thorjohnsen", "schetlur-nv"],
"Low Precision":["Tracin", "nv-guomingz", "Naveassaf"],
"Speculative Decoding":["yweng0828", "nekorobov", "lfr-0531"],
"Customized Kernels":["lowsfer", "PerkzZheng", "jdemouth-nvidia"],
"Performance": ["kaiyux", "jiahanc", "hypdeb"],
"Lora/P-tuning":["byshiue", "shaharmor98"],
"Disaggregated Serving":["Shixiaowei02", "joyang-nv", "chuangz0", "schetlur-nv"],
"Documentation":["nv-guomingz"],
"Sampling": ["dcampora", "lfr-0531", "Naveassaf", "syuoni", "yweng0828"],
"Memory": ["litaotju", "peaceh-nv"],
"Installation": ["hchings", "Superjomn", "nv-guomingz", "QiJune"],
"GitHub Configuration": ["tburt-nv", "niukuo"],
"Jenkins Pipelines": ["chzblych", "niukuo"],
"Test Configuration": ["niukuo", "syuoni", "LarryXFly"],
"Test Waive List": ["chzblych", "niukuo"],
"Integration Tests": ["LarryXFly", "niukuo"],
"Torch Framework": ["QiJune", "hlu1"],
"Torch Attention Backend": ["yuxianq", "hlu1"],
"Torch AutoDeploy": ["lucaslie", "suyoggupta"],
"Torch Compilation": ["litaotju", "yizhang-nv", "liji-nv"],
"Torch Custom Ops": ["yizhang-nv"],
"Torch Distributed": ["yilin-void", "yuxianq", "hyukn", "yizhang-nv", "hlu1"],
"Torch PyExecutor": ["dongxuy04", "funatiq", "dcampora", "HuiGao-NV"],
"Torch Speculative": ["lfr-0531", "mikeiovine"],
"Autotuner": ["hyukn", "litaotju"],
"Pipeline Interface": ["amukkara", "chang-l"],
"Torch Models": ["QiJune", "hlu1"],
"Torch Models DeepSeekV3": ["hlu1", "zongfeijing"],
"Torch Models Llama": ["chang-l", "mikeiovine"],
"Torch Modules": ["QiJune", "hlu1"],
"Torch Modules Attention": ["yuxianq", "hlu1"],
"Torch Modules Fused MOE": ["hlu1", "dongxuy04", "zongfeijing", "HuiGao-NV"],
"Torch Tests": ["QiJune", "hlu1"],
"PyTorch Examples": ["QiJune", "hlu1"]
}
33 changes: 33 additions & 0 deletions .github/module-paths.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"cpp/": "Generic Runtime",
"triton_backend/": "Triton Backend",
"tensorrt_llm/_torch/peft/": "Lora/P-tuning",
"tensorrt_llm/": "LLM API/Workflow",
"benchmarks/": "Performance",
"examples/disaggregated/": "Disaggregated Serving",
"docs/": "Documentation",
"docker/": "Installation",
".github/": "GitHub Configuration",
"jenkins/": "Jenkins Pipelines",
"tests/integration/test_lists/": "Test Configuration",
"tests/integration/test_lists/waives.txt": "Test Waive List",
"tests/integration/defs/": "Integration Tests",
"tensorrt_llm/_torch/": "Torch Framework",
"tensorrt_llm/_torch/attention_backend/": "Torch Attention Backend",
"tensorrt_llm/_torch/auto_deploy/": "Torch AutoDeploy",
"tensorrt_llm/_torch/compilation/": "Torch Compilation",
"tensorrt_llm/_torch/custom_ops/": "Torch Custom Ops",
"tensorrt_llm/_torch/distributed/": "Torch Distributed",
"tensorrt_llm/_torch/pyexecutor/": "Torch PyExecutor",
"tensorrt_llm/_torch/speculative/": "Torch Speculative",
"tensorrt_llm/autotuner.py": "Autotuner",
"tensorrt_llm/pipeline_interface.py": "Pipeline Interface",
"tensorrt_llm/_torch/models/": "Torch Models",
"tensorrt_llm/_torch/models/modeling_deepseekv3.py": "Torch Models DeepSeekV3",
"tensorrt_llm/_torch/models/modeling_llama.py": "Torch Models Llama",
"tensorrt_llm/_torch/modules/": "Torch Modules",
"tensorrt_llm/_torch/modules/attention.py": "Torch Modules Attention",
"tensorrt_llm/_torch/modules/fused_moe.py": "Torch Modules Fused MOE",
"tests/unittest/_torch/": "Torch Tests",
"examples/pytorch/": "PyTorch Examples"
}
295 changes: 295 additions & 0 deletions .github/scripts/assign_reviewers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,295 @@
import argparse
import json
import os
import random
import subprocess
import sys
from pathlib import Path


def get_pr_changed_files(pr_number: str) -> list[str]:
"""Get files changed in PR using GitHub CLI (more reliable than git diff)"""
result = subprocess.run(
[
"gh", "pr", "view", pr_number, "--json", "files", "--jq",
".files[].path"
],
capture_output=True,
text=True,
check=True,
)
return [line.strip() for line in result.stdout.splitlines() if line.strip()]


def get_existing_reviewers(pr_number: str) -> tuple[set[str], set[str]]:
"""Get currently assigned reviewers (users and teams) for a PR"""
try:
# Get user reviewers
user_result = subprocess.run(
[
"gh", "pr", "view", pr_number, "--json", "reviewRequests",
"--jq",
"(.reviewRequests // []) | .[] | select(.login) | .login"
],
capture_output=True,
text=True,
check=True,
)
user_reviewers = {
line.strip()
for line in user_result.stdout.splitlines() if line.strip()
}

# Get team reviewers
team_result = subprocess.run(
Comment on lines +43 to +44
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@venkywonka venkywonka Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@kevinch-nv kevinch-nv Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.
  2. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@venkywonka venkywonka Jul 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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 )
  2. 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.
  3. 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).
  4. 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.

[
"gh", "pr", "view", pr_number, "--json", "reviewRequests",
"--jq", "(.reviewRequests // []) | .[] | select(.name) | .name"
],
capture_output=True,
text=True,
check=True,
)
team_reviewers = {
line.strip()
for line in team_result.stdout.splitlines() if line.strip()
}

return user_reviewers, team_reviewers
except subprocess.CalledProcessError as e:
print(f"Warning: Could not fetch existing reviewers: {e}")
return set(), set()


def load_json(path: str):
with open(path, "r", encoding="utf-8") as f:
return json.load(f)


def map_modules(changed_files: list[str],
module_paths: dict[str, str]) -> tuple[set[str], list[str]]:
"""Map changed files to modules using MOST SPECIFIC (longest) prefix match"""
modules: set[str] = set()
unmapped_files: list[str] = []

for file in changed_files:
# Find ALL matching prefixes
matches = []
for prefix, module in module_paths.items():
if file.startswith(prefix):
matches.append((len(prefix), prefix, module))

if matches:
# Sort by prefix length (descending) to get most specific first
matches.sort(reverse=True)
most_specific_module = matches[0][2]
modules.add(most_specific_module)

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

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copilot uses AI. Check for mistakes.
print(f" File '{file}' has overlapping mappings:")
for _, prefix, module in matches:
marker = "→" if module == most_specific_module else " "
print(f" {marker} {prefix} -> {module}")
else:
unmapped_files.append(file)

return modules, unmapped_files


def gather_reviewers(
modules: set[str],
module_owners: dict[str, list[str]],
*,
pr_author: str | None = None,
existing_reviewers: set[str] | None = None,
per_module_limit: int = 2
) -> tuple[list[str], dict[str, list[str]], set[str]]:
"""
Gather reviewers ensuring each module gets representation.

Args:
modules: Set of module names that were touched
module_owners: Dict mapping module names to lists of owners
pr_author: PR author to exclude from reviewers
existing_reviewers: Set of already assigned reviewers to exclude
per_module_limit: Maximum reviewers to assign per module

Returns:
- List of all unique reviewers to assign
- Dict mapping modules to their assigned reviewers
- Set of modules without owners
"""
all_reviewers: set[str] = set()
module_assignments: dict[str, list[str]] = {}
modules_without_owners: set[str] = set()

for module in sorted(modules): # Sort for consistent ordering
owners = module_owners.get(module, [])
if not owners:
modules_without_owners.add(module)
module_assignments[module] = []
continue

# Filter out PR author and existing reviewers
eligible_owners = [
o for o in owners if o != pr_author and (
not existing_reviewers or o not in existing_reviewers)
]

if not eligible_owners:
# All owners are excluded
print(
f" ⚠️ Module '{module}': All owners excluded (PR author or already assigned)"
)
module_assignments[module] = []
continue

# Sample up to per_module_limit reviewers for this module
num_to_select = min(len(eligible_owners), per_module_limit)
selected = random.sample(eligible_owners, num_to_select)

module_assignments[module] = selected
all_reviewers.update(selected)

return sorted(all_reviewers), module_assignments, modules_without_owners


def main() -> None:
parser = argparse.ArgumentParser(
description="Assign reviewers based on changed modules")
parser.add_argument("--dry-run",
action="store_true",
help="Print the gh command instead of executing")
parser.add_argument(
"--force-assign",
action="store_true",
help=
"Assign reviewers even if some already exist (default: only assign if no reviewers)"
)
args = parser.parse_args()

pr_number = os.environ["PR_NUMBER"]
per_module_limit = int(os.environ.get("PER_MODULE_REVIEWER_LIMIT", "2"))
pr_author = os.environ.get("PR_AUTHOR")

print(f"Testing PR #{pr_number} with author: {pr_author}")
print(f"Per-module reviewer limit: {per_module_limit}")

# Check existing reviewers
existing_user_reviewers, existing_team_reviewers = get_existing_reviewers(
pr_number)
total_existing = len(existing_user_reviewers) + len(existing_team_reviewers)

print(f"Existing user reviewers: {sorted(existing_user_reviewers)}")
print(f"Existing team reviewers: {sorted(existing_team_reviewers)}")

# Skip assignment if reviewers already exist (unless forced)
if total_existing > 0 and not args.force_assign:
print(
f"✅ PR already has {total_existing} reviewer(s) assigned. Skipping auto-assignment."
)
print(" Use --force-assign to assign additional reviewers.")
return

try:
changed_files = get_pr_changed_files(pr_number)
print(f"Changed files: {changed_files}")

module_paths = load_json(Path(".github") / "module-paths.json")
module_owners = load_json(Path(".github") / "module-owners.json")

modules, unmapped_files = map_modules(changed_files, module_paths)
reviewers, module_assignments, modules_without_owners = gather_reviewers(
modules,
module_owners,
pr_author=pr_author,
existing_reviewers=
existing_user_reviewers, # Avoid re-assigning existing users
per_module_limit=per_module_limit)

print(f"\nChanged modules: {sorted(modules)}")

# Show module-specific assignments
if module_assignments:
print("\nModule assignments:")
for module, assigned in sorted(module_assignments.items()):
if assigned:
print(f" {module}: {assigned}")
else:
print(f" {module}: No eligible reviewers")

print(f"\nFinal reviewers to assign: {reviewers}")

# Provide detailed feedback about coverage gaps
if unmapped_files:
print(f"⚠️ Files with no module mapping: {unmapped_files}")
print(
f" These files are not covered in .github/module-paths.json")
print(
f" Consider adding appropriate module mappings for these paths."
)

if modules_without_owners:
print(
f"⚠️ Modules with no owners: {sorted(modules_without_owners)}")
print(
f" These modules exist in module-paths.json but have no owners in module-owners.json"
)
print(f" Consider adding owner assignments for these modules.")

if reviewers:
cmd = ["gh", "pr", "edit", pr_number]
for reviewer in reviewers:
cmd.extend(["--add-reviewer", reviewer])

if args.dry_run:
print(f"🔍 DRY RUN: {' '.join(cmd)}")
else:
try:
subprocess.run(cmd, check=True)
print(
f"✅ Successfully assigned {len(reviewers)} new reviewer(s)"
)
except subprocess.CalledProcessError as e:
print(f"❌ Failed to add reviewers: {e}", file=sys.stderr)
print(
" This might be due to permissions or invalid usernames"
)
sys.exit(1)
else:
print("✅ No new reviewers to assign")

# Explain why no reviewers were assigned
if not modules and not unmapped_files:
print(" Reason: No files were changed in this PR")
elif not modules and unmapped_files:
print(
" Reason: All changed files are unmapped (no module coverage)"
)
print(
" ➜ Action needed: Add module mappings to .github/module-paths.json"
)
elif modules and not reviewers:
if modules_without_owners:
print(" Reason: Matched modules have no assigned owners")
print(
" ➜ Action needed: Add owner assignments to .github/module-owners.json"
)
else:
print(
" Reason: All potential reviewers are already assigned or excluded"
)
else:
print(
" Reason: Complex combination of mapping/ownership issues (see warnings above)"
)

except subprocess.CalledProcessError as e:
print(f"❌ Error processing PR: {e}", file=sys.stderr)
sys.exit(1)


if __name__ == "__main__":
main()
Loading