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/45] 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 00000000000..0fea39a79ef --- /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 00000000000..183fdca84ad --- /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 00000000000..50bddbabe37 --- /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 9c8995b2ef0..f978dba153d 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/45] 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 0fea39a79ef..0c97a24db17 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 183fdca84ad..893218bc054 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 bab5760f012..c799be4a6bb 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/45] 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 893218bc054..1a1164a3822 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 50bddbabe37..96c272ed450 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/45] 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 96c272ed450..02b0c6bb8e0 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/45] 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 02b0c6bb8e0..81c1645aa41 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/45] 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 81c1645aa41..02cfbad9fc7 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/45] 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 02cfbad9fc7..01af81099b0 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/45] 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 1a1164a3822..a5e8104a639 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 01af81099b0..c0a00bfc6df 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/45] 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 c0a00bfc6df..bafe897e77b 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/45] 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 bafe897e77b..d7bf4b7d742 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/45] 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 d7bf4b7d742..efb99765e3a 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/45] 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 00000000000..0c97a24db17 --- /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 00000000000..a5e8104a639 --- /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 00000000000..efb99765e3a --- /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 bab5760f012..c799be4a6bb 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 9c8995b2ef0..f978dba153d 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/45] 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 f978dba153d..09d6ba1300d 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/45] 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 00000000000..0038f980201 --- /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 efb99765e3a..d7bf4b7d742 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/45] 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 00000000000..5295e3547ec --- /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 0c97a24db17..45699eb7afa 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 a5e8104a639..690e70fc7e2 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 09d6ba1300d..f195156a269 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/45] 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 00000000000..0fea39a79ef --- /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 00000000000..183fdca84ad --- /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 00000000000..50bddbabe37 --- /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 9c8995b2ef0..f978dba153d 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/45] 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 0fea39a79ef..0c97a24db17 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 183fdca84ad..893218bc054 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 bab5760f012..c799be4a6bb 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/45] 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 893218bc054..1a1164a3822 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 50bddbabe37..96c272ed450 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/45] 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 96c272ed450..02b0c6bb8e0 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/45] 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 02b0c6bb8e0..81c1645aa41 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/45] 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 81c1645aa41..02cfbad9fc7 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/45] 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 02cfbad9fc7..01af81099b0 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/45] 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 1a1164a3822..a5e8104a639 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 01af81099b0..c0a00bfc6df 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/45] 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 c0a00bfc6df..bafe897e77b 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/45] 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 bafe897e77b..d7bf4b7d742 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/45] 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 d7bf4b7d742..efb99765e3a 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/45] 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 91daf549e08..f195156a269 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/45] 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 690e70fc7e2..5272d52e8d3 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 0038f980201..0bc05e49da3 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/45] 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 91daf549e08..f195156a269 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/45] 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 5272d52e8d3..5e7445c2301 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 0bc05e49da3..3652e327d1e 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/45] 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 5ecb067cc01..4c484481aaf 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 c799be4a6bb..00000000000 --- 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/45] 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 00000000000..0c97a24db17 --- /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 00000000000..a5e8104a639 --- /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 00000000000..efb99765e3a --- /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 bab5760f012..c799be4a6bb 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 9c8995b2ef0..f978dba153d 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/45] 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 f978dba153d..09d6ba1300d 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/45] 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 00000000000..0038f980201 --- /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 efb99765e3a..d7bf4b7d742 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/45] 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 00000000000..5295e3547ec --- /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 0c97a24db17..45699eb7afa 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 a5e8104a639..690e70fc7e2 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 09d6ba1300d..f195156a269 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/45] 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 690e70fc7e2..5272d52e8d3 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 0038f980201..0bc05e49da3 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/45] 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 5272d52e8d3..ed7e0984f99 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 0bc05e49da3..6b815800772 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/45] 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 ed7e0984f99..3481f3d83c1 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 6b815800772..6d8b43c1c32 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 d7bf4b7d742..72259849935 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 1887a26a11f375c66ea7d7d9e853e1e10db89960 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 14 Jul 2025 16:08:00 -0700 Subject: [PATCH 39/45] add workflow Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/pr-checklist.md | 14 +++++++++ .github/workflows/pr-checklist.yml | 48 ++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 .github/pr-checklist.md create mode 100644 .github/workflows/pr-checklist.yml diff --git a/.github/pr-checklist.md b/.github/pr-checklist.md new file mode 100644 index 00000000000..4aba3c1f130 --- /dev/null +++ b/.github/pr-checklist.md @@ -0,0 +1,14 @@ +## 🚀 PR Checklist + +Please review and check all applicable items before requesting a review: + +- [ ] PR title follows format: `[JIRA/Issue][type] Description` +- [ ] PR description explains both **what** you're doing and **why** +- [ ] Code conforms to coding conventions (see `CODING_GUIDELINES.md`) +- [ ] Inputs validated and possible nullptr dereferences checked +- [ ] Test cases added for new code +- [ ] All existing tests pass +- [ ] PR and commit messages cleaned up via `git rebase -i` + +--- +**Note:** Check all applicable items and **resolve this comment** when ready for review. diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml new file mode 100644 index 00000000000..173529ea08a --- /dev/null +++ b/.github/workflows/pr-checklist.yml @@ -0,0 +1,48 @@ +name: PR Checklist +on: + pull_request: + types: [opened, reopened] + +permissions: + pull-requests: write + +jobs: + post-checklist: + runs-on: ubuntu-latest + if: github.event.pull_request.draft == false + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Post PR Checklist + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const fs = require('fs'); + const path = require('path'); + + // Read checklist from file + const checklistPath = path.join(process.env.GITHUB_WORKSPACE, '.github', 'pr-checklist.md'); + const checklistContent = fs.readFileSync(checklistPath, 'utf8'); + + // Check if we already posted a checklist comment + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number + }); + + const checklistExists = comments.some(comment => + comment.user.login === 'github-actions[bot]' && + comment.body.includes('PR Checklist') + ); + + if (!checklistExists) { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body: checklistContent + }); + } From 8a4b3654ef5a24e386a70ad0c9b1d6c47b91f510 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 14 Jul 2025 17:06:31 -0700 Subject: [PATCH 40/45] [pr-checklist] fix path spec in workflow Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/pr-checklist.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml index 173529ea08a..1d9d8edc4a2 100644 --- a/.github/workflows/pr-checklist.yml +++ b/.github/workflows/pr-checklist.yml @@ -20,10 +20,9 @@ jobs: github-token: ${{ secrets.GITHUB_TOKEN }} script: | const fs = require('fs'); - const path = require('path'); // Read checklist from file - const checklistPath = path.join(process.env.GITHUB_WORKSPACE, '.github', 'pr-checklist.md'); + const checklistPath = process.env.GITHUB_WORKSPACE + '/.github/pr-checklist.md'; const checklistContent = fs.readFileSync(checklistPath, 'utf8'); // Check if we already posted a checklist comment From a943918e6b4451496d38ca1db812425749ea95b6 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 14 Jul 2025 17:17:51 -0700 Subject: [PATCH 41/45] use pull_request_target Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/pr-checklist.yml | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml index 1d9d8edc4a2..6c08f13c670 100644 --- a/.github/workflows/pr-checklist.yml +++ b/.github/workflows/pr-checklist.yml @@ -1,10 +1,11 @@ name: PR Checklist on: - pull_request: - types: [opened, reopened] + pull_request_target: + types: [opened, reopened, ready_for_review] permissions: pull-requests: write + contents: read jobs: post-checklist: @@ -13,6 +14,9 @@ jobs: steps: - name: Checkout repository uses: actions/checkout@v4 + with: + # Important: checkout the PR head for reading the checklist file + ref: ${{ github.event.pull_request.head.sha }} - name: Post PR Checklist uses: actions/github-script@v7 @@ -44,4 +48,7 @@ jobs: issue_number: context.issue.number, body: checklistContent }); + console.log('Posted PR checklist comment'); + } else { + console.log('Checklist already exists, skipping'); } From 947806e1325bca6450f8998a8fb1099d3b5a4fcc Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 14 Jul 2025 18:08:32 -0700 Subject: [PATCH 42/45] =?UTF-8?q?add=20=F0=9F=91=8D=20enforcement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/pr-checklist.yml | 82 +++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 24 deletions(-) diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml index 6c08f13c670..402918ceee3 100644 --- a/.github/workflows/pr-checklist.yml +++ b/.github/workflows/pr-checklist.yml @@ -1,54 +1,88 @@ name: PR Checklist on: pull_request_target: - types: [opened, reopened, ready_for_review] + types: [opened, reopened, ready_for_review, synchronize] permissions: pull-requests: write - contents: read + statuses: write jobs: - post-checklist: + checklist: runs-on: ubuntu-latest if: github.event.pull_request.draft == false steps: - - name: Checkout repository - uses: actions/checkout@v4 - with: - # Important: checkout the PR head for reading the checklist file - ref: ${{ github.event.pull_request.head.sha }} - - - name: Post PR Checklist + - name: Manage PR Checklist uses: actions/github-script@v7 with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | - const fs = require('fs'); + const pr = context.payload.pull_request; + const checklistBody = `## 🚀 PR Checklist + + Please review all applicable items before requesting a review: + + - [ ] PR title follows format: \`[JIRA/Issue][type] Description\` + - [ ] PR description explains both **what** you're doing and **why** + - [ ] Code conforms to coding conventions (see \`CODING_GUIDELINES.md\`) + - [ ] Inputs validated and possible nullptr dereferences checked + - [ ] Test cases added for new code + - [ ] All existing tests pass + - [ ] PR and commit messages cleaned up via \`git rebase -i\` - // Read checklist from file - const checklistPath = process.env.GITHUB_WORKSPACE + '/.github/pr-checklist.md'; - const checklistContent = fs.readFileSync(checklistPath, 'utf8'); + --- + **Please check the items and react with 👍 to acknowledge you've reviewed the checklist.**`; - // Check if we already posted a checklist comment + // Get all comments const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: context.issue.number + issue_number: pr.number }); - const checklistExists = comments.some(comment => + // Find checklist comment + let checklistComment = comments.find(comment => comment.user.login === 'github-actions[bot]' && comment.body.includes('PR Checklist') ); - if (!checklistExists) { - await github.rest.issues.createComment({ + // Post checklist if not exists + if (!checklistComment && ['opened', 'reopened', 'ready_for_review'].includes(context.payload.action)) { + const { data: newComment } = await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: context.issue.number, - body: checklistContent + issue_number: pr.number, + body: checklistBody }); - console.log('Posted PR checklist comment'); - } else { - console.log('Checklist already exists, skipping'); + checklistComment = newComment; + console.log('Posted PR checklist'); } + + if (!checklistComment) { + console.log('No checklist comment found'); + return; + } + + // Check for thumbs up reactions + const { data: reactions } = await github.rest.reactions.listForIssueComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: checklistComment.id + }); + + const hasThumbsUp = reactions.some(reaction => reaction.content === '+1'); + + // Update commit status + await github.rest.repos.createCommitStatus({ + owner: context.repo.owner, + repo: context.repo.repo, + sha: pr.head.sha, + state: hasThumbsUp ? 'success' : 'pending', + context: 'PR Checklist', + description: hasThumbsUp + ? '✅ Checklist acknowledged' + : '⏳ Please 👍 the checklist to acknowledge', + target_url: checklistComment.html_url + }); + + console.log(`Status: ${hasThumbsUp ? 'success' : 'pending'}`); From 53d1941e4988247752a5ff7412ccb8e366a8f339 Mon Sep 17 00:00:00 2001 From: Venky <23023424+venkywonka@users.noreply.github.com> Date: Mon, 14 Jul 2025 16:39:59 -0700 Subject: [PATCH 43/45] Update CONTRIBUTING.md Signed-off-by: Venky <23023424+venkywonka@users.noreply.github.com> --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f195156a269..b4cb77e5272 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -92,10 +92,11 @@ 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. + * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review. * 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. + * An automatic comment will be generated with a checklist, that needs to be checked and resolved for merge. ### Automatic Reviewer Assignment From 33dab6aa0ff8993c3a2e552fc16e10ad98dedf99 Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 14 Jul 2025 18:08:32 -0700 Subject: [PATCH 44/45] =?UTF-8?q?add=20=F0=9F=91=8D=20enforcement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/workflows/pr-checklist.yml | 82 +++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 24 deletions(-) diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml index 6c08f13c670..1e02a7a0878 100644 --- a/.github/workflows/pr-checklist.yml +++ b/.github/workflows/pr-checklist.yml @@ -1,54 +1,88 @@ name: PR Checklist on: pull_request_target: - types: [opened, reopened, ready_for_review] + types: [opened, reopened, ready_for_review, synchronize] permissions: pull-requests: write - contents: read + statuses: write jobs: - post-checklist: + checklist: runs-on: ubuntu-latest if: github.event.pull_request.draft == false steps: - - name: Checkout repository - uses: actions/checkout@v4 - with: - # Important: checkout the PR head for reading the checklist file - ref: ${{ github.event.pull_request.head.sha }} - - - name: Post PR Checklist + - name: Manage PR Checklist uses: actions/github-script@v7 with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | - const fs = require('fs'); + const pr = context.payload.pull_request; + const checklistBody = `## 🚀 PR Checklist + + Please review all applicable items before requesting a review: + + - [ ] PR title follows format: \`[JIRA/Issue][type] Description\` + - [ ] PR description explains both **what** you're doing and **why** + - [ ] Code conforms to coding conventions (see \`CODING_GUIDELINES.md\`) + - [ ] Inputs validated and possible nullptr dereferences checked + - [ ] Test cases added for new code + - [ ] All existing tests pass + - [ ] PR and commit messages cleaned up via \`git rebase -i\` - // Read checklist from file - const checklistPath = process.env.GITHUB_WORKSPACE + '/.github/pr-checklist.md'; - const checklistContent = fs.readFileSync(checklistPath, 'utf8'); + --- + **Please check the items and react with 👍 to acknowledge you've reviewed the checklist.**`; - // Check if we already posted a checklist comment + // Get all comments const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: context.issue.number + issue_number: pr.number }); - const checklistExists = comments.some(comment => + // Find checklist comment + let checklistComment = comments.find(comment => comment.user.login === 'github-actions[bot]' && comment.body.includes('PR Checklist') ); - if (!checklistExists) { - await github.rest.issues.createComment({ + // Post checklist if not exists + if (!checklistComment && ['opened', 'reopened', 'ready_for_review'].includes(context.payload.action)) { + const { data: newComment } = await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: context.issue.number, - body: checklistContent + issue_number: pr.number, + body: checklistBody }); - console.log('Posted PR checklist comment'); - } else { - console.log('Checklist already exists, skipping'); + checklistComment = newComment; + console.log('Posted PR checklist'); } + + if (!checklistComment) { + console.log('No checklist comment found'); + return; + } + + // Check for thumbs up reactions + const { data: reactions } = await github.rest.reactions.listForIssueComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: checklistComment.id + }); + + const hasThumbsUp = reactions.some(reaction => reaction.content === '+1'); + + // Update commit status (using plain text to avoid Unicode issues) + await github.rest.repos.createCommitStatus({ + owner: context.repo.owner, + repo: context.repo.repo, + sha: pr.head.sha, + state: hasThumbsUp ? 'success' : 'pending', + context: 'PR Checklist', + description: hasThumbsUp + ? 'Checklist acknowledged' + : 'Please acknowledge the checklist with thumbs up', + target_url: checklistComment.html_url + }); + + console.log(`Status: ${hasThumbsUp ? 'success' : 'pending'}`); From 0efbd8302709616912f1687e708a4073ef82811e Mon Sep 17 00:00:00 2001 From: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> Date: Mon, 14 Jul 2025 19:04:01 -0700 Subject: [PATCH 45/45] enforce check instead of reaction Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/pr-checklist.md | 8 +- .github/workflows/pr-checklist.yml | 131 ++++++++++++++++++++++++----- 2 files changed, 111 insertions(+), 28 deletions(-) diff --git a/.github/pr-checklist.md b/.github/pr-checklist.md index 4aba3c1f130..28014b80cba 100644 --- a/.github/pr-checklist.md +++ b/.github/pr-checklist.md @@ -1,14 +1,12 @@ ## 🚀 PR Checklist -Please review and check all applicable items before requesting a review: - -- [ ] PR title follows format: `[JIRA/Issue][type] Description` +- [ ] PR title follows format: `[type] Description` - [ ] PR description explains both **what** you're doing and **why** - [ ] Code conforms to coding conventions (see `CODING_GUIDELINES.md`) -- [ ] Inputs validated and possible nullptr dereferences checked - [ ] Test cases added for new code - [ ] All existing tests pass - [ ] PR and commit messages cleaned up via `git rebase -i` --- -**Note:** Check all applicable items and **resolve this comment** when ready for review. +**Please ✅ check the below item to confirm you've reviewed the checklist when ready for review!.** +- [ ] **I have reviewed the above checklist and addressed all applicable items** diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml index 1e02a7a0878..02b005a1cf1 100644 --- a/.github/workflows/pr-checklist.yml +++ b/.github/workflows/pr-checklist.yml @@ -2,6 +2,14 @@ name: PR Checklist on: pull_request_target: types: [opened, reopened, ready_for_review, synchronize] + issue_comment: + types: [created, edited] + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to check' + required: true + type: string permissions: pull-requests: write @@ -10,34 +18,65 @@ permissions: jobs: checklist: runs-on: ubuntu-latest - if: github.event.pull_request.draft == false + if: github.event_name == 'workflow_dispatch' || github.event.pull_request.draft == false || github.event.issue.pull_request steps: - name: Manage PR Checklist uses: actions/github-script@v7 with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | - const pr = context.payload.pull_request; - const checklistBody = `## 🚀 PR Checklist + // Get PR information based on event type + let pr, prNumber; + if (context.eventName === 'pull_request_target') { + pr = context.payload.pull_request; + prNumber = pr.number; + } else if (context.eventName === 'issue_comment') { + // For issue_comment events, get PR info from the issue + if (!context.payload.issue.pull_request) { + console.log('Comment is not on a PR, skipping'); + return; + } + prNumber = context.payload.issue.number; + const { data: prData } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber + }); + pr = prData; + } else if (context.eventName === 'workflow_dispatch') { + // For manual dispatch, get PR info from input + prNumber = parseInt(context.payload.inputs.pr_number); + const { data: prData } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber + }); + pr = prData; + } else { + console.log('Unexpected event type:', context.eventName); + return; + } - Please review all applicable items before requesting a review: + console.log(`Processing PR #${prNumber}`); - - [ ] PR title follows format: \`[JIRA/Issue][type] Description\` + const checklistBody = `## 🚀 PR Checklist + + - [ ] PR title follows format: \`[type] Description\` - [ ] PR description explains both **what** you're doing and **why** - [ ] Code conforms to coding conventions (see \`CODING_GUIDELINES.md\`) - - [ ] Inputs validated and possible nullptr dereferences checked - [ ] Test cases added for new code - [ ] All existing tests pass - [ ] PR and commit messages cleaned up via \`git rebase -i\` --- - **Please check the items and react with 👍 to acknowledge you've reviewed the checklist.**`; + **Please ✅ check the below item to confirm you've reviewed the checklist when ready for review!.** + - [ ] **I have reviewed the above checklist and addressed all applicable items**`; // Get all comments const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: pr.number + issue_number: prNumber }); // Find checklist comment @@ -47,11 +86,13 @@ jobs: ); // Post checklist if not exists - if (!checklistComment && ['opened', 'reopened', 'ready_for_review'].includes(context.payload.action)) { + if (!checklistComment && + (context.eventName === 'workflow_dispatch' || + ['opened', 'reopened', 'ready_for_review'].includes(context.payload.action))) { const { data: newComment } = await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: pr.number, + issue_number: prNumber, body: checklistBody }); checklistComment = newComment; @@ -63,26 +104,70 @@ jobs: return; } - // Check for thumbs up reactions - const { data: reactions } = await github.rest.reactions.listForIssueComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: checklistComment.id - }); + // Parse the checklist comment to count checked items + const checkboxRegex = /- \[([ x])\]/gi; + const matches = [...checklistComment.body.matchAll(checkboxRegex)]; + const totalItems = matches.length; + const checkedItems = matches.filter(match => match[1].toLowerCase() === 'x').length; + + console.log(`Checklist status: ${checkedItems}/${totalItems} items checked`); - const hasThumbsUp = reactions.some(reaction => reaction.content === '+1'); + // Check if the acknowledgment checkbox (last one) is checked + const lastCheckbox = matches[matches.length - 1]; + const isAcknowledged = lastCheckbox && lastCheckbox[1].toLowerCase() === 'x'; - // Update commit status (using plain text to avoid Unicode issues) + // Determine status based on checked items + let state, description; + if (!isAcknowledged) { + state = 'pending'; + description = `Please acknowledge the checklist`; + } else { + state = 'success'; + description = `Checklist acknowledged`; + } + + // Update commit status await github.rest.repos.createCommitStatus({ owner: context.repo.owner, repo: context.repo.repo, sha: pr.head.sha, - state: hasThumbsUp ? 'success' : 'pending', + state: state, context: 'PR Checklist', - description: hasThumbsUp - ? 'Checklist acknowledged' - : 'Please acknowledge the checklist with thumbs up', + description: description, target_url: checklistComment.html_url }); - console.log(`Status: ${hasThumbsUp ? 'success' : 'pending'}`); + console.log(`Status: ${state} - ${description}`); + + // Add a helpful label based on checklist status + const labels = { + complete: 'checklist-complete', + incomplete: 'checklist-incomplete' + }; + + try { + // Remove both labels first + for (const label of Object.values(labels)) { + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: label + }); + } catch (e) { + // Label might not exist, that's ok + } + } + + // Add appropriate label + const labelToAdd = isAcknowledged ? labels.complete : labels.incomplete; + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: [labelToAdd] + }); + } catch (e) { + console.log('Could not update labels:', e.message); + }