From 00b419ea0f94bc137a86f2676cb1c17f3d20db5d Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 26 Jun 2025 12:45:57 -0700 Subject: [PATCH 01/46] first commit Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/module-paths.json | 10 +++ .github/scripts/assign_reviewers.py | 86 +++++++++++++++++++++ .github/workflows/auto-assign-reviewers.yml | 23 ++++++ CONTRIBUTING.md | 7 ++ 4 files changed, 126 insertions(+) create mode 100644 .github/module-paths.json create mode 100644 .github/scripts/assign_reviewers.py create mode 100644 .github/workflows/auto-assign-reviewers.yml diff --git a/.github/module-paths.json b/.github/module-paths.json new file mode 100644 index 000000000000..0fea39a79ef9 --- /dev/null +++ b/.github/module-paths.json @@ -0,0 +1,10 @@ +{ + "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" +} diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py new file mode 100644 index 000000000000..183fdca84ad4 --- /dev/null +++ b/.github/scripts/assign_reviewers.py @@ -0,0 +1,86 @@ +import argparse +import json +import os +import subprocess +from pathlib import Path + + +def run_git_diff(base: str, head: str) -> list[str]: + result = subprocess.run( + ["git", "diff", "--name-only", base, head], + capture_output=True, + text=True, + check=True, + ) + return [line.strip() for line in result.stdout.splitlines() if line.strip()] + + +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]) -> set[str]: + modules: set[str] = set() + for file in changed_files: + for prefix, module in module_paths.items(): + if file.startswith(prefix): + modules.add(module) + break + return modules + + +def gather_reviewers(modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None) -> list[str]: + reviewers: set[str] = set() + for module in modules: + reviewers.update(module_owners.get(module, [])) + + if pr_author: + reviewers.discard(pr_author) + + return sorted(reviewers) + + +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") + args = parser.parse_args() + + base_sha = os.environ["BASE_SHA"] + head_sha = os.environ["HEAD_SHA"] + pr_number = os.environ["PR_NUMBER"] + reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) + pr_author = os.environ.get("PR_AUTHOR") + + changed_files = run_git_diff(base_sha, head_sha) + module_paths = load_json(Path(".github") / "module-paths.json") + module_owners = load_json(Path(".github/workflows") / "module-owners.json") + + modules = map_modules(changed_files, module_paths) + reviewers = gather_reviewers(modules, module_owners, pr_author=pr_author) + + if reviewer_limit: + reviewers = reviewers[:reviewer_limit] + + if reviewers: + cmd = ["gh", "pr", "edit", pr_number] + for reviewer in reviewers: + cmd.extend(["--add-reviewer", reviewer]) + if args.dry_run: + print("Dry run:", " ".join(cmd)) + else: + subprocess.run(cmd, check=True) + + print(f"Changed modules: {sorted(modules)}") + print(f"Requested reviewers: {reviewers}") + + +if __name__ == "__main__": + main() \ No newline at end of file diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml new file mode 100644 index 000000000000..50bddbabe370 --- /dev/null +++ b/.github/workflows/auto-assign-reviewers.yml @@ -0,0 +1,23 @@ +name: Auto assign reviewers +on: + pull_request: + types: [opened, synchronize, reopened] +permissions: + pull-requests: write +jobs: + assign: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Assign reviewers + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REVIEWER_LIMIT: '3' + run: python3 .github/scripts/assign_reviewers.py \ No newline at end of file diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c8995b2ef0c..f978dba153dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,6 +93,13 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. + * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. + * You can test the assignment script locally with the `--dry-run` flag: + ```bash + GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ + PR_AUTHOR= \ + python3 .github/scripts/assign_reviewers.py --dry-run + ``` * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From 40a7385cf7d98a03b63f4f3a353d40023b714588 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 26 Jun 2025 17:37:19 -0700 Subject: [PATCH 02/46] add some more module-owners/modules Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/module-paths.json | 22 +++++++++++++++++++++- .github/scripts/assign_reviewers.py | 5 +++-- .github/workflows/module-owners.json | 23 +++++++++++++++++++++-- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/.github/module-paths.json b/.github/module-paths.json index 0fea39a79ef9..0c97a24db17a 100644 --- a/.github/module-paths.json +++ b/.github/module-paths.json @@ -6,5 +6,25 @@ "benchmarks/": "Performance", "examples/disaggregated/": "Disaggregated Serving", "docs/": "Documentation", - "docker/": "Installation" + "docker/": "Installation", + ".github/": "CI/CD", + "jenkins/": "CI/CD", + "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" } diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 183fdca84ad4..893218bc0548 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -1,6 +1,7 @@ import argparse import json import os +import random import subprocess from pathlib import Path @@ -66,8 +67,8 @@ def main() -> None: modules = map_modules(changed_files, module_paths) reviewers = gather_reviewers(modules, module_owners, pr_author=pr_author) - if reviewer_limit: - reviewers = reviewers[:reviewer_limit] + if reviewer_limit and len(reviewers) > reviewer_limit: + reviewers = random.sample(reviewers, reviewer_limit) if reviewers: cmd = ["gh", "pr", "edit", pr_number] diff --git a/.github/workflows/module-owners.json b/.github/workflows/module-owners.json index bab5760f012d..c799be4a6bb1 100644 --- a/.github/workflows/module-owners.json +++ b/.github/workflows/module-owners.json @@ -7,10 +7,29 @@ "Speculative Decoding":["yweng0828", "nekorobov", "lfr-0531"], "Customized Kernels":["lowsfer", "PerkzZheng", "jdemouth-nvidia"], "Performance": ["kaiyux", "jiahanc", "hypdeb"], - "Lora/P-tuning":["byshiue", "Naveassaf"], + "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"] + "Installation": ["hchings", "Superjomn", "nv-guomingz", "QiJune"], + "CI/CD": ["chzblych", "syuoni"], + "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"] } From fa9face610b48cdaa0c5baa97141f7c30a0e582d Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Fri, 27 Jun 2025 13:45:15 -0700 Subject: [PATCH 03/46] bug fix Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 14 ++++++++------ .github/workflows/auto-assign-reviewers.yml | 6 ++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 893218bc0548..1a1164a3822c 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -6,9 +6,13 @@ from pathlib import Path -def run_git_diff(base: str, head: str) -> list[str]: +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( - ["git", "diff", "--name-only", base, head], + [ + "gh", "pr", "view", pr_number, "--json", "files", "--jq", + ".files[].path" + ], capture_output=True, text=True, check=True, @@ -54,13 +58,11 @@ def main() -> None: help="Print the gh command instead of executing") args = parser.parse_args() - base_sha = os.environ["BASE_SHA"] - head_sha = os.environ["HEAD_SHA"] pr_number = os.environ["PR_NUMBER"] reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) pr_author = os.environ.get("PR_AUTHOR") - changed_files = run_git_diff(base_sha, head_sha) + changed_files = get_pr_changed_files(pr_number) module_paths = load_json(Path(".github") / "module-paths.json") module_owners = load_json(Path(".github/workflows") / "module-owners.json") @@ -84,4 +86,4 @@ def main() -> None: if __name__ == "__main__": - main() \ No newline at end of file + main() diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 50bddbabe370..96c272ed4500 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -14,10 +14,8 @@ jobs: python-version: '3.12' - name: Assign reviewers env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} PR_NUMBER: ${{ github.event.pull_request.number }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REVIEWER_LIMIT: '3' - run: python3 .github/scripts/assign_reviewers.py \ No newline at end of file + run: python3 .github/scripts/assign_reviewers.py From 4786bd9165c9e72296be3ac9fed2cc7f4d06b46b Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Fri, 27 Jun 2025 15:18:10 -0700 Subject: [PATCH 04/46] use default permissions in action Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 96c272ed4500..02b0c6bb8e08 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -2,8 +2,6 @@ name: Auto assign reviewers on: pull_request: types: [opened, synchronize, reopened] -permissions: - pull-requests: write jobs: assign: runs-on: ubuntu-latest From 6e849699674a51e410cec162bc1d4d11d924729e Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 09:30:06 -0700 Subject: [PATCH 05/46] use new REVIEW_ASSIGNING_TOKEN secret Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 02b0c6bb8e08..81c1645aa41f 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -14,6 +14,6 @@ jobs: env: PR_NUMBER: ${{ github.event.pull_request.number }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} REVIEWER_LIMIT: '3' run: python3 .github/scripts/assign_reviewers.py From 8f61bb05685b27e0d4ae1c7a3fb1d98a740c155f Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 10:10:00 -0700 Subject: [PATCH 06/46] pull_request->pull_request_target to access secrets Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 81c1645aa41f..02cfbad9fc7d 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request: + pull_request_target: types: [opened, synchronize, reopened] jobs: assign: From c32a374df185dbaf52034c0eeba15d037d81c38d Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 10:46:21 -0700 Subject: [PATCH 07/46] dry run changes Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 02cfbad9fc7d..01af81099b0c 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -2,6 +2,17 @@ name: Auto assign reviewers on: pull_request_target: types: [opened, synchronize, reopened] + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to test assignment on' + required: true + type: string + dry_run: + description: 'Run in dry-run mode (just print commands)' + required: false + type: boolean + default: true jobs: assign: runs-on: ubuntu-latest @@ -12,8 +23,8 @@ jobs: python-version: '3.12' - name: Assign reviewers env: - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_AUTHOR: ${{ github.event.pull_request.user.login }} + PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login || '' }} GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} REVIEWER_LIMIT: '3' - run: python3 .github/scripts/assign_reviewers.py + run: python3 .github/scripts/assign_reviewers.py ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} From 405f0d35df1e4fde3bff2926367ca123b031e895 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 15:19:36 -0700 Subject: [PATCH 08/46] Enhance auto-reviewer assignment with duplicate prevention and better UX - 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 <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 137 +++++++++++++++++--- .github/workflows/auto-assign-reviewers.yml | 14 +- 2 files changed, 128 insertions(+), 23 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 1a1164a3822c..a5e8104a6393 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -3,6 +3,7 @@ import os import random import subprocess +import sys from pathlib import Path @@ -20,6 +21,46 @@ def get_pr_changed_files(pr_number: str) -> list[str]: 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( + [ + "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) @@ -39,7 +80,8 @@ def map_modules(changed_files: list[str], module_paths: dict[str, def gather_reviewers(modules: set[str], module_owners: dict[str, list[str]], *, - pr_author: str | None = None) -> list[str]: + pr_author: str | None = None, + existing_reviewers: set[str] | None = None) -> list[str]: reviewers: set[str] = set() for module in modules: reviewers.update(module_owners.get(module, [])) @@ -47,6 +89,10 @@ def gather_reviewers(modules: set[str], if pr_author: reviewers.discard(pr_author) + # Remove existing reviewers to avoid duplicate assignments + if existing_reviewers: + reviewers -= existing_reviewers + return sorted(reviewers) @@ -56,33 +102,84 @@ def main() -> None: 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"] reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) pr_author = os.environ.get("PR_AUTHOR") - changed_files = get_pr_changed_files(pr_number) - module_paths = load_json(Path(".github") / "module-paths.json") - module_owners = load_json(Path(".github/workflows") / "module-owners.json") - - modules = map_modules(changed_files, module_paths) - reviewers = gather_reviewers(modules, module_owners, pr_author=pr_author) - - if reviewer_limit and len(reviewers) > reviewer_limit: - reviewers = random.sample(reviewers, reviewer_limit) - - if reviewers: - cmd = ["gh", "pr", "edit", pr_number] - for reviewer in reviewers: - cmd.extend(["--add-reviewer", reviewer]) - if args.dry_run: - print("Dry run:", " ".join(cmd)) + print(f"Testing PR #{pr_number} with author: {pr_author}") + + # 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/workflows") / "module-owners.json") + + modules = map_modules(changed_files, module_paths) + reviewers = gather_reviewers( + modules, + module_owners, + pr_author=pr_author, + existing_reviewers= + existing_user_reviewers # Avoid re-assigning existing users + ) + + if reviewer_limit and len(reviewers) > reviewer_limit: + reviewers = random.sample(reviewers, reviewer_limit) + + print(f"Changed modules: {sorted(modules)}") + print(f"Potential reviewers: {reviewers}") + + 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: - subprocess.run(cmd, check=True) + print("✅ No new reviewers to assign") - print(f"Changed modules: {sorted(modules)}") - print(f"Requested reviewers: {reviewers}") + except subprocess.CalledProcessError as e: + print(f"❌ Error processing PR: {e}", file=sys.stderr) + sys.exit(1) if __name__ == "__main__": diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 01af81099b0c..c0a00bfc6dfd 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request_target: + pull_request: types: [opened, synchronize, reopened] workflow_dispatch: inputs: @@ -13,6 +13,11 @@ on: required: false type: boolean default: true + force_assign: + description: 'Force assign even if reviewers already exist' + required: false + type: boolean + default: false jobs: assign: runs-on: ubuntu-latest @@ -24,7 +29,10 @@ jobs: - name: Assign reviewers env: PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} - PR_AUTHOR: ${{ github.event.pull_request.user.login || '' }} + PR_AUTHOR: ${{ github.event.pull_request.user.login || github.event.inputs.pr_author || '' }} GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} REVIEWER_LIMIT: '3' - run: python3 .github/scripts/assign_reviewers.py ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} + run: | + python3 .github/scripts/assign_reviewers.py \ + ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} \ + ${{ github.event.inputs.force_assign == 'true' && '--force-assign' || '' }} From 64438f5ec635b8e228b7e858cb56f7ed7ef1cfdd Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 15:32:34 -0700 Subject: [PATCH 09/46] Switch to pull_request_target for secret access Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index c0a00bfc6dfd..bafe897e77b0 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request: + pull_request_target: types: [opened, synchronize, reopened] workflow_dispatch: inputs: From d1a39b0aa895c51d6340690f7d0dabc4f6ca785e Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 16:03:58 -0700 Subject: [PATCH 10/46] Add explicit permissions for pull request writes and secret access Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index bafe897e77b0..d7bf4b7d7428 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -12,7 +12,7 @@ on: description: 'Run in dry-run mode (just print commands)' required: false type: boolean - default: true + default: false force_assign: description: 'Force assign even if reviewers already exist' required: false @@ -21,6 +21,9 @@ on: jobs: assign: runs-on: ubuntu-latest + permissions: + pull-requests: write + contents: read steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 From decd175ef2fd1916afa7bdcaa2211ffb660b6201 Mon Sep 17 00:00:00 2001 From: venkywonka <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 23:43:28 +0000 Subject: [PATCH 11/46] Switch from pull_request_target to pull_request trigger --- .github/workflows/auto-assign-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index d7bf4b7d7428..efb99765e3ab 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request_target: + pull_request: types: [opened, synchronize, reopened] workflow_dispatch: inputs: From 5c8b292f14773e922022afc1653159d0020b2959 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 26 Jun 2025 12:45:57 -0700 Subject: [PATCH 12/46] Add auto-assign feature in github actions for PRs - 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 <23023424+venkywonka@users.noreply.github.com> --- .github/module-paths.json | 30 ++++ .github/scripts/assign_reviewers.py | 186 ++++++++++++++++++++ .github/workflows/auto-assign-reviewers.yml | 41 +++++ .github/workflows/module-owners.json | 23 ++- CONTRIBUTING.md | 7 + 5 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 .github/module-paths.json create mode 100644 .github/scripts/assign_reviewers.py create mode 100644 .github/workflows/auto-assign-reviewers.yml diff --git a/.github/module-paths.json b/.github/module-paths.json new file mode 100644 index 000000000000..0c97a24db17a --- /dev/null +++ b/.github/module-paths.json @@ -0,0 +1,30 @@ +{ + "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/": "CI/CD", + "jenkins/": "CI/CD", + "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" +} diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py new file mode 100644 index 000000000000..a5e8104a6393 --- /dev/null +++ b/.github/scripts/assign_reviewers.py @@ -0,0 +1,186 @@ +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( + [ + "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]) -> set[str]: + modules: set[str] = set() + for file in changed_files: + for prefix, module in module_paths.items(): + if file.startswith(prefix): + modules.add(module) + break + return modules + + +def gather_reviewers(modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None, + existing_reviewers: set[str] | None = None) -> list[str]: + reviewers: set[str] = set() + for module in modules: + reviewers.update(module_owners.get(module, [])) + + if pr_author: + reviewers.discard(pr_author) + + # Remove existing reviewers to avoid duplicate assignments + if existing_reviewers: + reviewers -= existing_reviewers + + return sorted(reviewers) + + +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"] + reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) + pr_author = os.environ.get("PR_AUTHOR") + + print(f"Testing PR #{pr_number} with author: {pr_author}") + + # 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/workflows") / "module-owners.json") + + modules = map_modules(changed_files, module_paths) + reviewers = gather_reviewers( + modules, + module_owners, + pr_author=pr_author, + existing_reviewers= + existing_user_reviewers # Avoid re-assigning existing users + ) + + if reviewer_limit and len(reviewers) > reviewer_limit: + reviewers = random.sample(reviewers, reviewer_limit) + + print(f"Changed modules: {sorted(modules)}") + print(f"Potential reviewers: {reviewers}") + + 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") + + except subprocess.CalledProcessError as e: + print(f"❌ Error processing PR: {e}", file=sys.stderr) + sys.exit(1) + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml new file mode 100644 index 000000000000..efb99765e3ab --- /dev/null +++ b/.github/workflows/auto-assign-reviewers.yml @@ -0,0 +1,41 @@ +name: Auto assign reviewers +on: + pull_request: + types: [opened, synchronize, reopened] + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to test assignment on' + required: true + type: string + dry_run: + description: 'Run in dry-run mode (just print commands)' + required: false + type: boolean + default: false + force_assign: + description: 'Force assign even if reviewers already exist' + required: false + type: boolean + default: false +jobs: + assign: + runs-on: ubuntu-latest + permissions: + pull-requests: write + contents: read + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Assign reviewers + env: + PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login || github.event.inputs.pr_author || '' }} + GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} + REVIEWER_LIMIT: '3' + run: | + python3 .github/scripts/assign_reviewers.py \ + ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} \ + ${{ github.event.inputs.force_assign == 'true' && '--force-assign' || '' }} diff --git a/.github/workflows/module-owners.json b/.github/workflows/module-owners.json index bab5760f012d..c799be4a6bb1 100644 --- a/.github/workflows/module-owners.json +++ b/.github/workflows/module-owners.json @@ -7,10 +7,29 @@ "Speculative Decoding":["yweng0828", "nekorobov", "lfr-0531"], "Customized Kernels":["lowsfer", "PerkzZheng", "jdemouth-nvidia"], "Performance": ["kaiyux", "jiahanc", "hypdeb"], - "Lora/P-tuning":["byshiue", "Naveassaf"], + "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"] + "Installation": ["hchings", "Superjomn", "nv-guomingz", "QiJune"], + "CI/CD": ["chzblych", "syuoni"], + "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"] } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c8995b2ef0c..f978dba153dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,6 +93,13 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. + * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. + * You can test the assignment script locally with the `--dry-run` flag: + ```bash + GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ + PR_AUTHOR= \ + python3 .github/scripts/assign_reviewers.py --dry-run + ``` * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From e11ccf914fc340ddae23e0037e732dc5d40afc84 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Tue, 1 Jul 2025 00:39:28 +0000 Subject: [PATCH 13/46] docs: add detailed auto-assign PR reviewer documentation 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 <23023424+venkywonka@users.noreply.github.com> --- CONTRIBUTING.md | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f978dba153dc..09d6ba1300d0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,13 +93,27 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. - * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. - * You can test the assignment script locally with the `--dry-run` flag: - ```bash - GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ - PR_AUTHOR= \ - python3 .github/scripts/assign_reviewers.py --dry-run - ``` + +### Automatic Reviewer Assignment + +Reviewers are automatically assigned to PRs through a GitHub Action that: + +* **Triggers**: Runs automatically when PRs are opened, synchronized, or reopened +* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/workflows/module-owners.json` +* **Respects existing assignments**: Won't assign additional reviewers if any reviewers are already assigned (unless forced) +* **Excludes PR author**: Automatically excludes the PR author from reviewer assignments +* **Limits reviewer count**: Randomly samples up to 3 reviewers if more are eligible (configurable via `REVIEWER_LIMIT`) +* **Coexists with CODEOWNERS**: Works alongside GitHub's CODEOWNERS file (`.github/CODEOWNERS`) which enforces mandatory approvals for specific paths (e.g., API stability tests, release branches) + +The auto-assignment system analyzes all files changed in your PR, maps them to the appropriate code modules, and assigns reviewers from the module owner lists. This ensures domain experts review relevant changes while avoiding over-assignment. + +**Testing the assignment locally**: You can test reviewer assignment with the `--dry-run` flag: + ```bash + GH_TOKEN= PR_NUMBER= PR_AUTHOR= \ + python3 .github/scripts/assign_reviewers.py --dry-run + ``` + +**Manual assignment**: You can also manually trigger reviewer assignment via GitHub's workflow dispatch with options for dry-run mode and force-assignment. * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From 9be3f699f10f447233387ebe2a38dfd6bf292600 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Tue, 1 Jul 2025 19:39:36 -0700 Subject: [PATCH 14/46] add comprehensive testing, change to pull_request_target Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .../scripts/tests/test_assign_reviewers.py | 466 ++++++++++++++++++ .github/workflows/auto-assign-reviewers.yml | 2 +- 2 files changed, 467 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/tests/test_assign_reviewers.py diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py new file mode 100644 index 000000000000..0038f980201e --- /dev/null +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -0,0 +1,466 @@ +#!/usr/bin/env python3 +""" +End-to-end tests for assign_reviewers.py script. +Tests various scenarios without requiring GitHub API access or tokens. +""" + +import os +import subprocess +import sys +import unittest +from pathlib import Path +from unittest.mock import MagicMock, patch + +# Add parent directory to path to import the script +sys.path.insert(0, str(Path(__file__).parent.parent)) +import assign_reviewers + + +class TestAssignReviewers(unittest.TestCase): + """Test suite for the assign_reviewers.py script""" + + def setUp(self): + """Set up test fixtures""" + # Sample module-paths.json data + self.module_paths = { + "cpp/": "Generic Runtime", + "tensorrt_llm/": "LLM API/Workflow", + "benchmarks/": "Performance", + "docs/": "Documentation", + "tensorrt_llm/_torch/": "Torch Framework" + } + + # Sample module-owners.json data + self.module_owners = { + "Generic Runtime": ["user1", "user2", "user3"], + "LLM API/Workflow": ["user4", "user5"], + "Performance": ["user6", "user7", "user8"], + "Documentation": ["user9"], + "Torch Framework": ["user10", "user11"] + } + + # Set required environment variables + os.environ["PR_NUMBER"] = "123" + os.environ["PR_AUTHOR"] = "test_author" + os.environ["REVIEWER_LIMIT"] = "3" + + def tearDown(self): + """Clean up environment variables""" + for var in ["PR_NUMBER", "PR_AUTHOR", "REVIEWER_LIMIT"]: + if var in os.environ: + del os.environ[var] + + def _mock_subprocess_run(self, *args, **kwargs): + """Mock subprocess.run based on the command being executed""" + cmd = args[0] + cmd_str = ' '.join(cmd) # Join command for easier matching + + # Mock response for getting changed files + if "pr" in cmd and "view" in cmd and "files" in cmd: + return MagicMock(stdout=self.mock_changed_files, + stderr="", + returncode=0) + + # Mock response for getting existing reviewers (users) + elif "pr" in cmd and "view" in cmd and "reviewRequests" in cmd: + # Check if it's asking for login (users) or name (teams) + if "select(.login)" in cmd_str: + return MagicMock(stdout=self.mock_existing_users, + stderr="", + returncode=0) + elif "select(.name)" in cmd_str: + return MagicMock(stdout=self.mock_existing_teams, + stderr="", + returncode=0) + else: + return MagicMock(stdout="", stderr="", returncode=0) + + # Mock response for assigning reviewers + elif "pr" in cmd and "edit" in cmd: + self.assign_reviewers_called = True + self.assigned_reviewers = [ + cmd[i + 1] for i, arg in enumerate(cmd) + if arg == "--add-reviewer" + ] + return MagicMock(stdout="", stderr="", returncode=0) + + return MagicMock(stdout="", stderr="", returncode=0) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_single_module_changed(self, mock_run, mock_load_json): + """Test PR with files from a single module""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\ncpp/file2.h\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # Should respect limit + self.assertTrue( + all(r in ["user1", "user2", "user3"] + for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_multiple_modules_changed(self, mock_run, mock_load_json): + """Test PR with files from multiple modules""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\ndocs/README.md\nbenchmarks/test.py\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned from multiple modules + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # Should respect limit + # Should have mix of reviewers from different modules + all_possible = [ + "user1", "user2", "user3", "user6", "user7", "user8", "user9" + ] + self.assertTrue(all(r in all_possible for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_no_matching_module(self, mock_run, mock_load_json): + """Test PR with files that don't match any module""" + # Setup mocks + self.mock_changed_files = "unknown/file.txt\nrandom/path.py\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_existing_reviewers_skip(self, mock_run, mock_load_json): + """Test that assignment is skipped when reviewers already exist""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "existing_user1\nexisting_user2\n" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function (without force-assign) + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no new reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_force_assign_with_existing(self, mock_run, mock_load_json): + """Test force-assign flag with existing reviewers""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "user1\n" # user1 is already assigned + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run with force-assign flag + with patch('sys.argv', ['assign_reviewers.py', '--force-assign']): + assign_reviewers.main() + + # Verify reviewers were assigned, excluding already assigned ones + self.assertTrue(self.assign_reviewers_called) + self.assertNotIn("user1", + self.assigned_reviewers) # Should not re-assign + self.assertTrue( + all(r in ["user2", "user3"] for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_pr_author_excluded(self, mock_run, mock_load_json): + """Test that PR author is excluded from reviewers""" + # Setup with PR author as a potential reviewer + os.environ["PR_AUTHOR"] = "user2" + + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify author is not in assigned reviewers + self.assertTrue(self.assign_reviewers_called) + self.assertNotIn("user2", self.assigned_reviewers) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_reviewer_limit_zero(self, mock_run, mock_load_json): + """Test with reviewer limit set to 0 (no limit)""" + os.environ["REVIEWER_LIMIT"] = "0" + + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify all reviewers were assigned (no limit) + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # All from Generic Runtime + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_dry_run_mode(self, mock_run, mock_load_json): + """Test dry-run mode doesn't execute commands""" + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Capture printed output + import io + from contextlib import redirect_stdout + + f = io.StringIO() + with redirect_stdout(f): + with patch('sys.argv', ['assign_reviewers.py', '--dry-run']): + assign_reviewers.main() + + output = f.getvalue() + + # Verify dry run message was printed and no actual assignment + self.assertIn("DRY RUN:", output) + self.assertIn("gh pr edit", output) + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_empty_pr_no_files(self, mock_run, mock_load_json): + """Test PR with no changed files""" + self.mock_changed_files = "" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_subprocess_error_handling(self, mock_run, mock_load_json): + """Test error handling when subprocess commands fail""" + # Mock a subprocess error + mock_run.side_effect = subprocess.CalledProcessError( + 1, ["gh", "pr", "view"]) + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run should exit with error code + with self.assertRaises(SystemExit) as cm: + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + self.assertEqual(cm.exception.code, 1) + + def test_map_modules_function(self): + """Test the pure map_modules function""" + changed_files = [ + "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" + ] + + modules = assign_reviewers.map_modules(changed_files, self.module_paths) + + self.assertEqual(modules, {"Generic Runtime", "Documentation"}) + + def test_gather_reviewers_function(self): + """Test the pure gather_reviewers function""" + modules = {"Generic Runtime", "Documentation"} + + # Test without exclusions + reviewers = assign_reviewers.gather_reviewers(modules, + self.module_owners) + self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) + + # Test with author exclusion + reviewers = assign_reviewers.gather_reviewers(modules, + self.module_owners, + pr_author="user1") + self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) + + # Test with existing reviewers exclusion + reviewers = assign_reviewers.gather_reviewers( + modules, self.module_owners, existing_reviewers={"user2", "user9"}) + self.assertEqual(set(reviewers), {"user1", "user3"}) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_module_with_no_owners(self, mock_run, mock_load_json): + """Test module that has no owners defined""" + # Add a module with no owners + module_owners_with_empty = self.module_owners.copy() + module_owners_with_empty["Empty Module"] = [] + + self.mock_changed_files = "empty/file.txt\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ({ + "empty/": "Empty Module" + } if "module-paths" in str(path) else module_owners_with_empty) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_files_with_special_characters(self, mock_run, mock_load_json): + """Test files with special characters in names""" + self.mock_changed_files = "cpp/file with spaces.cpp\ncpp/file[brackets].h\ncpp/file@special.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned correctly despite special characters + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), 3) + + @patch('assign_reviewers.load_json') + def test_json_file_not_found(self, mock_load_json): + """Test handling of missing JSON configuration files""" + mock_load_json.side_effect = FileNotFoundError( + "module-paths.json not found") + + # Run should exit with error + with self.assertRaises(FileNotFoundError): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_large_reviewer_pool(self, mock_run, mock_load_json): + """Test with a large number of potential reviewers""" + # Create a module with many owners + large_module_owners = self.module_owners.copy() + large_module_owners["Large Module"] = [f"user{i}" for i in range(20)] + + self.mock_changed_files = "large/file.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ({ + "large/": "Large Module" + } if "module-paths" in str(path) else large_module_owners) + + # Run the main function with limit + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify only 3 reviewers were selected (respecting REVIEWER_LIMIT) + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), 3) + self.assertTrue( + all(r in [f"user{i}" for i in range(20)] + for r in self.assigned_reviewers)) + + @patch('subprocess.run') + def test_missing_environment_variables(self, mock_run): + """Test behavior when required environment variables are missing""" + # Remove PR_NUMBER + if "PR_NUMBER" in os.environ: + del os.environ["PR_NUMBER"] + + # Should raise KeyError + with self.assertRaises(KeyError): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + +if __name__ == "__main__": + # Run tests with verbose output + unittest.main(verbosity=2) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index efb99765e3ab..d7bf4b7d7428 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request: + pull_request_target: types: [opened, synchronize, reopened] workflow_dispatch: inputs: From 63339cd5ff0a8d84b36fb99004a1f5ad5901cd86 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 10:04:42 -0700 Subject: [PATCH 15/46] fix: Major module mapping overhaul for auto-assign reviewers - 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 <23023424+venkywonka@users.noreply.github.com> --- .github/module-owners.json | 39 +++++++++++++++++++++++++++++ .github/module-paths.json | 7 ++++-- .github/scripts/assign_reviewers.py | 3 +-- CONTRIBUTING.md | 8 +++--- 4 files changed, 49 insertions(+), 8 deletions(-) create mode 100644 .github/module-owners.json diff --git a/.github/module-owners.json b/.github/module-owners.json new file mode 100644 index 000000000000..5295e3547ec0 --- /dev/null +++ b/.github/module-owners.json @@ -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"] +} diff --git a/.github/module-paths.json b/.github/module-paths.json index 0c97a24db17a..45699eb7afa0 100644 --- a/.github/module-paths.json +++ b/.github/module-paths.json @@ -7,8 +7,11 @@ "examples/disaggregated/": "Disaggregated Serving", "docs/": "Documentation", "docker/": "Installation", - ".github/": "CI/CD", - "jenkins/": "CI/CD", + ".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", diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index a5e8104a6393..690e70fc7e20 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -137,8 +137,7 @@ def main() -> None: print(f"Changed files: {changed_files}") module_paths = load_json(Path(".github") / "module-paths.json") - module_owners = load_json( - Path(".github/workflows") / "module-owners.json") + module_owners = load_json(Path(".github") / "module-owners.json") modules = map_modules(changed_files, module_paths) reviewers = gather_reviewers( diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 09d6ba1300d0..f195156a269a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,13 +93,16 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. + * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. + * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. + * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. ### Automatic Reviewer Assignment Reviewers are automatically assigned to PRs through a GitHub Action that: * **Triggers**: Runs automatically when PRs are opened, synchronized, or reopened -* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/workflows/module-owners.json` +* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/module-owners.json` * **Respects existing assignments**: Won't assign additional reviewers if any reviewers are already assigned (unless forced) * **Excludes PR author**: Automatically excludes the PR author from reviewer assignments * **Limits reviewer count**: Randomly samples up to 3 reviewers if more are eligible (configurable via `REVIEWER_LIMIT`) @@ -114,9 +117,6 @@ The auto-assignment system analyzes all files changed in your PR, maps them to t ``` **Manual assignment**: You can also manually trigger reviewer assignment via GitHub's workflow dispatch with options for dry-run mode and force-assignment. - * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. - * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. - * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. ### PR Submission Policies From 782937454911d98db799af372b3416168fcff2e9 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 26 Jun 2025 12:45:57 -0700 Subject: [PATCH 16/46] first commit Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/module-paths.json | 10 +++ .github/scripts/assign_reviewers.py | 86 +++++++++++++++++++++ .github/workflows/auto-assign-reviewers.yml | 23 ++++++ CONTRIBUTING.md | 7 ++ 4 files changed, 126 insertions(+) create mode 100644 .github/module-paths.json create mode 100644 .github/scripts/assign_reviewers.py create mode 100644 .github/workflows/auto-assign-reviewers.yml diff --git a/.github/module-paths.json b/.github/module-paths.json new file mode 100644 index 000000000000..0fea39a79ef9 --- /dev/null +++ b/.github/module-paths.json @@ -0,0 +1,10 @@ +{ + "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" +} diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py new file mode 100644 index 000000000000..183fdca84ad4 --- /dev/null +++ b/.github/scripts/assign_reviewers.py @@ -0,0 +1,86 @@ +import argparse +import json +import os +import subprocess +from pathlib import Path + + +def run_git_diff(base: str, head: str) -> list[str]: + result = subprocess.run( + ["git", "diff", "--name-only", base, head], + capture_output=True, + text=True, + check=True, + ) + return [line.strip() for line in result.stdout.splitlines() if line.strip()] + + +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]) -> set[str]: + modules: set[str] = set() + for file in changed_files: + for prefix, module in module_paths.items(): + if file.startswith(prefix): + modules.add(module) + break + return modules + + +def gather_reviewers(modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None) -> list[str]: + reviewers: set[str] = set() + for module in modules: + reviewers.update(module_owners.get(module, [])) + + if pr_author: + reviewers.discard(pr_author) + + return sorted(reviewers) + + +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") + args = parser.parse_args() + + base_sha = os.environ["BASE_SHA"] + head_sha = os.environ["HEAD_SHA"] + pr_number = os.environ["PR_NUMBER"] + reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) + pr_author = os.environ.get("PR_AUTHOR") + + changed_files = run_git_diff(base_sha, head_sha) + module_paths = load_json(Path(".github") / "module-paths.json") + module_owners = load_json(Path(".github/workflows") / "module-owners.json") + + modules = map_modules(changed_files, module_paths) + reviewers = gather_reviewers(modules, module_owners, pr_author=pr_author) + + if reviewer_limit: + reviewers = reviewers[:reviewer_limit] + + if reviewers: + cmd = ["gh", "pr", "edit", pr_number] + for reviewer in reviewers: + cmd.extend(["--add-reviewer", reviewer]) + if args.dry_run: + print("Dry run:", " ".join(cmd)) + else: + subprocess.run(cmd, check=True) + + print(f"Changed modules: {sorted(modules)}") + print(f"Requested reviewers: {reviewers}") + + +if __name__ == "__main__": + main() \ No newline at end of file diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml new file mode 100644 index 000000000000..50bddbabe370 --- /dev/null +++ b/.github/workflows/auto-assign-reviewers.yml @@ -0,0 +1,23 @@ +name: Auto assign reviewers +on: + pull_request: + types: [opened, synchronize, reopened] +permissions: + pull-requests: write +jobs: + assign: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Assign reviewers + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REVIEWER_LIMIT: '3' + run: python3 .github/scripts/assign_reviewers.py \ No newline at end of file diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c8995b2ef0c..f978dba153dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,6 +93,13 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. + * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. + * You can test the assignment script locally with the `--dry-run` flag: + ```bash + GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ + PR_AUTHOR= \ + python3 .github/scripts/assign_reviewers.py --dry-run + ``` * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From 7091eaff92e88a838e5cdeafe3172e176f8b3f66 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 26 Jun 2025 17:37:19 -0700 Subject: [PATCH 17/46] add some more module-owners/modules Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/module-paths.json | 22 +++++++++++++++++++++- .github/scripts/assign_reviewers.py | 5 +++-- .github/workflows/module-owners.json | 23 +++++++++++++++++++++-- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/.github/module-paths.json b/.github/module-paths.json index 0fea39a79ef9..0c97a24db17a 100644 --- a/.github/module-paths.json +++ b/.github/module-paths.json @@ -6,5 +6,25 @@ "benchmarks/": "Performance", "examples/disaggregated/": "Disaggregated Serving", "docs/": "Documentation", - "docker/": "Installation" + "docker/": "Installation", + ".github/": "CI/CD", + "jenkins/": "CI/CD", + "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" } diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 183fdca84ad4..893218bc0548 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -1,6 +1,7 @@ import argparse import json import os +import random import subprocess from pathlib import Path @@ -66,8 +67,8 @@ def main() -> None: modules = map_modules(changed_files, module_paths) reviewers = gather_reviewers(modules, module_owners, pr_author=pr_author) - if reviewer_limit: - reviewers = reviewers[:reviewer_limit] + if reviewer_limit and len(reviewers) > reviewer_limit: + reviewers = random.sample(reviewers, reviewer_limit) if reviewers: cmd = ["gh", "pr", "edit", pr_number] diff --git a/.github/workflows/module-owners.json b/.github/workflows/module-owners.json index bab5760f012d..c799be4a6bb1 100644 --- a/.github/workflows/module-owners.json +++ b/.github/workflows/module-owners.json @@ -7,10 +7,29 @@ "Speculative Decoding":["yweng0828", "nekorobov", "lfr-0531"], "Customized Kernels":["lowsfer", "PerkzZheng", "jdemouth-nvidia"], "Performance": ["kaiyux", "jiahanc", "hypdeb"], - "Lora/P-tuning":["byshiue", "Naveassaf"], + "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"] + "Installation": ["hchings", "Superjomn", "nv-guomingz", "QiJune"], + "CI/CD": ["chzblych", "syuoni"], + "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"] } From 52713d22f3c8132b7b3becf367210df162b2ba77 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Fri, 27 Jun 2025 13:45:15 -0700 Subject: [PATCH 18/46] bug fix Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 14 ++++++++------ .github/workflows/auto-assign-reviewers.yml | 6 ++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 893218bc0548..1a1164a3822c 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -6,9 +6,13 @@ from pathlib import Path -def run_git_diff(base: str, head: str) -> list[str]: +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( - ["git", "diff", "--name-only", base, head], + [ + "gh", "pr", "view", pr_number, "--json", "files", "--jq", + ".files[].path" + ], capture_output=True, text=True, check=True, @@ -54,13 +58,11 @@ def main() -> None: help="Print the gh command instead of executing") args = parser.parse_args() - base_sha = os.environ["BASE_SHA"] - head_sha = os.environ["HEAD_SHA"] pr_number = os.environ["PR_NUMBER"] reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) pr_author = os.environ.get("PR_AUTHOR") - changed_files = run_git_diff(base_sha, head_sha) + changed_files = get_pr_changed_files(pr_number) module_paths = load_json(Path(".github") / "module-paths.json") module_owners = load_json(Path(".github/workflows") / "module-owners.json") @@ -84,4 +86,4 @@ def main() -> None: if __name__ == "__main__": - main() \ No newline at end of file + main() diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 50bddbabe370..96c272ed4500 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -14,10 +14,8 @@ jobs: python-version: '3.12' - name: Assign reviewers env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} PR_NUMBER: ${{ github.event.pull_request.number }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REVIEWER_LIMIT: '3' - run: python3 .github/scripts/assign_reviewers.py \ No newline at end of file + run: python3 .github/scripts/assign_reviewers.py From 8435ff05ffe7a159c25949ae9ec0a38c0b554c8f Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Fri, 27 Jun 2025 15:18:10 -0700 Subject: [PATCH 19/46] use default permissions in action Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 96c272ed4500..02b0c6bb8e08 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -2,8 +2,6 @@ name: Auto assign reviewers on: pull_request: types: [opened, synchronize, reopened] -permissions: - pull-requests: write jobs: assign: runs-on: ubuntu-latest From e58fe23fb821f781bfdcfcfcae2fb8de98a9f6f3 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 09:30:06 -0700 Subject: [PATCH 20/46] use new REVIEW_ASSIGNING_TOKEN secret Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 02b0c6bb8e08..81c1645aa41f 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -14,6 +14,6 @@ jobs: env: PR_NUMBER: ${{ github.event.pull_request.number }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} REVIEWER_LIMIT: '3' run: python3 .github/scripts/assign_reviewers.py From 89322bf153ba99f58260501cdbd7354fd562b421 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 10:10:00 -0700 Subject: [PATCH 21/46] pull_request->pull_request_target to access secrets Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 81c1645aa41f..02cfbad9fc7d 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request: + pull_request_target: types: [opened, synchronize, reopened] jobs: assign: From 9d78b23a07d22b582ba47456fad815e9ec275215 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 10:46:21 -0700 Subject: [PATCH 22/46] dry run changes Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 02cfbad9fc7d..01af81099b0c 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -2,6 +2,17 @@ name: Auto assign reviewers on: pull_request_target: types: [opened, synchronize, reopened] + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to test assignment on' + required: true + type: string + dry_run: + description: 'Run in dry-run mode (just print commands)' + required: false + type: boolean + default: true jobs: assign: runs-on: ubuntu-latest @@ -12,8 +23,8 @@ jobs: python-version: '3.12' - name: Assign reviewers env: - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_AUTHOR: ${{ github.event.pull_request.user.login }} + PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login || '' }} GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} REVIEWER_LIMIT: '3' - run: python3 .github/scripts/assign_reviewers.py + run: python3 .github/scripts/assign_reviewers.py ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} From 831501f3463e11e3a9bdc63f9b9fc7d1cfea2673 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 15:19:36 -0700 Subject: [PATCH 23/46] Enhance auto-reviewer assignment with duplicate prevention and better UX - 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 <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 137 +++++++++++++++++--- .github/workflows/auto-assign-reviewers.yml | 14 +- 2 files changed, 128 insertions(+), 23 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 1a1164a3822c..a5e8104a6393 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -3,6 +3,7 @@ import os import random import subprocess +import sys from pathlib import Path @@ -20,6 +21,46 @@ def get_pr_changed_files(pr_number: str) -> list[str]: 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( + [ + "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) @@ -39,7 +80,8 @@ def map_modules(changed_files: list[str], module_paths: dict[str, def gather_reviewers(modules: set[str], module_owners: dict[str, list[str]], *, - pr_author: str | None = None) -> list[str]: + pr_author: str | None = None, + existing_reviewers: set[str] | None = None) -> list[str]: reviewers: set[str] = set() for module in modules: reviewers.update(module_owners.get(module, [])) @@ -47,6 +89,10 @@ def gather_reviewers(modules: set[str], if pr_author: reviewers.discard(pr_author) + # Remove existing reviewers to avoid duplicate assignments + if existing_reviewers: + reviewers -= existing_reviewers + return sorted(reviewers) @@ -56,33 +102,84 @@ def main() -> None: 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"] reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) pr_author = os.environ.get("PR_AUTHOR") - changed_files = get_pr_changed_files(pr_number) - module_paths = load_json(Path(".github") / "module-paths.json") - module_owners = load_json(Path(".github/workflows") / "module-owners.json") - - modules = map_modules(changed_files, module_paths) - reviewers = gather_reviewers(modules, module_owners, pr_author=pr_author) - - if reviewer_limit and len(reviewers) > reviewer_limit: - reviewers = random.sample(reviewers, reviewer_limit) - - if reviewers: - cmd = ["gh", "pr", "edit", pr_number] - for reviewer in reviewers: - cmd.extend(["--add-reviewer", reviewer]) - if args.dry_run: - print("Dry run:", " ".join(cmd)) + print(f"Testing PR #{pr_number} with author: {pr_author}") + + # 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/workflows") / "module-owners.json") + + modules = map_modules(changed_files, module_paths) + reviewers = gather_reviewers( + modules, + module_owners, + pr_author=pr_author, + existing_reviewers= + existing_user_reviewers # Avoid re-assigning existing users + ) + + if reviewer_limit and len(reviewers) > reviewer_limit: + reviewers = random.sample(reviewers, reviewer_limit) + + print(f"Changed modules: {sorted(modules)}") + print(f"Potential reviewers: {reviewers}") + + 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: - subprocess.run(cmd, check=True) + print("✅ No new reviewers to assign") - print(f"Changed modules: {sorted(modules)}") - print(f"Requested reviewers: {reviewers}") + except subprocess.CalledProcessError as e: + print(f"❌ Error processing PR: {e}", file=sys.stderr) + sys.exit(1) if __name__ == "__main__": diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index 01af81099b0c..c0a00bfc6dfd 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request_target: + pull_request: types: [opened, synchronize, reopened] workflow_dispatch: inputs: @@ -13,6 +13,11 @@ on: required: false type: boolean default: true + force_assign: + description: 'Force assign even if reviewers already exist' + required: false + type: boolean + default: false jobs: assign: runs-on: ubuntu-latest @@ -24,7 +29,10 @@ jobs: - name: Assign reviewers env: PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} - PR_AUTHOR: ${{ github.event.pull_request.user.login || '' }} + PR_AUTHOR: ${{ github.event.pull_request.user.login || github.event.inputs.pr_author || '' }} GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} REVIEWER_LIMIT: '3' - run: python3 .github/scripts/assign_reviewers.py ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} + run: | + python3 .github/scripts/assign_reviewers.py \ + ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} \ + ${{ github.event.inputs.force_assign == 'true' && '--force-assign' || '' }} From d5cdb654437e34800149f9fa553a3f5f7cdec669 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 15:32:34 -0700 Subject: [PATCH 24/46] Switch to pull_request_target for secret access Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index c0a00bfc6dfd..bafe897e77b0 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request: + pull_request_target: types: [opened, synchronize, reopened] workflow_dispatch: inputs: From a2814d31b6f9c9f2db7264704e48d68e841d194c Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 16:03:58 -0700 Subject: [PATCH 25/46] Add explicit permissions for pull request writes and secret access Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign-reviewers.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index bafe897e77b0..d7bf4b7d7428 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -12,7 +12,7 @@ on: description: 'Run in dry-run mode (just print commands)' required: false type: boolean - default: true + default: false force_assign: description: 'Force assign even if reviewers already exist' required: false @@ -21,6 +21,9 @@ on: jobs: assign: runs-on: ubuntu-latest + permissions: + pull-requests: write + contents: read steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 From 26e45462089bcc72a31fbfe5efec952ecba93f64 Mon Sep 17 00:00:00 2001 From: venkywonka <23023424+venkywonka@users.noreply.github.com> Date: Mon, 30 Jun 2025 23:43:28 +0000 Subject: [PATCH 26/46] Switch from pull_request_target to pull_request trigger --- .github/workflows/auto-assign-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index d7bf4b7d7428..efb99765e3ab 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request_target: + pull_request: types: [opened, synchronize, reopened] workflow_dispatch: inputs: From 192a3be579e85ca443f510d0b2f3a9a5542489d0 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 10:35:39 -0700 Subject: [PATCH 27/46] Fix CONTRIBUTING.md to match fork version completely Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- CONTRIBUTING.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 91daf549e08f..f195156a269a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,13 +93,6 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. - * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. - * You can test the assignment script locally with the `--dry-run` flag: - ```bash - GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ - PR_AUTHOR= \ - python3 .github/scripts/assign_reviewers.py --dry-run - ``` * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From 18683e3bf754ba58dcea33680efc68c1b023c963 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 11:51:39 -0700 Subject: [PATCH 28/46] Enhanced assign_reviewers.py with detailed feedback for unmapped files 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 <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 84 ++++++++++++++++--- .../scripts/tests/test_assign_reviewers.py | 39 +++++++-- 2 files changed, 104 insertions(+), 19 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 690e70fc7e20..5272d52e8d3c 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -66,25 +66,43 @@ def load_json(path: str): return json.load(f) -def map_modules(changed_files: list[str], module_paths: dict[str, - str]) -> set[str]: +def map_modules(changed_files: list[str], + module_paths: dict[str, str]) -> tuple[set[str], list[str]]: + """Map changed files to modules and return both modules and unmapped files""" modules: set[str] = set() + unmapped_files: list[str] = [] + for file in changed_files: + mapped = False for prefix, module in module_paths.items(): if file.startswith(prefix): modules.add(module) + mapped = True break - return modules + + if not mapped: + 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) -> list[str]: +def gather_reviewers( + modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None, + existing_reviewers: set[str] | None = None +) -> tuple[list[str], set[str]]: + """Gather reviewers and return both reviewers and modules without owners""" reviewers: set[str] = set() + modules_without_owners: set[str] = set() + for module in modules: - reviewers.update(module_owners.get(module, [])) + owners = module_owners.get(module, []) + if owners: + reviewers.update(owners) + else: + modules_without_owners.add(module) if pr_author: reviewers.discard(pr_author) @@ -93,7 +111,7 @@ def gather_reviewers(modules: set[str], if existing_reviewers: reviewers -= existing_reviewers - return sorted(reviewers) + return sorted(reviewers), modules_without_owners def main() -> None: @@ -139,8 +157,8 @@ def main() -> None: module_paths = load_json(Path(".github") / "module-paths.json") module_owners = load_json(Path(".github") / "module-owners.json") - modules = map_modules(changed_files, module_paths) - reviewers = gather_reviewers( + modules, unmapped_files = map_modules(changed_files, module_paths) + reviewers, modules_without_owners = gather_reviewers( modules, module_owners, pr_author=pr_author, @@ -154,6 +172,23 @@ def main() -> None: print(f"Changed modules: {sorted(modules)}") print(f"Potential reviewers: {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: @@ -176,6 +211,31 @@ def main() -> None: 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) diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py index 0038f980201e..0bc05e49da33 100644 --- a/.github/scripts/tests/test_assign_reviewers.py +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -337,29 +337,54 @@ def test_map_modules_function(self): "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" ] - modules = assign_reviewers.map_modules(changed_files, self.module_paths) + modules, unmapped_files = assign_reviewers.map_modules( + changed_files, self.module_paths) self.assertEqual(modules, {"Generic Runtime", "Documentation"}) + self.assertEqual(unmapped_files, ["unknown/file.txt"]) def test_gather_reviewers_function(self): """Test the pure gather_reviewers function""" modules = {"Generic Runtime", "Documentation"} # Test without exclusions - reviewers = assign_reviewers.gather_reviewers(modules, - self.module_owners) + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners) self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) + self.assertEqual(modules_without_owners, set()) # Test with author exclusion - reviewers = assign_reviewers.gather_reviewers(modules, - self.module_owners, - pr_author="user1") + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners, pr_author="user1") self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) + self.assertEqual(modules_without_owners, set()) # Test with existing reviewers exclusion - reviewers = assign_reviewers.gather_reviewers( + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( modules, self.module_owners, existing_reviewers={"user2", "user9"}) self.assertEqual(set(reviewers), {"user1", "user3"}) + self.assertEqual(modules_without_owners, set()) + + def test_modules_without_owners(self): + """Test modules that have no owners defined""" + modules = {"Generic Runtime", "NonExistent Module"} + + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners) + + self.assertEqual(set(reviewers), {"user1", "user2", "user3"}) + self.assertEqual(modules_without_owners, {"NonExistent Module"}) + + def test_all_files_unmapped(self): + """Test when all files are unmapped""" + changed_files = ["unmapped/file1.txt", "another/file2.py"] + + modules, unmapped_files = assign_reviewers.map_modules( + changed_files, self.module_paths) + + self.assertEqual(modules, set()) + self.assertEqual(set(unmapped_files), + {"unmapped/file1.txt", "another/file2.py"}) @patch('assign_reviewers.load_json') @patch('subprocess.run') From cfb878976a4bf969815b7280f398cc26ffebf4c9 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 14:00:35 -0700 Subject: [PATCH 29/46] fix inconsistent CONTRIBUTING.md Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- CONTRIBUTING.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 91daf549e08f..f195156a269a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,13 +93,6 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. - * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. - * You can test the assignment script locally with the `--dry-run` flag: - ```bash - GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ - PR_AUTHOR= \ - python3 .github/scripts/assign_reviewers.py --dry-run - ``` * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From 518c15b1b64be51ce6232fd0e53751d8d38423f6 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 15:14:47 -0700 Subject: [PATCH 30/46] feat: enhance auto-assign reviewer script with CODEOWNERS support - Add CODEOWNERS-aware file categorization - Refactor script from 333 to 255 lines (23% reduction) - Add comprehensive test coverage including CODEOWNERS test - Improve logging and error handling - All 20 tests passing Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 370 ++++++++++-------- .../scripts/tests/test_assign_reviewers.py | 78 +++- 2 files changed, 270 insertions(+), 178 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 5272d52e8d3c..5e7445c23016 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -7,58 +7,32 @@ from pathlib import Path +def run_gh_command(cmd: list[str]) -> str: + """Run GitHub CLI command and return stdout""" + result = subprocess.run(cmd, capture_output=True, text=True, check=True) + return result.stdout.strip() + + 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( - [ - "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() - } + """Get files changed in PR""" + stdout = run_gh_command([ + "gh", "pr", "view", pr_number, "--json", "files", "--jq", + ".files[].path" + ]) + return [line.strip() for line in stdout.splitlines() if line.strip()] + - return user_reviewers, team_reviewers +def get_existing_reviewers(pr_number: str) -> set[str]: + """Get currently assigned user reviewers for a PR""" + try: + stdout = run_gh_command([ + "gh", "pr", "view", pr_number, "--json", "reviewRequests", "--jq", + "(.reviewRequests // []) | .[] | select(.login) | .login" + ]) + return {line.strip() for line in stdout.splitlines() if line.strip()} except subprocess.CalledProcessError as e: print(f"Warning: Could not fetch existing reviewers: {e}") - return set(), set() + return set() def load_json(path: str): @@ -66,36 +40,80 @@ def load_json(path: str): 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 and return both modules and unmapped files""" - modules: set[str] = set() - unmapped_files: list[str] = [] - - for file in changed_files: - mapped = False +def parse_codeowners() -> list[tuple[str, list[str]]]: + """Parse CODEOWNERS file and return list of (pattern, owners) tuples""" + codeowners_path = Path(".github/CODEOWNERS") + if not codeowners_path.exists(): + return [] + + patterns = [] + with open(codeowners_path, "r", encoding="utf-8") as f: + for line in f: + line = line.strip() + if line and not line.startswith("#"): + parts = line.split() + if len(parts) >= 2: + patterns.append((parts[0], parts[1:])) + return patterns + + +def is_covered_by_codeowners(filepath: str, + patterns: list[tuple[str, list[str]]]) -> bool: + """Check if a file is covered by CODEOWNERS""" + for pattern, _ in patterns: + if pattern.startswith("/"): + pattern = pattern[1:] # Remove leading slash + if filepath.startswith(pattern): + return True + return False + + +def categorize_files( + changed_files: list[str], + module_paths: dict[str, str]) -> tuple[list[str], list[str], list[str]]: + """Categorize files into CODEOWNERS, module-mapped, and unmapped""" + codeowners_patterns = parse_codeowners() + codeowners_files = [] + module_files = [] + unmapped_files = [] + + for filepath in changed_files: + if is_covered_by_codeowners(filepath, codeowners_patterns): + codeowners_files.append(filepath) + else: + # Check module mapping + mapped = False + for prefix in module_paths: + if filepath.startswith(prefix): + module_files.append(filepath) + mapped = True + break + if not mapped: + unmapped_files.append(filepath) + + return codeowners_files, module_files, unmapped_files + + +def get_modules_from_files(files: list[str], + module_paths: dict[str, str]) -> set[str]: + """Get modules from list of files""" + modules = set() + for file in files: for prefix, module in module_paths.items(): if file.startswith(prefix): modules.add(module) - mapped = True break - - if not mapped: - unmapped_files.append(file) - - return modules, unmapped_files + return modules -def gather_reviewers( +def get_reviewers_for_modules( modules: set[str], module_owners: dict[str, list[str]], - *, - pr_author: str | None = None, - existing_reviewers: set[str] | None = None -) -> tuple[list[str], set[str]]: - """Gather reviewers and return both reviewers and modules without owners""" - reviewers: set[str] = set() - modules_without_owners: set[str] = set() + pr_author: str = None, + existing: set[str] = None) -> tuple[list[str], set[str]]: + """Get reviewers for modules, excluding author and existing reviewers""" + reviewers = set() + modules_without_owners = set() for module in modules: owners = module_owners.get(module, []) @@ -106,135 +124,153 @@ def gather_reviewers( if pr_author: reviewers.discard(pr_author) - - # Remove existing reviewers to avoid duplicate assignments - if existing_reviewers: - reviewers -= existing_reviewers + if existing: + reviewers -= existing return sorted(reviewers), modules_without_owners +def log_coverage(label: str, files: list[str], description: str): + """Log file coverage information""" + if files: + print(f"✅ Files covered by {label}: {files}") + print(f" {description}") + + +def log_issues(modules_without_owners: set[str], unmapped_files: list[str]): + """Log configuration issues""" + if modules_without_owners: + print(f"⚠️ Modules with no owners: {sorted(modules_without_owners)}") + print(" Add owner assignments to .github/module-owners.json") + + if unmapped_files: + print(f"⚠️ Files with no coverage: {unmapped_files}") + print( + " Add mappings to .github/module-paths.json or .github/CODEOWNERS" + ) + + +def get_assignment_reason(codeowners_files: list[str], module_files: list[str], + unmapped_files: list[str], modules: set[str], + reviewers: list[str], + modules_without_owners: set[str]) -> str: + """Determine why no reviewers were assigned""" + if not codeowners_files and not module_files and not unmapped_files: + return "No files were changed in this PR" + elif codeowners_files and not module_files and not unmapped_files: + return "All changed files are covered by CODEOWNERS (GitHub will handle reviewer assignment)" + elif not modules and unmapped_files: + return "All mappable files are unmapped → Add module mappings to .github/module-paths.json" + elif modules and not reviewers: + if modules_without_owners: + return "Matched modules have no assigned owners → Add owners to .github/module-owners.json" + else: + return "All potential reviewers are already assigned or excluded" + else: + total_covered = len(codeowners_files) + len(module_files) + total_files = len(codeowners_files) + len(module_files) + len( + unmapped_files) + return f"{total_covered}/{total_files} files have reviewer coverage" + + +def assign_reviewers_to_pr(pr_number: str, reviewers: list[str], + dry_run: bool) -> bool: + """Assign reviewers to PR""" + if not reviewers: + return False + + cmd = ["gh", "pr", "edit", pr_number] + for reviewer in reviewers: + cmd.extend(["--add-reviewer", reviewer]) + + if dry_run: + print(f"🔍 DRY RUN: {' '.join(cmd)}") + return True + + try: + subprocess.run(cmd, check=True) + print( + f"✅ Successfully assigned {len(reviewers)} new reviewer(s) via auto-assignment" + ) + return True + 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) + + 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)" - ) + help="Print commands instead of executing") + parser.add_argument("--force-assign", + action="store_true", + help="Assign reviewers even if some already exist") args = parser.parse_args() + # Get environment variables pr_number = os.environ["PR_NUMBER"] reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) pr_author = os.environ.get("PR_AUTHOR") print(f"Testing PR #{pr_number} with author: {pr_author}") - # 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)}") + try: + # Check existing reviewers + existing_reviewers = get_existing_reviewers(pr_number) + print(f"Existing user reviewers: {sorted(existing_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 + # Skip assignment if reviewers already exist (unless forced) + if existing_reviewers and not args.force_assign: + print( + f"✅ PR already has {len(existing_reviewers)} reviewer(s) assigned. Skipping auto-assignment." + ) + print(" Use --force-assign to assign additional reviewers.") + return - try: + # Get changed files and load configurations 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, modules_without_owners = gather_reviewers( - modules, - module_owners, - pr_author=pr_author, - existing_reviewers= - existing_user_reviewers # Avoid re-assigning existing users - ) + # Categorize files by coverage type + codeowners_files, module_files, unmapped_files = categorize_files( + changed_files, module_paths) + # 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( + modules, module_owners, pr_author, existing_reviewers) + + # Apply reviewer limit if reviewer_limit and len(reviewers) > reviewer_limit: reviewers = random.sample(reviewers, reviewer_limit) print(f"Changed modules: {sorted(modules)}") print(f"Potential reviewers: {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)" - ) + # Log coverage information + log_coverage( + "CODEOWNERS", codeowners_files, + "These files will have reviewers automatically assigned by GitHub's CODEOWNERS mechanism" + ) + log_coverage("module-paths.json", module_files, + "These files are handled by the auto-assignment system") + log_issues(modules_without_owners, unmapped_files) + + # Assign reviewers or explain why not + if assign_reviewers_to_pr(pr_number, reviewers, args.dry_run): + return + + print("✅ No new reviewers to assign") + reason = get_assignment_reason(codeowners_files, module_files, + unmapped_files, modules, reviewers, + modules_without_owners) + print(f" Reason: {reason}") except subprocess.CalledProcessError as e: print(f"❌ Error processing PR: {e}", file=sys.stderr) diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py index 0bc05e49da33..3652e327d1e9 100644 --- a/.github/scripts/tests/test_assign_reviewers.py +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -4,10 +4,12 @@ Tests various scenarios without requiring GitHub API access or tokens. """ +import io import os import subprocess import sys import unittest +from contextlib import redirect_stdout from pathlib import Path from unittest.mock import MagicMock, patch @@ -332,36 +334,44 @@ def test_subprocess_error_handling(self, mock_run, mock_load_json): self.assertEqual(cm.exception.code, 1) def test_map_modules_function(self): - """Test the pure map_modules function""" + """Test the pure get_modules_from_files function""" changed_files = [ "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" ] - modules, unmapped_files = assign_reviewers.map_modules( + # Test the categorize_files function instead + codeowners_files, module_files, unmapped_files = assign_reviewers.categorize_files( changed_files, self.module_paths) - self.assertEqual(modules, {"Generic Runtime", "Documentation"}) + # Verify categorization + self.assertEqual(module_files, + ["cpp/main.cpp", "cpp/utils.h", "docs/README.md"]) self.assertEqual(unmapped_files, ["unknown/file.txt"]) + # Test get_modules_from_files + modules = assign_reviewers.get_modules_from_files( + module_files, self.module_paths) + self.assertEqual(modules, {"Generic Runtime", "Documentation"}) + def test_gather_reviewers_function(self): - """Test the pure gather_reviewers function""" + """Test the pure get_reviewers_for_modules function""" modules = {"Generic Runtime", "Documentation"} # Test without exclusions - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + reviewers, modules_without_owners = assign_reviewers.get_reviewers_for_modules( modules, self.module_owners) self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) self.assertEqual(modules_without_owners, set()) # Test with author exclusion - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + reviewers, modules_without_owners = assign_reviewers.get_reviewers_for_modules( modules, self.module_owners, pr_author="user1") self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) self.assertEqual(modules_without_owners, set()) # Test with existing reviewers exclusion - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners, existing_reviewers={"user2", "user9"}) + reviewers, modules_without_owners = assign_reviewers.get_reviewers_for_modules( + modules, self.module_owners, existing={"user2", "user9"}) self.assertEqual(set(reviewers), {"user1", "user3"}) self.assertEqual(modules_without_owners, set()) @@ -369,7 +379,7 @@ def test_modules_without_owners(self): """Test modules that have no owners defined""" modules = {"Generic Runtime", "NonExistent Module"} - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + reviewers, modules_without_owners = assign_reviewers.get_reviewers_for_modules( modules, self.module_owners) self.assertEqual(set(reviewers), {"user1", "user2", "user3"}) @@ -379,10 +389,11 @@ def test_all_files_unmapped(self): """Test when all files are unmapped""" changed_files = ["unmapped/file1.txt", "another/file2.py"] - modules, unmapped_files = assign_reviewers.map_modules( + codeowners_files, module_files, unmapped_files = assign_reviewers.categorize_files( changed_files, self.module_paths) - self.assertEqual(modules, set()) + self.assertEqual(codeowners_files, []) + self.assertEqual(module_files, []) self.assertEqual(set(unmapped_files), {"unmapped/file1.txt", "another/file2.py"}) @@ -485,6 +496,51 @@ def test_missing_environment_variables(self, mock_run): with patch('sys.argv', ['assign_reviewers.py']): assign_reviewers.main() + @patch('assign_reviewers.load_json') + @patch('assign_reviewers.parse_codeowners') + @patch('subprocess.run') + def test_codeowners_coverage_handling(self, mock_run, mock_parse_codeowners, + mock_load_json): + """Test that files covered by CODEOWNERS are handled correctly""" + # Setup mocks + self.mock_changed_files = "tests/unittest/api_stability/test_llm_api.py\ncpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + # Mock CODEOWNERS patterns + mock_parse_codeowners.return_value = [ + ("/tests/unittest/api_stability/", + ["@NVIDIA/trt-llm-noncommitted-api-review-committee"]), + ("/tensorrt_llm/_torch", ["@NVIDIA/trt-llm-torch-devs"]) + ] + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Capture output + f = io.StringIO() + with redirect_stdout(f): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + output = f.getvalue() + + # Verify reviewers assigned only for cpp file + self.assertTrue(self.assign_reviewers_called) + self.assertTrue( + all(r in ["user1", "user2", "user3"] + for r in self.assigned_reviewers)) + + # Verify CODEOWNERS messaging + self.assertIn("Files covered by CODEOWNERS:", output) + self.assertIn("tests/unittest/api_stability/test_llm_api.py", output) + self.assertIn("CODEOWNERS mechanism", output) + self.assertIn("Files covered by module-paths.json:", output) + self.assertIn("cpp/file1.cpp", output) + if __name__ == "__main__": # Run tests with verbose output From 244035257017df8042677e54563c0c9e9e384206 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 18:30:03 -0700 Subject: [PATCH 31/46] Update auto-assign.yml to use .github/module-owners.json instead of .github/workflows/module-owners.json Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/auto-assign.yml | 2 +- .github/workflows/module-owners.json | 35 ---------------------------- 2 files changed, 1 insertion(+), 36 deletions(-) delete mode 100644 .github/workflows/module-owners.json diff --git a/.github/workflows/auto-assign.yml b/.github/workflows/auto-assign.yml index 5ecb067cc01b..4c484481aaf5 100644 --- a/.github/workflows/auto-assign.yml +++ b/.github/workflows/auto-assign.yml @@ -22,7 +22,7 @@ jobs: const fs = require('fs'); // Read configuration file - const config = JSON.parse(fs.readFileSync('.github/workflows/module-owners.json', 'utf8')); + const config = JSON.parse(fs.readFileSync('.github/module-owners.json', 'utf8')); // Find matching label in config for (const [configLabel, users] of Object.entries(config)) { diff --git a/.github/workflows/module-owners.json b/.github/workflows/module-owners.json deleted file mode 100644 index c799be4a6bb1..000000000000 --- a/.github/workflows/module-owners.json +++ /dev/null @@ -1,35 +0,0 @@ -{ - "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"], - "CI/CD": ["chzblych", "syuoni"], - "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"] -} From b5d0d1f8f2b18db42d2ead83463e2d45bad2714b Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 26 Jun 2025 12:45:57 -0700 Subject: [PATCH 32/46] Add auto-assign feature in github actions for PRs - 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 <23023424+venkywonka@users.noreply.github.com> --- .github/module-paths.json | 30 ++++ .github/scripts/assign_reviewers.py | 186 ++++++++++++++++++++ .github/workflows/auto-assign-reviewers.yml | 41 +++++ .github/workflows/module-owners.json | 23 ++- CONTRIBUTING.md | 7 + 5 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 .github/module-paths.json create mode 100644 .github/scripts/assign_reviewers.py create mode 100644 .github/workflows/auto-assign-reviewers.yml diff --git a/.github/module-paths.json b/.github/module-paths.json new file mode 100644 index 000000000000..0c97a24db17a --- /dev/null +++ b/.github/module-paths.json @@ -0,0 +1,30 @@ +{ + "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/": "CI/CD", + "jenkins/": "CI/CD", + "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" +} diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py new file mode 100644 index 000000000000..a5e8104a6393 --- /dev/null +++ b/.github/scripts/assign_reviewers.py @@ -0,0 +1,186 @@ +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( + [ + "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]) -> set[str]: + modules: set[str] = set() + for file in changed_files: + for prefix, module in module_paths.items(): + if file.startswith(prefix): + modules.add(module) + break + return modules + + +def gather_reviewers(modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None, + existing_reviewers: set[str] | None = None) -> list[str]: + reviewers: set[str] = set() + for module in modules: + reviewers.update(module_owners.get(module, [])) + + if pr_author: + reviewers.discard(pr_author) + + # Remove existing reviewers to avoid duplicate assignments + if existing_reviewers: + reviewers -= existing_reviewers + + return sorted(reviewers) + + +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"] + reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) + pr_author = os.environ.get("PR_AUTHOR") + + print(f"Testing PR #{pr_number} with author: {pr_author}") + + # 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/workflows") / "module-owners.json") + + modules = map_modules(changed_files, module_paths) + reviewers = gather_reviewers( + modules, + module_owners, + pr_author=pr_author, + existing_reviewers= + existing_user_reviewers # Avoid re-assigning existing users + ) + + if reviewer_limit and len(reviewers) > reviewer_limit: + reviewers = random.sample(reviewers, reviewer_limit) + + print(f"Changed modules: {sorted(modules)}") + print(f"Potential reviewers: {reviewers}") + + 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") + + except subprocess.CalledProcessError as e: + print(f"❌ Error processing PR: {e}", file=sys.stderr) + sys.exit(1) + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml new file mode 100644 index 000000000000..efb99765e3ab --- /dev/null +++ b/.github/workflows/auto-assign-reviewers.yml @@ -0,0 +1,41 @@ +name: Auto assign reviewers +on: + pull_request: + types: [opened, synchronize, reopened] + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to test assignment on' + required: true + type: string + dry_run: + description: 'Run in dry-run mode (just print commands)' + required: false + type: boolean + default: false + force_assign: + description: 'Force assign even if reviewers already exist' + required: false + type: boolean + default: false +jobs: + assign: + runs-on: ubuntu-latest + permissions: + pull-requests: write + contents: read + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Assign reviewers + env: + PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login || github.event.inputs.pr_author || '' }} + GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} + REVIEWER_LIMIT: '3' + run: | + python3 .github/scripts/assign_reviewers.py \ + ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} \ + ${{ github.event.inputs.force_assign == 'true' && '--force-assign' || '' }} diff --git a/.github/workflows/module-owners.json b/.github/workflows/module-owners.json index bab5760f012d..c799be4a6bb1 100644 --- a/.github/workflows/module-owners.json +++ b/.github/workflows/module-owners.json @@ -7,10 +7,29 @@ "Speculative Decoding":["yweng0828", "nekorobov", "lfr-0531"], "Customized Kernels":["lowsfer", "PerkzZheng", "jdemouth-nvidia"], "Performance": ["kaiyux", "jiahanc", "hypdeb"], - "Lora/P-tuning":["byshiue", "Naveassaf"], + "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"] + "Installation": ["hchings", "Superjomn", "nv-guomingz", "QiJune"], + "CI/CD": ["chzblych", "syuoni"], + "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"] } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c8995b2ef0c..f978dba153dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,6 +93,13 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. + * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. + * You can test the assignment script locally with the `--dry-run` flag: + ```bash + GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ + PR_AUTHOR= \ + python3 .github/scripts/assign_reviewers.py --dry-run + ``` * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From 8ab55538526d77dbfe8d3a18c7dc4fbddf1ade66 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Tue, 1 Jul 2025 00:39:28 +0000 Subject: [PATCH 33/46] docs: add detailed auto-assign PR reviewer documentation 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 <23023424+venkywonka@users.noreply.github.com> --- CONTRIBUTING.md | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f978dba153dc..09d6ba1300d0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,13 +93,27 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. - * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. - * You can test the assignment script locally with the `--dry-run` flag: - ```bash - GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ - PR_AUTHOR= \ - python3 .github/scripts/assign_reviewers.py --dry-run - ``` + +### Automatic Reviewer Assignment + +Reviewers are automatically assigned to PRs through a GitHub Action that: + +* **Triggers**: Runs automatically when PRs are opened, synchronized, or reopened +* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/workflows/module-owners.json` +* **Respects existing assignments**: Won't assign additional reviewers if any reviewers are already assigned (unless forced) +* **Excludes PR author**: Automatically excludes the PR author from reviewer assignments +* **Limits reviewer count**: Randomly samples up to 3 reviewers if more are eligible (configurable via `REVIEWER_LIMIT`) +* **Coexists with CODEOWNERS**: Works alongside GitHub's CODEOWNERS file (`.github/CODEOWNERS`) which enforces mandatory approvals for specific paths (e.g., API stability tests, release branches) + +The auto-assignment system analyzes all files changed in your PR, maps them to the appropriate code modules, and assigns reviewers from the module owner lists. This ensures domain experts review relevant changes while avoiding over-assignment. + +**Testing the assignment locally**: You can test reviewer assignment with the `--dry-run` flag: + ```bash + GH_TOKEN= PR_NUMBER= PR_AUTHOR= \ + python3 .github/scripts/assign_reviewers.py --dry-run + ``` + +**Manual assignment**: You can also manually trigger reviewer assignment via GitHub's workflow dispatch with options for dry-run mode and force-assignment. * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From eaef7049cbfa804147000397647b2f5852f05410 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Tue, 1 Jul 2025 19:39:36 -0700 Subject: [PATCH 34/46] add comprehensive testing, change to pull_request_target Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .../scripts/tests/test_assign_reviewers.py | 466 ++++++++++++++++++ .github/workflows/auto-assign-reviewers.yml | 2 +- 2 files changed, 467 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/tests/test_assign_reviewers.py diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py new file mode 100644 index 000000000000..0038f980201e --- /dev/null +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -0,0 +1,466 @@ +#!/usr/bin/env python3 +""" +End-to-end tests for assign_reviewers.py script. +Tests various scenarios without requiring GitHub API access or tokens. +""" + +import os +import subprocess +import sys +import unittest +from pathlib import Path +from unittest.mock import MagicMock, patch + +# Add parent directory to path to import the script +sys.path.insert(0, str(Path(__file__).parent.parent)) +import assign_reviewers + + +class TestAssignReviewers(unittest.TestCase): + """Test suite for the assign_reviewers.py script""" + + def setUp(self): + """Set up test fixtures""" + # Sample module-paths.json data + self.module_paths = { + "cpp/": "Generic Runtime", + "tensorrt_llm/": "LLM API/Workflow", + "benchmarks/": "Performance", + "docs/": "Documentation", + "tensorrt_llm/_torch/": "Torch Framework" + } + + # Sample module-owners.json data + self.module_owners = { + "Generic Runtime": ["user1", "user2", "user3"], + "LLM API/Workflow": ["user4", "user5"], + "Performance": ["user6", "user7", "user8"], + "Documentation": ["user9"], + "Torch Framework": ["user10", "user11"] + } + + # Set required environment variables + os.environ["PR_NUMBER"] = "123" + os.environ["PR_AUTHOR"] = "test_author" + os.environ["REVIEWER_LIMIT"] = "3" + + def tearDown(self): + """Clean up environment variables""" + for var in ["PR_NUMBER", "PR_AUTHOR", "REVIEWER_LIMIT"]: + if var in os.environ: + del os.environ[var] + + def _mock_subprocess_run(self, *args, **kwargs): + """Mock subprocess.run based on the command being executed""" + cmd = args[0] + cmd_str = ' '.join(cmd) # Join command for easier matching + + # Mock response for getting changed files + if "pr" in cmd and "view" in cmd and "files" in cmd: + return MagicMock(stdout=self.mock_changed_files, + stderr="", + returncode=0) + + # Mock response for getting existing reviewers (users) + elif "pr" in cmd and "view" in cmd and "reviewRequests" in cmd: + # Check if it's asking for login (users) or name (teams) + if "select(.login)" in cmd_str: + return MagicMock(stdout=self.mock_existing_users, + stderr="", + returncode=0) + elif "select(.name)" in cmd_str: + return MagicMock(stdout=self.mock_existing_teams, + stderr="", + returncode=0) + else: + return MagicMock(stdout="", stderr="", returncode=0) + + # Mock response for assigning reviewers + elif "pr" in cmd and "edit" in cmd: + self.assign_reviewers_called = True + self.assigned_reviewers = [ + cmd[i + 1] for i, arg in enumerate(cmd) + if arg == "--add-reviewer" + ] + return MagicMock(stdout="", stderr="", returncode=0) + + return MagicMock(stdout="", stderr="", returncode=0) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_single_module_changed(self, mock_run, mock_load_json): + """Test PR with files from a single module""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\ncpp/file2.h\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # Should respect limit + self.assertTrue( + all(r in ["user1", "user2", "user3"] + for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_multiple_modules_changed(self, mock_run, mock_load_json): + """Test PR with files from multiple modules""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\ndocs/README.md\nbenchmarks/test.py\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned from multiple modules + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # Should respect limit + # Should have mix of reviewers from different modules + all_possible = [ + "user1", "user2", "user3", "user6", "user7", "user8", "user9" + ] + self.assertTrue(all(r in all_possible for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_no_matching_module(self, mock_run, mock_load_json): + """Test PR with files that don't match any module""" + # Setup mocks + self.mock_changed_files = "unknown/file.txt\nrandom/path.py\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_existing_reviewers_skip(self, mock_run, mock_load_json): + """Test that assignment is skipped when reviewers already exist""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "existing_user1\nexisting_user2\n" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function (without force-assign) + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no new reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_force_assign_with_existing(self, mock_run, mock_load_json): + """Test force-assign flag with existing reviewers""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "user1\n" # user1 is already assigned + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run with force-assign flag + with patch('sys.argv', ['assign_reviewers.py', '--force-assign']): + assign_reviewers.main() + + # Verify reviewers were assigned, excluding already assigned ones + self.assertTrue(self.assign_reviewers_called) + self.assertNotIn("user1", + self.assigned_reviewers) # Should not re-assign + self.assertTrue( + all(r in ["user2", "user3"] for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_pr_author_excluded(self, mock_run, mock_load_json): + """Test that PR author is excluded from reviewers""" + # Setup with PR author as a potential reviewer + os.environ["PR_AUTHOR"] = "user2" + + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify author is not in assigned reviewers + self.assertTrue(self.assign_reviewers_called) + self.assertNotIn("user2", self.assigned_reviewers) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_reviewer_limit_zero(self, mock_run, mock_load_json): + """Test with reviewer limit set to 0 (no limit)""" + os.environ["REVIEWER_LIMIT"] = "0" + + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify all reviewers were assigned (no limit) + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # All from Generic Runtime + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_dry_run_mode(self, mock_run, mock_load_json): + """Test dry-run mode doesn't execute commands""" + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Capture printed output + import io + from contextlib import redirect_stdout + + f = io.StringIO() + with redirect_stdout(f): + with patch('sys.argv', ['assign_reviewers.py', '--dry-run']): + assign_reviewers.main() + + output = f.getvalue() + + # Verify dry run message was printed and no actual assignment + self.assertIn("DRY RUN:", output) + self.assertIn("gh pr edit", output) + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_empty_pr_no_files(self, mock_run, mock_load_json): + """Test PR with no changed files""" + self.mock_changed_files = "" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_subprocess_error_handling(self, mock_run, mock_load_json): + """Test error handling when subprocess commands fail""" + # Mock a subprocess error + mock_run.side_effect = subprocess.CalledProcessError( + 1, ["gh", "pr", "view"]) + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run should exit with error code + with self.assertRaises(SystemExit) as cm: + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + self.assertEqual(cm.exception.code, 1) + + def test_map_modules_function(self): + """Test the pure map_modules function""" + changed_files = [ + "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" + ] + + modules = assign_reviewers.map_modules(changed_files, self.module_paths) + + self.assertEqual(modules, {"Generic Runtime", "Documentation"}) + + def test_gather_reviewers_function(self): + """Test the pure gather_reviewers function""" + modules = {"Generic Runtime", "Documentation"} + + # Test without exclusions + reviewers = assign_reviewers.gather_reviewers(modules, + self.module_owners) + self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) + + # Test with author exclusion + reviewers = assign_reviewers.gather_reviewers(modules, + self.module_owners, + pr_author="user1") + self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) + + # Test with existing reviewers exclusion + reviewers = assign_reviewers.gather_reviewers( + modules, self.module_owners, existing_reviewers={"user2", "user9"}) + self.assertEqual(set(reviewers), {"user1", "user3"}) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_module_with_no_owners(self, mock_run, mock_load_json): + """Test module that has no owners defined""" + # Add a module with no owners + module_owners_with_empty = self.module_owners.copy() + module_owners_with_empty["Empty Module"] = [] + + self.mock_changed_files = "empty/file.txt\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ({ + "empty/": "Empty Module" + } if "module-paths" in str(path) else module_owners_with_empty) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_files_with_special_characters(self, mock_run, mock_load_json): + """Test files with special characters in names""" + self.mock_changed_files = "cpp/file with spaces.cpp\ncpp/file[brackets].h\ncpp/file@special.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned correctly despite special characters + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), 3) + + @patch('assign_reviewers.load_json') + def test_json_file_not_found(self, mock_load_json): + """Test handling of missing JSON configuration files""" + mock_load_json.side_effect = FileNotFoundError( + "module-paths.json not found") + + # Run should exit with error + with self.assertRaises(FileNotFoundError): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_large_reviewer_pool(self, mock_run, mock_load_json): + """Test with a large number of potential reviewers""" + # Create a module with many owners + large_module_owners = self.module_owners.copy() + large_module_owners["Large Module"] = [f"user{i}" for i in range(20)] + + self.mock_changed_files = "large/file.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ({ + "large/": "Large Module" + } if "module-paths" in str(path) else large_module_owners) + + # Run the main function with limit + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify only 3 reviewers were selected (respecting REVIEWER_LIMIT) + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), 3) + self.assertTrue( + all(r in [f"user{i}" for i in range(20)] + for r in self.assigned_reviewers)) + + @patch('subprocess.run') + def test_missing_environment_variables(self, mock_run): + """Test behavior when required environment variables are missing""" + # Remove PR_NUMBER + if "PR_NUMBER" in os.environ: + del os.environ["PR_NUMBER"] + + # Should raise KeyError + with self.assertRaises(KeyError): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + +if __name__ == "__main__": + # Run tests with verbose output + unittest.main(verbosity=2) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index efb99765e3ab..d7bf4b7d7428 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request: + pull_request_target: types: [opened, synchronize, reopened] workflow_dispatch: inputs: From c3ff1b4e004bce538a1a110c8d6cfc23ce900528 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 10:04:42 -0700 Subject: [PATCH 35/46] fix: Major module mapping overhaul for auto-assign reviewers - 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 <23023424+venkywonka@users.noreply.github.com> --- .github/module-owners.json | 39 +++++++++++++++++++++++++++++ .github/module-paths.json | 7 ++++-- .github/scripts/assign_reviewers.py | 3 +-- CONTRIBUTING.md | 8 +++--- 4 files changed, 49 insertions(+), 8 deletions(-) create mode 100644 .github/module-owners.json diff --git a/.github/module-owners.json b/.github/module-owners.json new file mode 100644 index 000000000000..5295e3547ec0 --- /dev/null +++ b/.github/module-owners.json @@ -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"] +} diff --git a/.github/module-paths.json b/.github/module-paths.json index 0c97a24db17a..45699eb7afa0 100644 --- a/.github/module-paths.json +++ b/.github/module-paths.json @@ -7,8 +7,11 @@ "examples/disaggregated/": "Disaggregated Serving", "docs/": "Documentation", "docker/": "Installation", - ".github/": "CI/CD", - "jenkins/": "CI/CD", + ".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", diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index a5e8104a6393..690e70fc7e20 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -137,8 +137,7 @@ def main() -> None: print(f"Changed files: {changed_files}") module_paths = load_json(Path(".github") / "module-paths.json") - module_owners = load_json( - Path(".github/workflows") / "module-owners.json") + module_owners = load_json(Path(".github") / "module-owners.json") modules = map_modules(changed_files, module_paths) reviewers = gather_reviewers( diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 09d6ba1300d0..f195156a269a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,13 +93,16 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. + * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. + * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. + * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. ### Automatic Reviewer Assignment Reviewers are automatically assigned to PRs through a GitHub Action that: * **Triggers**: Runs automatically when PRs are opened, synchronized, or reopened -* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/workflows/module-owners.json` +* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/module-owners.json` * **Respects existing assignments**: Won't assign additional reviewers if any reviewers are already assigned (unless forced) * **Excludes PR author**: Automatically excludes the PR author from reviewer assignments * **Limits reviewer count**: Randomly samples up to 3 reviewers if more are eligible (configurable via `REVIEWER_LIMIT`) @@ -114,9 +117,6 @@ The auto-assignment system analyzes all files changed in your PR, maps them to t ``` **Manual assignment**: You can also manually trigger reviewer assignment via GitHub's workflow dispatch with options for dry-run mode and force-assignment. - * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. - * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. - * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. ### PR Submission Policies From d3d110fa5140a765e9c717bfef2dc539a0f4c498 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 11:51:39 -0700 Subject: [PATCH 36/46] Enhanced assign_reviewers.py with detailed feedback for unmapped files 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 <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 84 ++++++++++++++++--- .../scripts/tests/test_assign_reviewers.py | 39 +++++++-- 2 files changed, 104 insertions(+), 19 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 690e70fc7e20..5272d52e8d3c 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -66,25 +66,43 @@ def load_json(path: str): return json.load(f) -def map_modules(changed_files: list[str], module_paths: dict[str, - str]) -> set[str]: +def map_modules(changed_files: list[str], + module_paths: dict[str, str]) -> tuple[set[str], list[str]]: + """Map changed files to modules and return both modules and unmapped files""" modules: set[str] = set() + unmapped_files: list[str] = [] + for file in changed_files: + mapped = False for prefix, module in module_paths.items(): if file.startswith(prefix): modules.add(module) + mapped = True break - return modules + + if not mapped: + 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) -> list[str]: +def gather_reviewers( + modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None, + existing_reviewers: set[str] | None = None +) -> tuple[list[str], set[str]]: + """Gather reviewers and return both reviewers and modules without owners""" reviewers: set[str] = set() + modules_without_owners: set[str] = set() + for module in modules: - reviewers.update(module_owners.get(module, [])) + owners = module_owners.get(module, []) + if owners: + reviewers.update(owners) + else: + modules_without_owners.add(module) if pr_author: reviewers.discard(pr_author) @@ -93,7 +111,7 @@ def gather_reviewers(modules: set[str], if existing_reviewers: reviewers -= existing_reviewers - return sorted(reviewers) + return sorted(reviewers), modules_without_owners def main() -> None: @@ -139,8 +157,8 @@ def main() -> None: module_paths = load_json(Path(".github") / "module-paths.json") module_owners = load_json(Path(".github") / "module-owners.json") - modules = map_modules(changed_files, module_paths) - reviewers = gather_reviewers( + modules, unmapped_files = map_modules(changed_files, module_paths) + reviewers, modules_without_owners = gather_reviewers( modules, module_owners, pr_author=pr_author, @@ -154,6 +172,23 @@ def main() -> None: print(f"Changed modules: {sorted(modules)}") print(f"Potential reviewers: {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: @@ -176,6 +211,31 @@ def main() -> None: 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) diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py index 0038f980201e..0bc05e49da33 100644 --- a/.github/scripts/tests/test_assign_reviewers.py +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -337,29 +337,54 @@ def test_map_modules_function(self): "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" ] - modules = assign_reviewers.map_modules(changed_files, self.module_paths) + modules, unmapped_files = assign_reviewers.map_modules( + changed_files, self.module_paths) self.assertEqual(modules, {"Generic Runtime", "Documentation"}) + self.assertEqual(unmapped_files, ["unknown/file.txt"]) def test_gather_reviewers_function(self): """Test the pure gather_reviewers function""" modules = {"Generic Runtime", "Documentation"} # Test without exclusions - reviewers = assign_reviewers.gather_reviewers(modules, - self.module_owners) + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners) self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) + self.assertEqual(modules_without_owners, set()) # Test with author exclusion - reviewers = assign_reviewers.gather_reviewers(modules, - self.module_owners, - pr_author="user1") + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners, pr_author="user1") self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) + self.assertEqual(modules_without_owners, set()) # Test with existing reviewers exclusion - reviewers = assign_reviewers.gather_reviewers( + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( modules, self.module_owners, existing_reviewers={"user2", "user9"}) self.assertEqual(set(reviewers), {"user1", "user3"}) + self.assertEqual(modules_without_owners, set()) + + def test_modules_without_owners(self): + """Test modules that have no owners defined""" + modules = {"Generic Runtime", "NonExistent Module"} + + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners) + + self.assertEqual(set(reviewers), {"user1", "user2", "user3"}) + self.assertEqual(modules_without_owners, {"NonExistent Module"}) + + def test_all_files_unmapped(self): + """Test when all files are unmapped""" + changed_files = ["unmapped/file1.txt", "another/file2.py"] + + modules, unmapped_files = assign_reviewers.map_modules( + changed_files, self.module_paths) + + self.assertEqual(modules, set()) + self.assertEqual(set(unmapped_files), + {"unmapped/file1.txt", "another/file2.py"}) @patch('assign_reviewers.load_json') @patch('subprocess.run') From d2cc56167067e77a54c3631aa2f3183bd81e587b Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Tue, 8 Jul 2025 14:06:24 -0700 Subject: [PATCH 37/46] fix 'first-match-wins' bug and test it Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 26 +++++++++++---- .../scripts/tests/test_assign_reviewers.py | 32 +++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 5272d52e8d3c..ed7e0984f99f 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -68,19 +68,31 @@ def load_json(path: str): def map_modules(changed_files: list[str], module_paths: dict[str, str]) -> tuple[set[str], list[str]]: - """Map changed files to modules and return both modules and unmapped files""" + """Map changed files to modules using MOST SPECIFIC (longest) prefix match""" modules: set[str] = set() unmapped_files: list[str] = [] for file in changed_files: - mapped = False + # Find ALL matching prefixes + matches = [] for prefix, module in module_paths.items(): if file.startswith(prefix): - modules.add(module) - mapped = True - break - - if not mapped: + 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] + 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 diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py index 0bc05e49da33..6b815800772b 100644 --- a/.github/scripts/tests/test_assign_reviewers.py +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -386,6 +386,38 @@ def test_all_files_unmapped(self): self.assertEqual(set(unmapped_files), {"unmapped/file1.txt", "another/file2.py"}) + def test_most_specific_module_mapping(self): + """Test that files map to the most specific module match""" + # Create overlapping module paths similar to the real config + overlapping_paths = { + "tensorrt_llm/": "LLM API/Workflow", + "tensorrt_llm/_torch/": "Torch Framework", + "tensorrt_llm/_torch/models/": "Torch Models", + "tensorrt_llm/_torch/models/modeling_llama.py": + "Torch Models Llama", + "tests/": "General Tests", + "tests/integration/": "Integration Tests", + "tests/integration/test_lists/": "Test Configuration", + } + + # Test individual files mapping to most specific modules + test_cases = [ + ("tensorrt_llm/api.py", "LLM API/Workflow"), + ("tensorrt_llm/_torch/utils.py", "Torch Framework"), + ("tensorrt_llm/_torch/models/bert.py", "Torch Models"), + ("tensorrt_llm/_torch/models/modeling_llama.py", + "Torch Models Llama"), + ("tests/unit_test.py", "General Tests"), + ("tests/integration/test_x.py", "Integration Tests"), + ("tests/integration/test_lists/config.json", "Test Configuration"), + ] + + for file, expected_module in test_cases: + modules, _ = assign_reviewers.map_modules([file], overlapping_paths) + self.assertEqual( + modules, {expected_module}, + f"File '{file}' should map to '{expected_module}'") + @patch('assign_reviewers.load_json') @patch('subprocess.run') def test_module_with_no_owners(self, mock_run, mock_load_json): From 2597ad5bd27aa318497dd5997f0c8d70a4cc20ad Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Tue, 8 Jul 2025 14:39:30 -0700 Subject: [PATCH 38/46] implement per-module reviewer assignment for better coverage, test it Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 90 ++- .../scripts/tests/test_assign_reviewers.py | 738 +++++++++--------- .github/workflows/auto-assign-reviewers.yml | 2 +- 3 files changed, 413 insertions(+), 417 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index ed7e0984f99f..3481f3d83c1d 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -99,31 +99,61 @@ def map_modules(changed_files: list[str], def gather_reviewers( - modules: set[str], - module_owners: dict[str, list[str]], - *, - pr_author: str | None = None, - existing_reviewers: set[str] | None = None -) -> tuple[list[str], set[str]]: - """Gather reviewers and return both reviewers and modules without owners""" - reviewers: set[str] = set() + 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 modules: + for module in sorted(modules): # Sort for consistent ordering owners = module_owners.get(module, []) - if owners: - reviewers.update(owners) - else: + 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 - if pr_author: - reviewers.discard(pr_author) + # 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) - # Remove existing reviewers to avoid duplicate assignments - if existing_reviewers: - reviewers -= existing_reviewers + module_assignments[module] = selected + all_reviewers.update(selected) - return sorted(reviewers), modules_without_owners + return sorted(all_reviewers), module_assignments, modules_without_owners def main() -> None: @@ -141,10 +171,11 @@ def main() -> None: args = parser.parse_args() pr_number = os.environ["PR_NUMBER"] - reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) + 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( @@ -170,19 +201,26 @@ def main() -> None: module_owners = load_json(Path(".github") / "module-owners.json") modules, unmapped_files = map_modules(changed_files, module_paths) - reviewers, modules_without_owners = gather_reviewers( + 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 - ) + existing_user_reviewers, # Avoid re-assigning existing users + per_module_limit=per_module_limit) - if reviewer_limit and len(reviewers) > reviewer_limit: - reviewers = random.sample(reviewers, reviewer_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"Changed modules: {sorted(modules)}") - print(f"Potential reviewers: {reviewers}") + print(f"\nFinal reviewers to assign: {reviewers}") # Provide detailed feedback about coverage gaps if unmapped_files: diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py index 6b815800772b..6d8b43c1c326 100644 --- a/.github/scripts/tests/test_assign_reviewers.py +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -7,46 +7,42 @@ import os import subprocess import sys -import unittest from pathlib import Path +from unittest import TestCase from unittest.mock import MagicMock, patch -# Add parent directory to path to import the script +# Add the parent directory to the path so we can import assign_reviewers sys.path.insert(0, str(Path(__file__).parent.parent)) import assign_reviewers -class TestAssignReviewers(unittest.TestCase): +class TestAssignReviewers(TestCase): """Test suite for the assign_reviewers.py script""" def setUp(self): - """Set up test fixtures""" - # Sample module-paths.json data + """Set up test environment""" + # Set required environment variables + os.environ["PR_NUMBER"] = "123" + os.environ["PR_AUTHOR"] = "test-author" + os.environ["PER_MODULE_REVIEWER_LIMIT"] = "2" + + # Set up test data self.module_paths = { "cpp/": "Generic Runtime", - "tensorrt_llm/": "LLM API/Workflow", - "benchmarks/": "Performance", "docs/": "Documentation", - "tensorrt_llm/_torch/": "Torch Framework" } - # Sample module-owners.json data self.module_owners = { "Generic Runtime": ["user1", "user2", "user3"], - "LLM API/Workflow": ["user4", "user5"], - "Performance": ["user6", "user7", "user8"], "Documentation": ["user9"], - "Torch Framework": ["user10", "user11"] + "Module1": ["owner1", "owner2"], + "Module2": ["owner3", "owner4"], + "Module3": [], # No owners } - # Set required environment variables - os.environ["PR_NUMBER"] = "123" - os.environ["PR_AUTHOR"] = "test_author" - os.environ["REVIEWER_LIMIT"] = "3" - def tearDown(self): """Clean up environment variables""" - for var in ["PR_NUMBER", "PR_AUTHOR", "REVIEWER_LIMIT"]: + for var in ["PR_NUMBER", "PR_AUTHOR", "PER_MODULE_REVIEWER_LIMIT"]: if var in os.environ: del os.environ[var] @@ -86,401 +82,377 @@ def _mock_subprocess_run(self, *args, **kwargs): return MagicMock(stdout="", stderr="", returncode=0) - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_single_module_changed(self, mock_run, mock_load_json): - """Test PR with files from a single module""" - # Setup mocks - self.mock_changed_files = "cpp/file1.cpp\ncpp/file2.h\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False - - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + # ========== Unit Tests for Core Functions ========== - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() - - # Verify reviewers were assigned - self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), - 3) # Should respect limit - self.assertTrue( - all(r in ["user1", "user2", "user3"] - for r in self.assigned_reviewers)) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_multiple_modules_changed(self, mock_run, mock_load_json): - """Test PR with files from multiple modules""" - # Setup mocks - self.mock_changed_files = "cpp/file1.cpp\ndocs/README.md\nbenchmarks/test.py\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False - - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + def test_module_mapping_scenarios(self): + """Test various module mapping scenarios with parametrized data""" + test_cases = [ + # Basic mapping with unmapped files + { + "name": + "basic_mapping", + "files": [ + "cpp/main.cpp", "cpp/utils.h", "docs/README.md", + "unknown/file.txt" + ], + "paths": { + "cpp/": "Generic Runtime", + "docs/": "Documentation" + }, + "expected_modules": {"Generic Runtime", "Documentation"}, + "expected_unmapped": ["unknown/file.txt"] + }, + # Most specific module matching + { + "name": "most_specific_single", + "files": ["tensorrt_llm/_torch/models/bert.py"], + "paths": { + "tensorrt_llm/": "LLM API/Workflow", + "tensorrt_llm/_torch/": "Torch Framework", + "tensorrt_llm/_torch/models/": "Torch Models" + }, + "expected_modules": {"Torch Models"}, + "expected_unmapped": [] + }, + # Multiple files with overlapping paths + { + "name": + "multiple_overlapping", + "files": [ + "tensorrt_llm/config.py", "tensorrt_llm/_torch/base.py", + "tensorrt_llm/_torch/models/gpt.py" + ], + "paths": { + "tensorrt_llm/": "LLM API/Workflow", + "tensorrt_llm/_torch/": "Torch Framework", + "tensorrt_llm/_torch/models/": "Torch Models" + }, + "expected_modules": + {"LLM API/Workflow", "Torch Framework", "Torch Models"}, + "expected_unmapped": [] + }, + # All files unmapped + { + "name": "all_unmapped", + "files": ["unmapped/file1.txt", "another/file2.py"], + "paths": { + "cpp/": "Generic Runtime" + }, + "expected_modules": set(), + "expected_unmapped": ["unmapped/file1.txt", "another/file2.py"] + }, + # Exact file match priority + { + "name": "exact_file_match", + "files": ["tests/integration/test_lists/waives.txt"], + "paths": { + "tests/": "General Tests", + "tests/integration/": "Integration Tests", + "tests/integration/test_lists/": "Test Configuration", + "tests/integration/test_lists/waives.txt": "Test Waive List" + }, + "expected_modules": {"Test Waive List"}, + "expected_unmapped": [] + } + ] - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + for case in test_cases: + with self.subTest(case=case["name"]): + modules, unmapped = assign_reviewers.map_modules( + case["files"], case["paths"]) + self.assertEqual(modules, case["expected_modules"], + f"Failed for case: {case['name']}") + self.assertEqual(set(unmapped), set(case["expected_unmapped"]), + f"Failed for case: {case['name']}") + + def test_gather_reviewers_basic(self): + """Test basic gather_reviewers functionality""" + modules = {"Module1", "Module2", "Module3"} + + reviewers, module_assignments, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners, per_module_limit=10) + + # Should get all unique reviewers from modules with owners + expected = ["owner1", "owner2", "owner3", "owner4"] + self.assertEqual(set(reviewers), set(expected)) + + # Check module assignments + self.assertEqual(set(module_assignments["Module1"]), + {"owner1", "owner2"}) + self.assertEqual(set(module_assignments["Module2"]), + {"owner3", "owner4"}) + self.assertEqual(module_assignments["Module3"], []) + self.assertEqual(modules_without_owners, {"Module3"}) + + def test_gather_reviewers_exclusions(self): + """Test reviewer exclusion functionality""" + modules = {"Module1", "Module2"} + + # Test PR author exclusion + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + modules, + self.module_owners, + pr_author="owner1", + per_module_limit=10) + + self.assertNotIn("owner1", reviewers) + self.assertNotIn("owner1", module_assignments["Module1"]) + + # Test existing reviewers exclusion + existing = {"owner2", "owner3"} + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + modules, + self.module_owners, + existing_reviewers=existing, + per_module_limit=10) + + self.assertFalse(any(r in existing for r in reviewers)) + + def test_per_module_reviewer_limit(self): + """Test per-module reviewer limit functionality""" + modules = {"Module1", "Module2"} + module_owners = { + "Module1": ["a", "b", "c", "d", "e"], # 5 owners + "Module2": ["f", "g", "h"], # 3 owners + } - # Verify reviewers were assigned from multiple modules - self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), - 3) # Should respect limit - # Should have mix of reviewers from different modules - all_possible = [ - "user1", "user2", "user3", "user6", "user7", "user8", "user9" - ] - self.assertTrue(all(r in all_possible for r in self.assigned_reviewers)) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_no_matching_module(self, mock_run, mock_load_json): - """Test PR with files that don't match any module""" - # Setup mocks - self.mock_changed_files = "unknown/file.txt\nrandom/path.py\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + modules, module_owners, per_module_limit=2) - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + # Each module should have at most 2 reviewers + self.assertEqual(len(module_assignments["Module1"]), 2) + self.assertEqual(len(module_assignments["Module2"]), 2) + self.assertEqual(len(reviewers), 4) - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + # Verify reviewers are from correct modules + self.assertTrue( + set(module_assignments["Module1"]).issubset( + {"a", "b", "c", "d", "e"})) + self.assertTrue( + set(module_assignments["Module2"]).issubset({"f", "g", "h"})) + + def test_module_reviewer_overlap(self): + """Test handling when reviewers own multiple modules""" + modules = {"Module1", "Module2", "Module3"} + module_owners = { + "Module1": ["shared", "owner1"], + "Module2": ["shared", "owner2"], + "Module3": ["owner3"], + } - # Verify no reviewers were assigned - self.assertFalse(self.assign_reviewers_called) + # Run multiple times to test randomness + total_reviewers_counts = [] + for _ in range(10): + reviewers, _, _ = assign_reviewers.gather_reviewers( + modules, module_owners, per_module_limit=1) + total_reviewers_counts.append(len(reviewers)) + + # Should see both 2 and 3 reviewers due to random selection of 'shared' + self.assertTrue(any(count == 2 for count in total_reviewers_counts)) + self.assertTrue(any(count == 3 for count in total_reviewers_counts)) + + def test_module_coverage_edge_cases(self): + """Test edge cases in module coverage""" + module_owners = { + "Module1": ["alice", "bob"], + "Module2": ["bob"], # Only bob owns this + "Module3": ["charlie"], + } - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_existing_reviewers_skip(self, mock_run, mock_load_json): - """Test that assignment is skipped when reviewers already exist""" - # Setup mocks - self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "existing_user1\nexisting_user2\n" + # Case 1: PR author owns a module entirely + modules = {"Module1", "Module2", "Module3"} + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + modules, module_owners, pr_author="bob", per_module_limit=2) + + self.assertEqual(module_assignments["Module2"], + []) # No eligible reviewers + self.assertEqual(module_assignments["Module1"], ["alice"]) + self.assertEqual(module_assignments["Module3"], ["charlie"]) + + # Case 2: All owners already assigned + existing = {"alice", "charlie"} + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + {"Module1", "Module3"}, + module_owners, + pr_author="bob", + existing_reviewers=existing, + per_module_limit=2) + + self.assertEqual(len(reviewers), 0) + self.assertEqual(module_assignments["Module1"], []) + self.assertEqual(module_assignments["Module3"], []) + + # ========== Integration Tests ========== + + def _run_integration_test(self, + changed_files, + expected_reviewer_count=None, + expected_assigned=True, + pr_author=None, + existing_users="", + extra_assertions=None): + """Helper method to run integration tests with common setup""" + self.mock_changed_files = changed_files + self.mock_existing_users = existing_users self.mock_existing_teams = "" self.assign_reviewers_called = False - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + if pr_author: + os.environ["PR_AUTHOR"] = pr_author - # Run the main function (without force-assign) - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - # Verify no new reviewers were assigned - self.assertFalse(self.assign_reviewers_called) + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_force_assign_with_existing(self, mock_run, mock_load_json): - """Test force-assign flag with existing reviewers""" - # Setup mocks - self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "user1\n" # user1 is already assigned - self.mock_existing_teams = "" - self.assign_reviewers_called = False + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + self.assertEqual(self.assign_reviewers_called, expected_assigned) - # Run with force-assign flag - with patch('sys.argv', ['assign_reviewers.py', '--force-assign']): - assign_reviewers.main() + if expected_reviewer_count is not None and expected_assigned: + self.assertEqual(len(self.assigned_reviewers), + expected_reviewer_count) - # Verify reviewers were assigned, excluding already assigned ones - self.assertTrue(self.assign_reviewers_called) - self.assertNotIn("user1", - self.assigned_reviewers) # Should not re-assign - self.assertTrue( - all(r in ["user2", "user3"] for r in self.assigned_reviewers)) + if extra_assertions and expected_assigned: + extra_assertions(self) - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_pr_author_excluded(self, mock_run, mock_load_json): + def test_single_module_changed(self): + """Test PR with files from a single module""" + self._run_integration_test( + changed_files="cpp/file1.cpp\ncpp/file2.h\n", + expected_reviewer_count=2, + extra_assertions=lambda self: self.assertTrue( + all(r in ["user1", "user2", "user3"] + for r in self.assigned_reviewers))) + + def test_multiple_modules_changed(self): + """Test PR with files from multiple modules""" + self._run_integration_test( + changed_files="cpp/file1.cpp\ndocs/README.md\n", + expected_reviewer_count= + 3, # 2 from Generic Runtime, 1 from Documentation + extra_assertions=lambda self: self.assertTrue( + all(r in ["user1", "user2", "user3", "user9"] + for r in self.assigned_reviewers))) + + def test_no_files_or_unmapped(self): + """Test PR with no files or unmapped files""" + # No files + self._run_integration_test(changed_files="", expected_assigned=False) + + # Unmapped files + self._run_integration_test( + changed_files="unknown/file.txt\nrandom/path.py\n", + expected_assigned=False) + + def test_pr_author_excluded(self): """Test that PR author is excluded from reviewers""" - # Setup with PR author as a potential reviewer - os.environ["PR_AUTHOR"] = "user2" - + self._run_integration_test( + changed_files="cpp/file1.cpp\n", + pr_author="user2", + expected_reviewer_count=2, + extra_assertions=lambda self: self.assertNotIn( + "user2", self.assigned_reviewers)) + + def test_existing_reviewers_behavior(self): + """Test behavior with existing reviewers""" + # Should skip assignment when reviewers exist + self._run_integration_test( + changed_files="cpp/file1.cpp\n", + existing_users="existing_user1\nexisting_user2\n", + expected_assigned=False) + + # Force assign with existing reviewers self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "" + self.mock_existing_users = "user1\n" self.mock_existing_teams = "" self.assign_reviewers_called = False - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - # Verify author is not in assigned reviewers - self.assertTrue(self.assign_reviewers_called) - self.assertNotIn("user2", self.assigned_reviewers) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_reviewer_limit_zero(self, mock_run, mock_load_json): - """Test with reviewer limit set to 0 (no limit)""" - os.environ["REVIEWER_LIMIT"] = "0" - - self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with patch('sys.argv', ['assign_reviewers.py', '--force-assign']): + assign_reviewers.main() - # Verify all reviewers were assigned (no limit) self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), - 3) # All from Generic Runtime - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_dry_run_mode(self, mock_run, mock_load_json): - """Test dry-run mode doesn't execute commands""" - self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + self.assertNotIn("user1", self.assigned_reviewers) - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Capture printed output + def test_special_modes(self): + """Test dry-run and error modes""" + # Dry run mode import io from contextlib import redirect_stdout - f = io.StringIO() - with redirect_stdout(f): - with patch('sys.argv', ['assign_reviewers.py', '--dry-run']): - assign_reviewers.main() - - output = f.getvalue() - - # Verify dry run message was printed and no actual assignment - self.assertIn("DRY RUN:", output) - self.assertIn("gh pr edit", output) - self.assertFalse(self.assign_reviewers_called) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_empty_pr_no_files(self, mock_run, mock_load_json): - """Test PR with no changed files""" - self.mock_changed_files = "" + self.mock_changed_files = "cpp/file1.cpp\n" self.mock_existing_users = "" self.mock_existing_teams = "" self.assign_reviewers_called = False - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() - - # Verify no reviewers were assigned - self.assertFalse(self.assign_reviewers_called) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_subprocess_error_handling(self, mock_run, mock_load_json): - """Test error handling when subprocess commands fail""" - # Mock a subprocess error - mock_run.side_effect = subprocess.CalledProcessError( - 1, ["gh", "pr", "view"]) - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Run should exit with error code - with self.assertRaises(SystemExit) as cm: - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() - - self.assertEqual(cm.exception.code, 1) - - def test_map_modules_function(self): - """Test the pure map_modules function""" - changed_files = [ - "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" - ] - - modules, unmapped_files = assign_reviewers.map_modules( - changed_files, self.module_paths) - - self.assertEqual(modules, {"Generic Runtime", "Documentation"}) - self.assertEqual(unmapped_files, ["unknown/file.txt"]) - - def test_gather_reviewers_function(self): - """Test the pure gather_reviewers function""" - modules = {"Generic Runtime", "Documentation"} - - # Test without exclusions - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners) - self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) - self.assertEqual(modules_without_owners, set()) - - # Test with author exclusion - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners, pr_author="user1") - self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) - self.assertEqual(modules_without_owners, set()) - - # Test with existing reviewers exclusion - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners, existing_reviewers={"user2", "user9"}) - self.assertEqual(set(reviewers), {"user1", "user3"}) - self.assertEqual(modules_without_owners, set()) - - def test_modules_without_owners(self): - """Test modules that have no owners defined""" - modules = {"Generic Runtime", "NonExistent Module"} - - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners) - - self.assertEqual(set(reviewers), {"user1", "user2", "user3"}) - self.assertEqual(modules_without_owners, {"NonExistent Module"}) - - def test_all_files_unmapped(self): - """Test when all files are unmapped""" - changed_files = ["unmapped/file1.txt", "another/file2.py"] - - modules, unmapped_files = assign_reviewers.map_modules( - changed_files, self.module_paths) - - self.assertEqual(modules, set()) - self.assertEqual(set(unmapped_files), - {"unmapped/file1.txt", "another/file2.py"}) - - def test_most_specific_module_mapping(self): - """Test that files map to the most specific module match""" - # Create overlapping module paths similar to the real config - overlapping_paths = { - "tensorrt_llm/": "LLM API/Workflow", - "tensorrt_llm/_torch/": "Torch Framework", - "tensorrt_llm/_torch/models/": "Torch Models", - "tensorrt_llm/_torch/models/modeling_llama.py": - "Torch Models Llama", - "tests/": "General Tests", - "tests/integration/": "Integration Tests", - "tests/integration/test_lists/": "Test Configuration", - } - - # Test individual files mapping to most specific modules - test_cases = [ - ("tensorrt_llm/api.py", "LLM API/Workflow"), - ("tensorrt_llm/_torch/utils.py", "Torch Framework"), - ("tensorrt_llm/_torch/models/bert.py", "Torch Models"), - ("tensorrt_llm/_torch/models/modeling_llama.py", - "Torch Models Llama"), - ("tests/unit_test.py", "General Tests"), - ("tests/integration/test_x.py", "Integration Tests"), - ("tests/integration/test_lists/config.json", "Test Configuration"), - ] - - for file, expected_module in test_cases: - modules, _ = assign_reviewers.map_modules([file], overlapping_paths) - self.assertEqual( - modules, {expected_module}, - f"File '{file}' should map to '{expected_module}'") - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_module_with_no_owners(self, mock_run, mock_load_json): - """Test module that has no owners defined""" - # Add a module with no owners - module_owners_with_empty = self.module_owners.copy() - module_owners_with_empty["Empty Module"] = [] - - self.mock_changed_files = "empty/file.txt\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + f = io.StringIO() + with redirect_stdout(f): + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ({ - "empty/": "Empty Module" - } if "module-paths" in str(path) else module_owners_with_empty) + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with patch('sys.argv', ['assign_reviewers.py', '--dry-run']): + assign_reviewers.main() - # Verify no reviewers were assigned + output = f.getvalue() + self.assertIn("DRY RUN:", output) self.assertFalse(self.assign_reviewers_called) - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_files_with_special_characters(self, mock_run, mock_load_json): - """Test files with special characters in names""" - self.mock_changed_files = "cpp/file with spaces.cpp\ncpp/file[brackets].h\ncpp/file@special.cpp\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + def test_error_handling(self): + """Test various error handling scenarios""" + # Subprocess error + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + mock_run.side_effect = subprocess.CalledProcessError( + 1, ["gh", "pr", "view"]) + mock_load_json.side_effect = lambda path: self.module_paths - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with self.assertRaises(SystemExit) as cm: + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + self.assertEqual(cm.exception.code, 1) - # Verify reviewers were assigned correctly despite special characters - self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), 3) + # Missing JSON file + with patch('assign_reviewers.load_json') as mock_load_json: + mock_load_json.side_effect = FileNotFoundError( + "module-paths.json not found") - @patch('assign_reviewers.load_json') - def test_json_file_not_found(self, mock_load_json): - """Test handling of missing JSON configuration files""" - mock_load_json.side_effect = FileNotFoundError( - "module-paths.json not found") + with self.assertRaises(FileNotFoundError): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() - # Run should exit with error - with self.assertRaises(FileNotFoundError): + # Missing environment variable + del os.environ["PR_NUMBER"] + with self.assertRaises(KeyError): with patch('sys.argv', ['assign_reviewers.py']): assign_reviewers.main() - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_large_reviewer_pool(self, mock_run, mock_load_json): - """Test with a large number of potential reviewers""" - # Create a module with many owners + def test_edge_cases_integration(self): + """Test edge cases in full integration""" + # Files with special characters + self._run_integration_test( + changed_files= + "cpp/file with spaces.cpp\ncpp/file[brackets].h\ncpp/file@special.cpp\n", + expected_reviewer_count=2) + + # Large reviewer pool large_module_owners = self.module_owners.copy() large_module_owners["Large Module"] = [f"user{i}" for i in range(20)] @@ -489,35 +461,21 @@ def test_large_reviewer_pool(self, mock_run, mock_load_json): self.mock_existing_teams = "" self.assign_reviewers_called = False - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ({ - "large/": "Large Module" - } if "module-paths" in str(path) else large_module_owners) + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - # Run the main function with limit - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ({ + "large/": "Large Module" + } if "module-paths" in str(path) else large_module_owners) - # Verify only 3 reviewers were selected (respecting REVIEWER_LIMIT) - self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), 3) - self.assertTrue( - all(r in [f"user{i}" for i in range(20)] - for r in self.assigned_reviewers)) - - @patch('subprocess.run') - def test_missing_environment_variables(self, mock_run): - """Test behavior when required environment variables are missing""" - # Remove PR_NUMBER - if "PR_NUMBER" in os.environ: - del os.environ["PR_NUMBER"] - - # Should raise KeyError - with self.assertRaises(KeyError): with patch('sys.argv', ['assign_reviewers.py']): assign_reviewers.main() + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), 2) # Per-module limit + if __name__ == "__main__": - # Run tests with verbose output + import unittest unittest.main(verbosity=2) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index d7bf4b7d7428..722598499351 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -34,7 +34,7 @@ jobs: PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} PR_AUTHOR: ${{ github.event.pull_request.user.login || github.event.inputs.pr_author || '' }} GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} - REVIEWER_LIMIT: '3' + PER_MODULE_REVIEWER_LIMIT: '2' run: | python3 .github/scripts/assign_reviewers.py \ ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} \ From 0f9a1db0d64c08787e105fca9fe4d4a09abb35b0 Mon Sep 17 00:00:00 2001 From: Venky <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 11:08:15 -0700 Subject: [PATCH 39/46] Update test_pip_install.py Signed-off-by: Venky <23023424+venkywonka@users.noreply.github.com> --- tests/unittest/test_pip_install.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unittest/test_pip_install.py b/tests/unittest/test_pip_install.py index 11288e09cec0..95139da97e7e 100644 --- a/tests/unittest/test_pip_install.py +++ b/tests/unittest/test_pip_install.py @@ -74,3 +74,4 @@ def test_pip_install(): if __name__ == "__main__": test_pip_install() + # touch this file to see what reviewers it auto-assigns From b4b87ea5e1b01b2e97e5d1f1a6257bad5655d902 Mon Sep 17 00:00:00 2001 From: Venky <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 14:02:56 -0700 Subject: [PATCH 40/46] undo Signed-off-by: Venky <23023424+venkywonka@users.noreply.github.com> --- tests/unittest/test_pip_install.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unittest/test_pip_install.py b/tests/unittest/test_pip_install.py index 95139da97e7e..11288e09cec0 100644 --- a/tests/unittest/test_pip_install.py +++ b/tests/unittest/test_pip_install.py @@ -74,4 +74,3 @@ def test_pip_install(): if __name__ == "__main__": test_pip_install() - # touch this file to see what reviewers it auto-assigns From 4bea24af5fa724be09659baba251c3ac8ed51f2a Mon Sep 17 00:00:00 2001 From: Venky <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 14:04:59 -0700 Subject: [PATCH 41/46] Update test_llm_api.py Signed-off-by: Venky <23023424+venkywonka@users.noreply.github.com> --- tests/unittest/api_stability/test_llm_api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unittest/api_stability/test_llm_api.py b/tests/unittest/api_stability/test_llm_api.py index 6960f993286c..283ca016efa5 100644 --- a/tests/unittest/api_stability/test_llm_api.py +++ b/tests/unittest/api_stability/test_llm_api.py @@ -197,3 +197,5 @@ class TestQuantConfig(ApiStabilityTestHarness): class TestCalibConfig(ApiStabilityTestHarness): TEST_CLASS = CalibConfig REFERENCE_FILE = "calib_config.yaml" +# Touch this file and see if the code owners is invoked but should automatically assign the @NVIDIA/trt-llm-noncommitted-api-review-committee as reviewers for the PR. +# and also see if the GitHub action to auto-assign PR reviews respects this assignment and does not override it or do something else. From 979944877d9bfe9ed4cbeb23735df3884a7ff428 Mon Sep 17 00:00:00 2001 From: Venky <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 15:28:57 -0700 Subject: [PATCH 42/46] Update utils.py Signed-off-by: Venky <23023424+venkywonka@users.noreply.github.com> --- examples/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/utils.py b/examples/utils.py index c7556298bc24..8edccfdf70d8 100644 --- a/examples/utils.py +++ b/examples/utils.py @@ -581,3 +581,4 @@ def add_common_args(parser): parser.add_argument('--backend', type=str, default=None) return parser + # touch From 2bc7da19bfc0fa0b4f4aa4826c21101ed91f5680 Mon Sep 17 00:00:00 2001 From: Venky <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 15:37:56 -0700 Subject: [PATCH 43/46] Update pull_request_template.md Signed-off-by: Venky <23023424+venkywonka@users.noreply.github.com> --- .github/pull_request_template.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index b407b6624098..0b532e49ed21 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -18,6 +18,7 @@ Please explain the issue and the solution in short. +# touch ## GitHub Bot Help From 89660f5f33df7947205354ed446815e0d9f2fd7b Mon Sep 17 00:00:00 2001 From: Venky <23023424+venkywonka@users.noreply.github.com> Date: Thu, 3 Jul 2025 18:36:58 -0700 Subject: [PATCH 44/46] undo Signed-off-by: Venky <23023424+venkywonka@users.noreply.github.com> --- .github/pull_request_template.md | 1 - examples/utils.py | 1 - tests/unittest/api_stability/test_llm_api.py | 2 -- 3 files changed, 4 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 0b532e49ed21..b407b6624098 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -18,7 +18,6 @@ Please explain the issue and the solution in short. -# touch ## GitHub Bot Help diff --git a/examples/utils.py b/examples/utils.py index 8edccfdf70d8..c7556298bc24 100644 --- a/examples/utils.py +++ b/examples/utils.py @@ -581,4 +581,3 @@ def add_common_args(parser): parser.add_argument('--backend', type=str, default=None) return parser - # touch diff --git a/tests/unittest/api_stability/test_llm_api.py b/tests/unittest/api_stability/test_llm_api.py index 283ca016efa5..6960f993286c 100644 --- a/tests/unittest/api_stability/test_llm_api.py +++ b/tests/unittest/api_stability/test_llm_api.py @@ -197,5 +197,3 @@ class TestQuantConfig(ApiStabilityTestHarness): class TestCalibConfig(ApiStabilityTestHarness): TEST_CLASS = CalibConfig REFERENCE_FILE = "calib_config.yaml" -# Touch this file and see if the code owners is invoked but should automatically assign the @NVIDIA/trt-llm-noncommitted-api-review-committee as reviewers for the PR. -# and also see if the GitHub action to auto-assign PR reviews respects this assignment and does not override it or do something else. From c5377a86d11ee95fc937f7d88f0aecd0ba92fa77 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Tue, 8 Jul 2025 15:03:44 -0700 Subject: [PATCH 45/46] touch some modules Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- cpp/touch.txt | 0 tensorrt_llm/_torch/models/modeling_deepseekv3.py | 2 +- tensorrt_llm/_torch/models/touch.txt | 0 triton_backend/touch.txt | 0 4 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 cpp/touch.txt create mode 100644 tensorrt_llm/_torch/models/touch.txt create mode 100644 triton_backend/touch.txt diff --git a/cpp/touch.txt b/cpp/touch.txt new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tensorrt_llm/_torch/models/modeling_deepseekv3.py b/tensorrt_llm/_torch/models/modeling_deepseekv3.py index abb73bbdb7b1..3eadcf089b42 100644 --- a/tensorrt_llm/_torch/models/modeling_deepseekv3.py +++ b/tensorrt_llm/_torch/models/modeling_deepseekv3.py @@ -24,7 +24,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. # -------------------------------------------------- - +# touch import math import os import warnings diff --git a/tensorrt_llm/_torch/models/touch.txt b/tensorrt_llm/_torch/models/touch.txt new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/triton_backend/touch.txt b/triton_backend/touch.txt new file mode 100644 index 000000000000..e69de29bb2d1 From cd05c7e00489ec0d3313b412c5368efef3c7282d Mon Sep 17 00:00:00 2001 From: Venky <23023424+venkywonka@users.noreply.github.com> Date: Wed, 9 Jul 2025 19:07:46 -0700 Subject: [PATCH 46/46] Update libnuma_conan.py Signed-off-by: Venky <23023424+venkywonka@users.noreply.github.com> --- cpp/libnuma_conan.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/libnuma_conan.py b/cpp/libnuma_conan.py index 286c3131716e..a0afbeaeb7ed 100644 --- a/cpp/libnuma_conan.py +++ b/cpp/libnuma_conan.py @@ -1,4 +1,5 @@ import os +# touch from conan import ConanFile from conan.errors import ConanInvalidConfiguration