From 0b7ced97c6640fb9d4e667117438f71658f66935 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 1/7] Add auto-assign feature in github actions for PRs - Add logic to check existing reviewers before assignment - Skip auto-assignment if reviewers already exist (unless --force-assign) - Improve logging and error handling with status messages - Add force-assign option for manual override - Switch back to pull_request trigger for proper access - Fix workflow parameter handling for force_assign option Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/module-paths.json | 30 ++++ .github/scripts/assign_reviewers.py | 186 ++++++++++++++++++++ .github/workflows/auto-assign-reviewers.yml | 41 +++++ .github/workflows/module-owners.json | 23 ++- CONTRIBUTING.md | 7 + 5 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 .github/module-paths.json create mode 100644 .github/scripts/assign_reviewers.py create mode 100644 .github/workflows/auto-assign-reviewers.yml diff --git a/.github/module-paths.json b/.github/module-paths.json new file mode 100644 index 000000000000..0c97a24db17a --- /dev/null +++ b/.github/module-paths.json @@ -0,0 +1,30 @@ +{ + "cpp/": "Generic Runtime", + "triton_backend/": "Triton Backend", + "tensorrt_llm/_torch/peft/": "Lora/P-tuning", + "tensorrt_llm/": "LLM API/Workflow", + "benchmarks/": "Performance", + "examples/disaggregated/": "Disaggregated Serving", + "docs/": "Documentation", + "docker/": "Installation", + ".github/": "CI/CD", + "jenkins/": "CI/CD", + "tensorrt_llm/_torch/": "Torch Framework", + "tensorrt_llm/_torch/attention_backend/": "Torch Attention Backend", + "tensorrt_llm/_torch/auto_deploy/": "Torch AutoDeploy", + "tensorrt_llm/_torch/compilation/": "Torch Compilation", + "tensorrt_llm/_torch/custom_ops/": "Torch Custom Ops", + "tensorrt_llm/_torch/distributed/": "Torch Distributed", + "tensorrt_llm/_torch/pyexecutor/": "Torch PyExecutor", + "tensorrt_llm/_torch/speculative/": "Torch Speculative", + "tensorrt_llm/autotuner.py": "Autotuner", + "tensorrt_llm/pipeline_interface.py": "Pipeline Interface", + "tensorrt_llm/_torch/models/": "Torch Models", + "tensorrt_llm/_torch/models/modeling_deepseekv3.py": "Torch Models DeepSeekV3", + "tensorrt_llm/_torch/models/modeling_llama.py": "Torch Models Llama", + "tensorrt_llm/_torch/modules/": "Torch Modules", + "tensorrt_llm/_torch/modules/attention.py": "Torch Modules Attention", + "tensorrt_llm/_torch/modules/fused_moe.py": "Torch Modules Fused MOE", + "tests/unittest/_torch/": "Torch Tests", + "examples/pytorch/": "PyTorch Examples" +} diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py new file mode 100644 index 000000000000..a5e8104a6393 --- /dev/null +++ b/.github/scripts/assign_reviewers.py @@ -0,0 +1,186 @@ +import argparse +import json +import os +import random +import subprocess +import sys +from pathlib import Path + + +def get_pr_changed_files(pr_number: str) -> list[str]: + """Get files changed in PR using GitHub CLI (more reliable than git diff)""" + result = subprocess.run( + [ + "gh", "pr", "view", pr_number, "--json", "files", "--jq", + ".files[].path" + ], + capture_output=True, + text=True, + check=True, + ) + return [line.strip() for line in result.stdout.splitlines() if line.strip()] + + +def get_existing_reviewers(pr_number: str) -> tuple[set[str], set[str]]: + """Get currently assigned reviewers (users and teams) for a PR""" + try: + # Get user reviewers + user_result = subprocess.run( + [ + "gh", "pr", "view", pr_number, "--json", "reviewRequests", + "--jq", + "(.reviewRequests // []) | .[] | select(.login) | .login" + ], + capture_output=True, + text=True, + check=True, + ) + user_reviewers = { + line.strip() + for line in user_result.stdout.splitlines() if line.strip() + } + + # Get team reviewers + team_result = subprocess.run( + [ + "gh", "pr", "view", pr_number, "--json", "reviewRequests", + "--jq", "(.reviewRequests // []) | .[] | select(.name) | .name" + ], + capture_output=True, + text=True, + check=True, + ) + team_reviewers = { + line.strip() + for line in team_result.stdout.splitlines() if line.strip() + } + + return user_reviewers, team_reviewers + except subprocess.CalledProcessError as e: + print(f"Warning: Could not fetch existing reviewers: {e}") + return set(), set() + + +def load_json(path: str): + with open(path, "r", encoding="utf-8") as f: + return json.load(f) + + +def map_modules(changed_files: list[str], module_paths: dict[str, + str]) -> set[str]: + modules: set[str] = set() + for file in changed_files: + for prefix, module in module_paths.items(): + if file.startswith(prefix): + modules.add(module) + break + return modules + + +def gather_reviewers(modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None, + existing_reviewers: set[str] | None = None) -> list[str]: + reviewers: set[str] = set() + for module in modules: + reviewers.update(module_owners.get(module, [])) + + if pr_author: + reviewers.discard(pr_author) + + # Remove existing reviewers to avoid duplicate assignments + if existing_reviewers: + reviewers -= existing_reviewers + + return sorted(reviewers) + + +def main() -> None: + parser = argparse.ArgumentParser( + description="Assign reviewers based on changed modules") + parser.add_argument("--dry-run", + action="store_true", + help="Print the gh command instead of executing") + parser.add_argument( + "--force-assign", + action="store_true", + help= + "Assign reviewers even if some already exist (default: only assign if no reviewers)" + ) + args = parser.parse_args() + + pr_number = os.environ["PR_NUMBER"] + reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) + pr_author = os.environ.get("PR_AUTHOR") + + print(f"Testing PR #{pr_number} with author: {pr_author}") + + # Check existing reviewers + existing_user_reviewers, existing_team_reviewers = get_existing_reviewers( + pr_number) + total_existing = len(existing_user_reviewers) + len(existing_team_reviewers) + + print(f"Existing user reviewers: {sorted(existing_user_reviewers)}") + print(f"Existing team reviewers: {sorted(existing_team_reviewers)}") + + # Skip assignment if reviewers already exist (unless forced) + if total_existing > 0 and not args.force_assign: + print( + f"✅ PR already has {total_existing} reviewer(s) assigned. Skipping auto-assignment." + ) + print(" Use --force-assign to assign additional reviewers.") + return + + try: + changed_files = get_pr_changed_files(pr_number) + print(f"Changed files: {changed_files}") + + module_paths = load_json(Path(".github") / "module-paths.json") + module_owners = load_json( + Path(".github/workflows") / "module-owners.json") + + modules = map_modules(changed_files, module_paths) + reviewers = gather_reviewers( + modules, + module_owners, + pr_author=pr_author, + existing_reviewers= + existing_user_reviewers # Avoid re-assigning existing users + ) + + if reviewer_limit and len(reviewers) > reviewer_limit: + reviewers = random.sample(reviewers, reviewer_limit) + + print(f"Changed modules: {sorted(modules)}") + print(f"Potential reviewers: {reviewers}") + + if reviewers: + cmd = ["gh", "pr", "edit", pr_number] + for reviewer in reviewers: + cmd.extend(["--add-reviewer", reviewer]) + + if args.dry_run: + print(f"🔍 DRY RUN: {' '.join(cmd)}") + else: + try: + subprocess.run(cmd, check=True) + print( + f"✅ Successfully assigned {len(reviewers)} new reviewer(s)" + ) + except subprocess.CalledProcessError as e: + print(f"❌ Failed to add reviewers: {e}", file=sys.stderr) + print( + " This might be due to permissions or invalid usernames" + ) + sys.exit(1) + else: + print("✅ No new reviewers to assign") + + except subprocess.CalledProcessError as e: + print(f"❌ Error processing PR: {e}", file=sys.stderr) + sys.exit(1) + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml new file mode 100644 index 000000000000..efb99765e3ab --- /dev/null +++ b/.github/workflows/auto-assign-reviewers.yml @@ -0,0 +1,41 @@ +name: Auto assign reviewers +on: + pull_request: + types: [opened, synchronize, reopened] + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to test assignment on' + required: true + type: string + dry_run: + description: 'Run in dry-run mode (just print commands)' + required: false + type: boolean + default: false + force_assign: + description: 'Force assign even if reviewers already exist' + required: false + type: boolean + default: false +jobs: + assign: + runs-on: ubuntu-latest + permissions: + pull-requests: write + contents: read + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Assign reviewers + env: + PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login || github.event.inputs.pr_author || '' }} + GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} + REVIEWER_LIMIT: '3' + run: | + python3 .github/scripts/assign_reviewers.py \ + ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} \ + ${{ github.event.inputs.force_assign == 'true' && '--force-assign' || '' }} diff --git a/.github/workflows/module-owners.json b/.github/workflows/module-owners.json index bab5760f012d..c799be4a6bb1 100644 --- a/.github/workflows/module-owners.json +++ b/.github/workflows/module-owners.json @@ -7,10 +7,29 @@ "Speculative Decoding":["yweng0828", "nekorobov", "lfr-0531"], "Customized Kernels":["lowsfer", "PerkzZheng", "jdemouth-nvidia"], "Performance": ["kaiyux", "jiahanc", "hypdeb"], - "Lora/P-tuning":["byshiue", "Naveassaf"], + "Lora/P-tuning":["byshiue", "shaharmor98"], "Disaggregated Serving":["Shixiaowei02", "joyang-nv", "chuangz0", "schetlur-nv"], "Documentation":["nv-guomingz"], "Sampling": ["dcampora", "lfr-0531", "Naveassaf", "syuoni", "yweng0828"], "Memory": ["litaotju", "peaceh-nv"], - "Installation": ["hchings", "Superjomn", "nv-guomingz", "QiJune"] + "Installation": ["hchings", "Superjomn", "nv-guomingz", "QiJune"], + "CI/CD": ["chzblych", "syuoni"], + "Torch Framework": ["QiJune", "hlu1"], + "Torch Attention Backend": ["yuxianq", "hlu1"], + "Torch AutoDeploy": ["lucaslie", "suyoggupta"], + "Torch Compilation": ["litaotju", "yizhang-nv", "liji-nv"], + "Torch Custom Ops": ["yizhang-nv"], + "Torch Distributed": ["yilin-void", "yuxianq", "hyukn", "yizhang-nv", "hlu1"], + "Torch PyExecutor": ["dongxuy04", "funatiq", "dcampora", "HuiGao-NV"], + "Torch Speculative": ["lfr-0531", "mikeiovine"], + "Autotuner": ["hyukn", "litaotju"], + "Pipeline Interface": ["amukkara", "chang-l"], + "Torch Models": ["QiJune", "hlu1"], + "Torch Models DeepSeekV3": ["hlu1", "zongfeijing"], + "Torch Models Llama": ["chang-l", "mikeiovine"], + "Torch Modules": ["QiJune", "hlu1"], + "Torch Modules Attention": ["yuxianq", "hlu1"], + "Torch Modules Fused MOE": ["hlu1", "dongxuy04", "zongfeijing", "HuiGao-NV"], + "Torch Tests": ["QiJune", "hlu1"], + "PyTorch Examples": ["QiJune", "hlu1"] } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c8995b2ef0c..f978dba153dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,6 +93,13 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. + * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. + * You can test the assignment script locally with the `--dry-run` flag: + ```bash + GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ + PR_AUTHOR= \ + python3 .github/scripts/assign_reviewers.py --dry-run + ``` * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From 4c465e21e4e47014ba07e34a2bbb9740690da9b7 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 2/7] docs: add detailed auto-assign PR reviewer documentation Add comprehensive documentation about the GitHub action for automatic PR reviewer assignment, including its behavior with CODEOWNERS, module-based assignment, and existing reviewer respect. Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- CONTRIBUTING.md | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f978dba153dc..09d6ba1300d0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,13 +93,27 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. - * Reviewers are automatically requested based on the modules affected in the PR. Module paths are defined in `.github/module-paths.json` and ownership in `.github/workflows/module-owners.json`. - * You can test the assignment script locally with the `--dry-run` flag: - ```bash - GH_TOKEN= BASE_SHA= HEAD_SHA= PR_NUMBER= \ - PR_AUTHOR= \ - python3 .github/scripts/assign_reviewers.py --dry-run - ``` + +### Automatic Reviewer Assignment + +Reviewers are automatically assigned to PRs through a GitHub Action that: + +* **Triggers**: Runs automatically when PRs are opened, synchronized, or reopened +* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/workflows/module-owners.json` +* **Respects existing assignments**: Won't assign additional reviewers if any reviewers are already assigned (unless forced) +* **Excludes PR author**: Automatically excludes the PR author from reviewer assignments +* **Limits reviewer count**: Randomly samples up to 3 reviewers if more are eligible (configurable via `REVIEWER_LIMIT`) +* **Coexists with CODEOWNERS**: Works alongside GitHub's CODEOWNERS file (`.github/CODEOWNERS`) which enforces mandatory approvals for specific paths (e.g., API stability tests, release branches) + +The auto-assignment system analyzes all files changed in your PR, maps them to the appropriate code modules, and assigns reviewers from the module owner lists. This ensures domain experts review relevant changes while avoiding over-assignment. + +**Testing the assignment locally**: You can test reviewer assignment with the `--dry-run` flag: + ```bash + GH_TOKEN= PR_NUMBER= PR_AUTHOR= \ + python3 .github/scripts/assign_reviewers.py --dry-run + ``` + +**Manual assignment**: You can also manually trigger reviewer assignment via GitHub's workflow dispatch with options for dry-run mode and force-assignment. * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. From dd95efe68b7b4d74e043784952cbbd995ce57140 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 3/7] add comprehensive testing, change to pull_request_target Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .../scripts/tests/test_assign_reviewers.py | 466 ++++++++++++++++++ .github/workflows/auto-assign-reviewers.yml | 2 +- 2 files changed, 467 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/tests/test_assign_reviewers.py diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py new file mode 100644 index 000000000000..0038f980201e --- /dev/null +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -0,0 +1,466 @@ +#!/usr/bin/env python3 +""" +End-to-end tests for assign_reviewers.py script. +Tests various scenarios without requiring GitHub API access or tokens. +""" + +import os +import subprocess +import sys +import unittest +from pathlib import Path +from unittest.mock import MagicMock, patch + +# Add parent directory to path to import the script +sys.path.insert(0, str(Path(__file__).parent.parent)) +import assign_reviewers + + +class TestAssignReviewers(unittest.TestCase): + """Test suite for the assign_reviewers.py script""" + + def setUp(self): + """Set up test fixtures""" + # Sample module-paths.json data + self.module_paths = { + "cpp/": "Generic Runtime", + "tensorrt_llm/": "LLM API/Workflow", + "benchmarks/": "Performance", + "docs/": "Documentation", + "tensorrt_llm/_torch/": "Torch Framework" + } + + # Sample module-owners.json data + self.module_owners = { + "Generic Runtime": ["user1", "user2", "user3"], + "LLM API/Workflow": ["user4", "user5"], + "Performance": ["user6", "user7", "user8"], + "Documentation": ["user9"], + "Torch Framework": ["user10", "user11"] + } + + # Set required environment variables + os.environ["PR_NUMBER"] = "123" + os.environ["PR_AUTHOR"] = "test_author" + os.environ["REVIEWER_LIMIT"] = "3" + + def tearDown(self): + """Clean up environment variables""" + for var in ["PR_NUMBER", "PR_AUTHOR", "REVIEWER_LIMIT"]: + if var in os.environ: + del os.environ[var] + + def _mock_subprocess_run(self, *args, **kwargs): + """Mock subprocess.run based on the command being executed""" + cmd = args[0] + cmd_str = ' '.join(cmd) # Join command for easier matching + + # Mock response for getting changed files + if "pr" in cmd and "view" in cmd and "files" in cmd: + return MagicMock(stdout=self.mock_changed_files, + stderr="", + returncode=0) + + # Mock response for getting existing reviewers (users) + elif "pr" in cmd and "view" in cmd and "reviewRequests" in cmd: + # Check if it's asking for login (users) or name (teams) + if "select(.login)" in cmd_str: + return MagicMock(stdout=self.mock_existing_users, + stderr="", + returncode=0) + elif "select(.name)" in cmd_str: + return MagicMock(stdout=self.mock_existing_teams, + stderr="", + returncode=0) + else: + return MagicMock(stdout="", stderr="", returncode=0) + + # Mock response for assigning reviewers + elif "pr" in cmd and "edit" in cmd: + self.assign_reviewers_called = True + self.assigned_reviewers = [ + cmd[i + 1] for i, arg in enumerate(cmd) + if arg == "--add-reviewer" + ] + return MagicMock(stdout="", stderr="", returncode=0) + + return MagicMock(stdout="", stderr="", returncode=0) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_single_module_changed(self, mock_run, mock_load_json): + """Test PR with files from a single module""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\ncpp/file2.h\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # Should respect limit + self.assertTrue( + all(r in ["user1", "user2", "user3"] + for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_multiple_modules_changed(self, mock_run, mock_load_json): + """Test PR with files from multiple modules""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\ndocs/README.md\nbenchmarks/test.py\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned from multiple modules + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # Should respect limit + # Should have mix of reviewers from different modules + all_possible = [ + "user1", "user2", "user3", "user6", "user7", "user8", "user9" + ] + self.assertTrue(all(r in all_possible for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_no_matching_module(self, mock_run, mock_load_json): + """Test PR with files that don't match any module""" + # Setup mocks + self.mock_changed_files = "unknown/file.txt\nrandom/path.py\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_existing_reviewers_skip(self, mock_run, mock_load_json): + """Test that assignment is skipped when reviewers already exist""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "existing_user1\nexisting_user2\n" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function (without force-assign) + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no new reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_force_assign_with_existing(self, mock_run, mock_load_json): + """Test force-assign flag with existing reviewers""" + # Setup mocks + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "user1\n" # user1 is already assigned + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run with force-assign flag + with patch('sys.argv', ['assign_reviewers.py', '--force-assign']): + assign_reviewers.main() + + # Verify reviewers were assigned, excluding already assigned ones + self.assertTrue(self.assign_reviewers_called) + self.assertNotIn("user1", + self.assigned_reviewers) # Should not re-assign + self.assertTrue( + all(r in ["user2", "user3"] for r in self.assigned_reviewers)) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_pr_author_excluded(self, mock_run, mock_load_json): + """Test that PR author is excluded from reviewers""" + # Setup with PR author as a potential reviewer + os.environ["PR_AUTHOR"] = "user2" + + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify author is not in assigned reviewers + self.assertTrue(self.assign_reviewers_called) + self.assertNotIn("user2", self.assigned_reviewers) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_reviewer_limit_zero(self, mock_run, mock_load_json): + """Test with reviewer limit set to 0 (no limit)""" + os.environ["REVIEWER_LIMIT"] = "0" + + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify all reviewers were assigned (no limit) + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), + 3) # All from Generic Runtime + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_dry_run_mode(self, mock_run, mock_load_json): + """Test dry-run mode doesn't execute commands""" + self.mock_changed_files = "cpp/file1.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Capture printed output + import io + from contextlib import redirect_stdout + + f = io.StringIO() + with redirect_stdout(f): + with patch('sys.argv', ['assign_reviewers.py', '--dry-run']): + assign_reviewers.main() + + output = f.getvalue() + + # Verify dry run message was printed and no actual assignment + self.assertIn("DRY RUN:", output) + self.assertIn("gh pr edit", output) + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_empty_pr_no_files(self, mock_run, mock_load_json): + """Test PR with no changed files""" + self.mock_changed_files = "" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_subprocess_error_handling(self, mock_run, mock_load_json): + """Test error handling when subprocess commands fail""" + # Mock a subprocess error + mock_run.side_effect = subprocess.CalledProcessError( + 1, ["gh", "pr", "view"]) + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run should exit with error code + with self.assertRaises(SystemExit) as cm: + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + self.assertEqual(cm.exception.code, 1) + + def test_map_modules_function(self): + """Test the pure map_modules function""" + changed_files = [ + "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" + ] + + modules = assign_reviewers.map_modules(changed_files, self.module_paths) + + self.assertEqual(modules, {"Generic Runtime", "Documentation"}) + + def test_gather_reviewers_function(self): + """Test the pure gather_reviewers function""" + modules = {"Generic Runtime", "Documentation"} + + # Test without exclusions + reviewers = assign_reviewers.gather_reviewers(modules, + self.module_owners) + self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) + + # Test with author exclusion + reviewers = assign_reviewers.gather_reviewers(modules, + self.module_owners, + pr_author="user1") + self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) + + # Test with existing reviewers exclusion + reviewers = assign_reviewers.gather_reviewers( + modules, self.module_owners, existing_reviewers={"user2", "user9"}) + self.assertEqual(set(reviewers), {"user1", "user3"}) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_module_with_no_owners(self, mock_run, mock_load_json): + """Test module that has no owners defined""" + # Add a module with no owners + module_owners_with_empty = self.module_owners.copy() + module_owners_with_empty["Empty Module"] = [] + + self.mock_changed_files = "empty/file.txt\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ({ + "empty/": "Empty Module" + } if "module-paths" in str(path) else module_owners_with_empty) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify no reviewers were assigned + self.assertFalse(self.assign_reviewers_called) + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_files_with_special_characters(self, mock_run, mock_load_json): + """Test files with special characters in names""" + self.mock_changed_files = "cpp/file with spaces.cpp\ncpp/file[brackets].h\ncpp/file@special.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) + + # Run the main function + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify reviewers were assigned correctly despite special characters + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), 3) + + @patch('assign_reviewers.load_json') + def test_json_file_not_found(self, mock_load_json): + """Test handling of missing JSON configuration files""" + mock_load_json.side_effect = FileNotFoundError( + "module-paths.json not found") + + # Run should exit with error + with self.assertRaises(FileNotFoundError): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + @patch('assign_reviewers.load_json') + @patch('subprocess.run') + def test_large_reviewer_pool(self, mock_run, mock_load_json): + """Test with a large number of potential reviewers""" + # Create a module with many owners + large_module_owners = self.module_owners.copy() + large_module_owners["Large Module"] = [f"user{i}" for i in range(20)] + + self.mock_changed_files = "large/file.cpp\n" + self.mock_existing_users = "" + self.mock_existing_teams = "" + self.assign_reviewers_called = False + + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ({ + "large/": "Large Module" + } if "module-paths" in str(path) else large_module_owners) + + # Run the main function with limit + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + # Verify only 3 reviewers were selected (respecting REVIEWER_LIMIT) + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), 3) + self.assertTrue( + all(r in [f"user{i}" for i in range(20)] + for r in self.assigned_reviewers)) + + @patch('subprocess.run') + def test_missing_environment_variables(self, mock_run): + """Test behavior when required environment variables are missing""" + # Remove PR_NUMBER + if "PR_NUMBER" in os.environ: + del os.environ["PR_NUMBER"] + + # Should raise KeyError + with self.assertRaises(KeyError): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + + +if __name__ == "__main__": + # Run tests with verbose output + unittest.main(verbosity=2) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index efb99765e3ab..d7bf4b7d7428 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -1,6 +1,6 @@ name: Auto assign reviewers on: - pull_request: + pull_request_target: types: [opened, synchronize, reopened] workflow_dispatch: inputs: From e435dc2379d6e76d8255c66b7cb6036efb8d10ca 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 4/7] fix: Major module mapping overhaul for auto-assign reviewers - Replace broad 'CI/CD' with 5 granular modules - Separate test concerns from pipeline logic - Move module-owners.json to .github/ for consistency Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/module-owners.json | 39 +++++++++++++++++++++++++++++ .github/module-paths.json | 7 ++++-- .github/scripts/assign_reviewers.py | 3 +-- CONTRIBUTING.md | 8 +++--- 4 files changed, 49 insertions(+), 8 deletions(-) create mode 100644 .github/module-owners.json diff --git a/.github/module-owners.json b/.github/module-owners.json new file mode 100644 index 000000000000..5295e3547ec0 --- /dev/null +++ b/.github/module-owners.json @@ -0,0 +1,39 @@ +{ + "Generic Runtime": ["funatiq", "pcastonguay", "Shixiaowei02", "MartinMarciniszyn", "schetlur-nv", "dcampora"], + "Triton Backend": ["Tabrizian", "pcastonguay", "schetlur-nv"], + "LLM API/Workflow": ["Superjomn", "syuoni", "nv-guomingz", "litaotju", "QiJune"], + "KV-Cache Management":["thorjohnsen", "schetlur-nv"], + "Low Precision":["Tracin", "nv-guomingz", "Naveassaf"], + "Speculative Decoding":["yweng0828", "nekorobov", "lfr-0531"], + "Customized Kernels":["lowsfer", "PerkzZheng", "jdemouth-nvidia"], + "Performance": ["kaiyux", "jiahanc", "hypdeb"], + "Lora/P-tuning":["byshiue", "shaharmor98"], + "Disaggregated Serving":["Shixiaowei02", "joyang-nv", "chuangz0", "schetlur-nv"], + "Documentation":["nv-guomingz"], + "Sampling": ["dcampora", "lfr-0531", "Naveassaf", "syuoni", "yweng0828"], + "Memory": ["litaotju", "peaceh-nv"], + "Installation": ["hchings", "Superjomn", "nv-guomingz", "QiJune"], + "GitHub Configuration": ["tburt-nv", "niukuo"], + "Jenkins Pipelines": ["chzblych", "niukuo"], + "Test Configuration": ["niukuo", "syuoni", "LarryXFly"], + "Test Waive List": ["chzblych", "niukuo"], + "Integration Tests": ["LarryXFly", "niukuo"], + "Torch Framework": ["QiJune", "hlu1"], + "Torch Attention Backend": ["yuxianq", "hlu1"], + "Torch AutoDeploy": ["lucaslie", "suyoggupta"], + "Torch Compilation": ["litaotju", "yizhang-nv", "liji-nv"], + "Torch Custom Ops": ["yizhang-nv"], + "Torch Distributed": ["yilin-void", "yuxianq", "hyukn", "yizhang-nv", "hlu1"], + "Torch PyExecutor": ["dongxuy04", "funatiq", "dcampora", "HuiGao-NV"], + "Torch Speculative": ["lfr-0531", "mikeiovine"], + "Autotuner": ["hyukn", "litaotju"], + "Pipeline Interface": ["amukkara", "chang-l"], + "Torch Models": ["QiJune", "hlu1"], + "Torch Models DeepSeekV3": ["hlu1", "zongfeijing"], + "Torch Models Llama": ["chang-l", "mikeiovine"], + "Torch Modules": ["QiJune", "hlu1"], + "Torch Modules Attention": ["yuxianq", "hlu1"], + "Torch Modules Fused MOE": ["hlu1", "dongxuy04", "zongfeijing", "HuiGao-NV"], + "Torch Tests": ["QiJune", "hlu1"], + "PyTorch Examples": ["QiJune", "hlu1"] +} diff --git a/.github/module-paths.json b/.github/module-paths.json index 0c97a24db17a..45699eb7afa0 100644 --- a/.github/module-paths.json +++ b/.github/module-paths.json @@ -7,8 +7,11 @@ "examples/disaggregated/": "Disaggregated Serving", "docs/": "Documentation", "docker/": "Installation", - ".github/": "CI/CD", - "jenkins/": "CI/CD", + ".github/": "GitHub Configuration", + "jenkins/": "Jenkins Pipelines", + "tests/integration/test_lists/": "Test Configuration", + "tests/integration/test_lists/waives.txt": "Test Waive List", + "tests/integration/defs/": "Integration Tests", "tensorrt_llm/_torch/": "Torch Framework", "tensorrt_llm/_torch/attention_backend/": "Torch Attention Backend", "tensorrt_llm/_torch/auto_deploy/": "Torch AutoDeploy", diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index a5e8104a6393..690e70fc7e20 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -137,8 +137,7 @@ def main() -> None: print(f"Changed files: {changed_files}") module_paths = load_json(Path(".github") / "module-paths.json") - module_owners = load_json( - Path(".github/workflows") / "module-owners.json") + module_owners = load_json(Path(".github") / "module-owners.json") modules = map_modules(changed_files, module_paths) reviewers = gather_reviewers( diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 09d6ba1300d0..f195156a269a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,13 +93,16 @@ Developer workflow for code contributions is as follows: 3. Once the code changes are staged on the fork and ready for review, a [Pull Request](https://help.github.com/en/articles/about-pull-requests) (PR) can be [requested](https://help.github.com/en/articles/creating-a-pull-request) to merge the changes from a branch of the fork into a selected branch of upstream. PRs should typically target the `main` branch. * Creation of a PR creation kicks off the code review process. * At least one TensorRT-LLM engineer will be assigned for the review. When the PR is under review, the label `Pending Review` will be added to the PR. + * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. + * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. + * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. ### Automatic Reviewer Assignment Reviewers are automatically assigned to PRs through a GitHub Action that: * **Triggers**: Runs automatically when PRs are opened, synchronized, or reopened -* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/workflows/module-owners.json` +* **Module-based assignment**: Maps changed files to modules using `.github/module-paths.json` and assigns reviewers based on module ownership defined in `.github/module-owners.json` * **Respects existing assignments**: Won't assign additional reviewers if any reviewers are already assigned (unless forced) * **Excludes PR author**: Automatically excludes the PR author from reviewer assignments * **Limits reviewer count**: Randomly samples up to 3 reviewers if more are eligible (configurable via `REVIEWER_LIMIT`) @@ -114,9 +117,6 @@ The auto-assignment system analyzes all files changed in your PR, maps them to t ``` **Manual assignment**: You can also manually trigger reviewer assignment via GitHub's workflow dispatch with options for dry-run mode and force-assignment. - * If changes are requested, then the reviewer will add the label `Changes Requested` to the PR. - * Once changes are approved, CI will be launched to validate the change. When CI passes, the reviewer will merge the PR. - * If CI reports any failures, it's up to the requester to fix any CI failures before requesting another review. ### PR Submission Policies From c8410c644893e8875feeae987ef0bfa5959326ee 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 5/7] Enhanced assign_reviewers.py with detailed feedback for unmapped files and modules - Modified map_modules() to return both modules and unmapped files - Enhanced gather_reviewers() to track modules without owners - Added comprehensive feedback when no reviewers are assigned: - Warns about files with no module mapping - Warns about modules with no owners - Explains specific reasons for no assignment - Provides actionable guidance for fixing coverage gaps - Updated test cases to cover new functionality - Added test cases for unmapped files and modules without owners Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 84 ++++++++++++++++--- .../scripts/tests/test_assign_reviewers.py | 39 +++++++-- 2 files changed, 104 insertions(+), 19 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 690e70fc7e20..5272d52e8d3c 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -66,25 +66,43 @@ def load_json(path: str): return json.load(f) -def map_modules(changed_files: list[str], module_paths: dict[str, - str]) -> set[str]: +def map_modules(changed_files: list[str], + module_paths: dict[str, str]) -> tuple[set[str], list[str]]: + """Map changed files to modules and return both modules and unmapped files""" modules: set[str] = set() + unmapped_files: list[str] = [] + for file in changed_files: + mapped = False for prefix, module in module_paths.items(): if file.startswith(prefix): modules.add(module) + mapped = True break - return modules + + if not mapped: + unmapped_files.append(file) + + return modules, unmapped_files -def gather_reviewers(modules: set[str], - module_owners: dict[str, list[str]], - *, - pr_author: str | None = None, - existing_reviewers: set[str] | None = None) -> list[str]: +def gather_reviewers( + modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None, + existing_reviewers: set[str] | None = None +) -> tuple[list[str], set[str]]: + """Gather reviewers and return both reviewers and modules without owners""" reviewers: set[str] = set() + modules_without_owners: set[str] = set() + for module in modules: - reviewers.update(module_owners.get(module, [])) + owners = module_owners.get(module, []) + if owners: + reviewers.update(owners) + else: + modules_without_owners.add(module) if pr_author: reviewers.discard(pr_author) @@ -93,7 +111,7 @@ def gather_reviewers(modules: set[str], if existing_reviewers: reviewers -= existing_reviewers - return sorted(reviewers) + return sorted(reviewers), modules_without_owners def main() -> None: @@ -139,8 +157,8 @@ def main() -> None: module_paths = load_json(Path(".github") / "module-paths.json") module_owners = load_json(Path(".github") / "module-owners.json") - modules = map_modules(changed_files, module_paths) - reviewers = gather_reviewers( + modules, unmapped_files = map_modules(changed_files, module_paths) + reviewers, modules_without_owners = gather_reviewers( modules, module_owners, pr_author=pr_author, @@ -154,6 +172,23 @@ def main() -> None: print(f"Changed modules: {sorted(modules)}") print(f"Potential reviewers: {reviewers}") + # Provide detailed feedback about coverage gaps + if unmapped_files: + print(f"⚠️ Files with no module mapping: {unmapped_files}") + print( + f" These files are not covered in .github/module-paths.json") + print( + f" Consider adding appropriate module mappings for these paths." + ) + + if modules_without_owners: + print( + f"⚠️ Modules with no owners: {sorted(modules_without_owners)}") + print( + f" These modules exist in module-paths.json but have no owners in module-owners.json" + ) + print(f" Consider adding owner assignments for these modules.") + if reviewers: cmd = ["gh", "pr", "edit", pr_number] for reviewer in reviewers: @@ -176,6 +211,31 @@ def main() -> None: else: print("✅ No new reviewers to assign") + # Explain why no reviewers were assigned + if not modules and not unmapped_files: + print(" Reason: No files were changed in this PR") + elif not modules and unmapped_files: + print( + " Reason: All changed files are unmapped (no module coverage)" + ) + print( + " ➜ Action needed: Add module mappings to .github/module-paths.json" + ) + elif modules and not reviewers: + if modules_without_owners: + print(" Reason: Matched modules have no assigned owners") + print( + " ➜ Action needed: Add owner assignments to .github/module-owners.json" + ) + else: + print( + " Reason: All potential reviewers are already assigned or excluded" + ) + else: + print( + " Reason: Complex combination of mapping/ownership issues (see warnings above)" + ) + except subprocess.CalledProcessError as e: print(f"❌ Error processing PR: {e}", file=sys.stderr) sys.exit(1) diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py index 0038f980201e..0bc05e49da33 100644 --- a/.github/scripts/tests/test_assign_reviewers.py +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -337,29 +337,54 @@ def test_map_modules_function(self): "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" ] - modules = assign_reviewers.map_modules(changed_files, self.module_paths) + modules, unmapped_files = assign_reviewers.map_modules( + changed_files, self.module_paths) self.assertEqual(modules, {"Generic Runtime", "Documentation"}) + self.assertEqual(unmapped_files, ["unknown/file.txt"]) def test_gather_reviewers_function(self): """Test the pure gather_reviewers function""" modules = {"Generic Runtime", "Documentation"} # Test without exclusions - reviewers = assign_reviewers.gather_reviewers(modules, - self.module_owners) + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners) self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) + self.assertEqual(modules_without_owners, set()) # Test with author exclusion - reviewers = assign_reviewers.gather_reviewers(modules, - self.module_owners, - pr_author="user1") + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners, pr_author="user1") self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) + self.assertEqual(modules_without_owners, set()) # Test with existing reviewers exclusion - reviewers = assign_reviewers.gather_reviewers( + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( modules, self.module_owners, existing_reviewers={"user2", "user9"}) self.assertEqual(set(reviewers), {"user1", "user3"}) + self.assertEqual(modules_without_owners, set()) + + def test_modules_without_owners(self): + """Test modules that have no owners defined""" + modules = {"Generic Runtime", "NonExistent Module"} + + reviewers, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners) + + self.assertEqual(set(reviewers), {"user1", "user2", "user3"}) + self.assertEqual(modules_without_owners, {"NonExistent Module"}) + + def test_all_files_unmapped(self): + """Test when all files are unmapped""" + changed_files = ["unmapped/file1.txt", "another/file2.py"] + + modules, unmapped_files = assign_reviewers.map_modules( + changed_files, self.module_paths) + + self.assertEqual(modules, set()) + self.assertEqual(set(unmapped_files), + {"unmapped/file1.txt", "another/file2.py"}) @patch('assign_reviewers.load_json') @patch('subprocess.run') From 8c6f689153df5ef2c5cedbe826bf8fa574c7ca0d 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 6/7] fix 'first-match-wins' bug and test it Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 26 +++++++++++---- .../scripts/tests/test_assign_reviewers.py | 32 +++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index 5272d52e8d3c..ed7e0984f99f 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -68,19 +68,31 @@ def load_json(path: str): def map_modules(changed_files: list[str], module_paths: dict[str, str]) -> tuple[set[str], list[str]]: - """Map changed files to modules and return both modules and unmapped files""" + """Map changed files to modules using MOST SPECIFIC (longest) prefix match""" modules: set[str] = set() unmapped_files: list[str] = [] for file in changed_files: - mapped = False + # Find ALL matching prefixes + matches = [] for prefix, module in module_paths.items(): if file.startswith(prefix): - modules.add(module) - mapped = True - break - - if not mapped: + matches.append((len(prefix), prefix, module)) + + if matches: + # Sort by prefix length (descending) to get most specific first + matches.sort(reverse=True) + most_specific_module = matches[0][2] + modules.add(most_specific_module) + + # Log if there were multiple matches (for debugging) + if len(matches) > 1: + matches[0][1] + print(f" File '{file}' has overlapping mappings:") + for _, prefix, module in matches: + marker = "→" if module == most_specific_module else " " + print(f" {marker} {prefix} -> {module}") + else: unmapped_files.append(file) return modules, unmapped_files diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py index 0bc05e49da33..6b815800772b 100644 --- a/.github/scripts/tests/test_assign_reviewers.py +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -386,6 +386,38 @@ def test_all_files_unmapped(self): self.assertEqual(set(unmapped_files), {"unmapped/file1.txt", "another/file2.py"}) + def test_most_specific_module_mapping(self): + """Test that files map to the most specific module match""" + # Create overlapping module paths similar to the real config + overlapping_paths = { + "tensorrt_llm/": "LLM API/Workflow", + "tensorrt_llm/_torch/": "Torch Framework", + "tensorrt_llm/_torch/models/": "Torch Models", + "tensorrt_llm/_torch/models/modeling_llama.py": + "Torch Models Llama", + "tests/": "General Tests", + "tests/integration/": "Integration Tests", + "tests/integration/test_lists/": "Test Configuration", + } + + # Test individual files mapping to most specific modules + test_cases = [ + ("tensorrt_llm/api.py", "LLM API/Workflow"), + ("tensorrt_llm/_torch/utils.py", "Torch Framework"), + ("tensorrt_llm/_torch/models/bert.py", "Torch Models"), + ("tensorrt_llm/_torch/models/modeling_llama.py", + "Torch Models Llama"), + ("tests/unit_test.py", "General Tests"), + ("tests/integration/test_x.py", "Integration Tests"), + ("tests/integration/test_lists/config.json", "Test Configuration"), + ] + + for file, expected_module in test_cases: + modules, _ = assign_reviewers.map_modules([file], overlapping_paths) + self.assertEqual( + modules, {expected_module}, + f"File '{file}' should map to '{expected_module}'") + @patch('assign_reviewers.load_json') @patch('subprocess.run') def test_module_with_no_owners(self, mock_run, mock_load_json): From bb53b5b4395ba8dc7b82d7811cfad5631afeeb6d 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 7/7] implement per-module reviewer assignment for better coverage, test it Signed-off-by: Venky Ganesh <23023424+venkywonka@users.noreply.github.com> --- .github/scripts/assign_reviewers.py | 90 ++- .../scripts/tests/test_assign_reviewers.py | 738 +++++++++--------- .github/workflows/auto-assign-reviewers.yml | 2 +- 3 files changed, 413 insertions(+), 417 deletions(-) diff --git a/.github/scripts/assign_reviewers.py b/.github/scripts/assign_reviewers.py index ed7e0984f99f..3481f3d83c1d 100644 --- a/.github/scripts/assign_reviewers.py +++ b/.github/scripts/assign_reviewers.py @@ -99,31 +99,61 @@ def map_modules(changed_files: list[str], def gather_reviewers( - modules: set[str], - module_owners: dict[str, list[str]], - *, - pr_author: str | None = None, - existing_reviewers: set[str] | None = None -) -> tuple[list[str], set[str]]: - """Gather reviewers and return both reviewers and modules without owners""" - reviewers: set[str] = set() + modules: set[str], + module_owners: dict[str, list[str]], + *, + pr_author: str | None = None, + existing_reviewers: set[str] | None = None, + per_module_limit: int = 2 +) -> tuple[list[str], dict[str, list[str]], set[str]]: + """ + Gather reviewers ensuring each module gets representation. + + Args: + modules: Set of module names that were touched + module_owners: Dict mapping module names to lists of owners + pr_author: PR author to exclude from reviewers + existing_reviewers: Set of already assigned reviewers to exclude + per_module_limit: Maximum reviewers to assign per module + + Returns: + - List of all unique reviewers to assign + - Dict mapping modules to their assigned reviewers + - Set of modules without owners + """ + all_reviewers: set[str] = set() + module_assignments: dict[str, list[str]] = {} modules_without_owners: set[str] = set() - for module in modules: + for module in sorted(modules): # Sort for consistent ordering owners = module_owners.get(module, []) - if owners: - reviewers.update(owners) - else: + if not owners: modules_without_owners.add(module) + module_assignments[module] = [] + continue + + # Filter out PR author and existing reviewers + eligible_owners = [ + o for o in owners if o != pr_author and ( + not existing_reviewers or o not in existing_reviewers) + ] + + if not eligible_owners: + # All owners are excluded + print( + f" ⚠️ Module '{module}': All owners excluded (PR author or already assigned)" + ) + module_assignments[module] = [] + continue - if pr_author: - reviewers.discard(pr_author) + # Sample up to per_module_limit reviewers for this module + num_to_select = min(len(eligible_owners), per_module_limit) + selected = random.sample(eligible_owners, num_to_select) - # Remove existing reviewers to avoid duplicate assignments - if existing_reviewers: - reviewers -= existing_reviewers + module_assignments[module] = selected + all_reviewers.update(selected) - return sorted(reviewers), modules_without_owners + return sorted(all_reviewers), module_assignments, modules_without_owners def main() -> None: @@ -141,10 +171,11 @@ def main() -> None: args = parser.parse_args() pr_number = os.environ["PR_NUMBER"] - reviewer_limit = int(os.environ.get("REVIEWER_LIMIT", "0")) + per_module_limit = int(os.environ.get("PER_MODULE_REVIEWER_LIMIT", "2")) pr_author = os.environ.get("PR_AUTHOR") print(f"Testing PR #{pr_number} with author: {pr_author}") + print(f"Per-module reviewer limit: {per_module_limit}") # Check existing reviewers existing_user_reviewers, existing_team_reviewers = get_existing_reviewers( @@ -170,19 +201,26 @@ def main() -> None: module_owners = load_json(Path(".github") / "module-owners.json") modules, unmapped_files = map_modules(changed_files, module_paths) - reviewers, modules_without_owners = gather_reviewers( + reviewers, module_assignments, modules_without_owners = gather_reviewers( modules, module_owners, pr_author=pr_author, existing_reviewers= - existing_user_reviewers # Avoid re-assigning existing users - ) + existing_user_reviewers, # Avoid re-assigning existing users + per_module_limit=per_module_limit) - if reviewer_limit and len(reviewers) > reviewer_limit: - reviewers = random.sample(reviewers, reviewer_limit) + print(f"\nChanged modules: {sorted(modules)}") + + # Show module-specific assignments + if module_assignments: + print("\nModule assignments:") + for module, assigned in sorted(module_assignments.items()): + if assigned: + print(f" {module}: {assigned}") + else: + print(f" {module}: No eligible reviewers") - print(f"Changed modules: {sorted(modules)}") - print(f"Potential reviewers: {reviewers}") + print(f"\nFinal reviewers to assign: {reviewers}") # Provide detailed feedback about coverage gaps if unmapped_files: diff --git a/.github/scripts/tests/test_assign_reviewers.py b/.github/scripts/tests/test_assign_reviewers.py index 6b815800772b..6d8b43c1c326 100644 --- a/.github/scripts/tests/test_assign_reviewers.py +++ b/.github/scripts/tests/test_assign_reviewers.py @@ -7,46 +7,42 @@ import os import subprocess import sys -import unittest from pathlib import Path +from unittest import TestCase from unittest.mock import MagicMock, patch -# Add parent directory to path to import the script +# Add the parent directory to the path so we can import assign_reviewers sys.path.insert(0, str(Path(__file__).parent.parent)) import assign_reviewers -class TestAssignReviewers(unittest.TestCase): +class TestAssignReviewers(TestCase): """Test suite for the assign_reviewers.py script""" def setUp(self): - """Set up test fixtures""" - # Sample module-paths.json data + """Set up test environment""" + # Set required environment variables + os.environ["PR_NUMBER"] = "123" + os.environ["PR_AUTHOR"] = "test-author" + os.environ["PER_MODULE_REVIEWER_LIMIT"] = "2" + + # Set up test data self.module_paths = { "cpp/": "Generic Runtime", - "tensorrt_llm/": "LLM API/Workflow", - "benchmarks/": "Performance", "docs/": "Documentation", - "tensorrt_llm/_torch/": "Torch Framework" } - # Sample module-owners.json data self.module_owners = { "Generic Runtime": ["user1", "user2", "user3"], - "LLM API/Workflow": ["user4", "user5"], - "Performance": ["user6", "user7", "user8"], "Documentation": ["user9"], - "Torch Framework": ["user10", "user11"] + "Module1": ["owner1", "owner2"], + "Module2": ["owner3", "owner4"], + "Module3": [], # No owners } - # Set required environment variables - os.environ["PR_NUMBER"] = "123" - os.environ["PR_AUTHOR"] = "test_author" - os.environ["REVIEWER_LIMIT"] = "3" - def tearDown(self): """Clean up environment variables""" - for var in ["PR_NUMBER", "PR_AUTHOR", "REVIEWER_LIMIT"]: + for var in ["PR_NUMBER", "PR_AUTHOR", "PER_MODULE_REVIEWER_LIMIT"]: if var in os.environ: del os.environ[var] @@ -86,401 +82,377 @@ def _mock_subprocess_run(self, *args, **kwargs): return MagicMock(stdout="", stderr="", returncode=0) - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_single_module_changed(self, mock_run, mock_load_json): - """Test PR with files from a single module""" - # Setup mocks - self.mock_changed_files = "cpp/file1.cpp\ncpp/file2.h\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False - - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + # ========== Unit Tests for Core Functions ========== - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() - - # Verify reviewers were assigned - self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), - 3) # Should respect limit - self.assertTrue( - all(r in ["user1", "user2", "user3"] - for r in self.assigned_reviewers)) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_multiple_modules_changed(self, mock_run, mock_load_json): - """Test PR with files from multiple modules""" - # Setup mocks - self.mock_changed_files = "cpp/file1.cpp\ndocs/README.md\nbenchmarks/test.py\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False - - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + def test_module_mapping_scenarios(self): + """Test various module mapping scenarios with parametrized data""" + test_cases = [ + # Basic mapping with unmapped files + { + "name": + "basic_mapping", + "files": [ + "cpp/main.cpp", "cpp/utils.h", "docs/README.md", + "unknown/file.txt" + ], + "paths": { + "cpp/": "Generic Runtime", + "docs/": "Documentation" + }, + "expected_modules": {"Generic Runtime", "Documentation"}, + "expected_unmapped": ["unknown/file.txt"] + }, + # Most specific module matching + { + "name": "most_specific_single", + "files": ["tensorrt_llm/_torch/models/bert.py"], + "paths": { + "tensorrt_llm/": "LLM API/Workflow", + "tensorrt_llm/_torch/": "Torch Framework", + "tensorrt_llm/_torch/models/": "Torch Models" + }, + "expected_modules": {"Torch Models"}, + "expected_unmapped": [] + }, + # Multiple files with overlapping paths + { + "name": + "multiple_overlapping", + "files": [ + "tensorrt_llm/config.py", "tensorrt_llm/_torch/base.py", + "tensorrt_llm/_torch/models/gpt.py" + ], + "paths": { + "tensorrt_llm/": "LLM API/Workflow", + "tensorrt_llm/_torch/": "Torch Framework", + "tensorrt_llm/_torch/models/": "Torch Models" + }, + "expected_modules": + {"LLM API/Workflow", "Torch Framework", "Torch Models"}, + "expected_unmapped": [] + }, + # All files unmapped + { + "name": "all_unmapped", + "files": ["unmapped/file1.txt", "another/file2.py"], + "paths": { + "cpp/": "Generic Runtime" + }, + "expected_modules": set(), + "expected_unmapped": ["unmapped/file1.txt", "another/file2.py"] + }, + # Exact file match priority + { + "name": "exact_file_match", + "files": ["tests/integration/test_lists/waives.txt"], + "paths": { + "tests/": "General Tests", + "tests/integration/": "Integration Tests", + "tests/integration/test_lists/": "Test Configuration", + "tests/integration/test_lists/waives.txt": "Test Waive List" + }, + "expected_modules": {"Test Waive List"}, + "expected_unmapped": [] + } + ] - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + for case in test_cases: + with self.subTest(case=case["name"]): + modules, unmapped = assign_reviewers.map_modules( + case["files"], case["paths"]) + self.assertEqual(modules, case["expected_modules"], + f"Failed for case: {case['name']}") + self.assertEqual(set(unmapped), set(case["expected_unmapped"]), + f"Failed for case: {case['name']}") + + def test_gather_reviewers_basic(self): + """Test basic gather_reviewers functionality""" + modules = {"Module1", "Module2", "Module3"} + + reviewers, module_assignments, modules_without_owners = assign_reviewers.gather_reviewers( + modules, self.module_owners, per_module_limit=10) + + # Should get all unique reviewers from modules with owners + expected = ["owner1", "owner2", "owner3", "owner4"] + self.assertEqual(set(reviewers), set(expected)) + + # Check module assignments + self.assertEqual(set(module_assignments["Module1"]), + {"owner1", "owner2"}) + self.assertEqual(set(module_assignments["Module2"]), + {"owner3", "owner4"}) + self.assertEqual(module_assignments["Module3"], []) + self.assertEqual(modules_without_owners, {"Module3"}) + + def test_gather_reviewers_exclusions(self): + """Test reviewer exclusion functionality""" + modules = {"Module1", "Module2"} + + # Test PR author exclusion + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + modules, + self.module_owners, + pr_author="owner1", + per_module_limit=10) + + self.assertNotIn("owner1", reviewers) + self.assertNotIn("owner1", module_assignments["Module1"]) + + # Test existing reviewers exclusion + existing = {"owner2", "owner3"} + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + modules, + self.module_owners, + existing_reviewers=existing, + per_module_limit=10) + + self.assertFalse(any(r in existing for r in reviewers)) + + def test_per_module_reviewer_limit(self): + """Test per-module reviewer limit functionality""" + modules = {"Module1", "Module2"} + module_owners = { + "Module1": ["a", "b", "c", "d", "e"], # 5 owners + "Module2": ["f", "g", "h"], # 3 owners + } - # Verify reviewers were assigned from multiple modules - self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), - 3) # Should respect limit - # Should have mix of reviewers from different modules - all_possible = [ - "user1", "user2", "user3", "user6", "user7", "user8", "user9" - ] - self.assertTrue(all(r in all_possible for r in self.assigned_reviewers)) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_no_matching_module(self, mock_run, mock_load_json): - """Test PR with files that don't match any module""" - # Setup mocks - self.mock_changed_files = "unknown/file.txt\nrandom/path.py\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + modules, module_owners, per_module_limit=2) - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + # Each module should have at most 2 reviewers + self.assertEqual(len(module_assignments["Module1"]), 2) + self.assertEqual(len(module_assignments["Module2"]), 2) + self.assertEqual(len(reviewers), 4) - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + # Verify reviewers are from correct modules + self.assertTrue( + set(module_assignments["Module1"]).issubset( + {"a", "b", "c", "d", "e"})) + self.assertTrue( + set(module_assignments["Module2"]).issubset({"f", "g", "h"})) + + def test_module_reviewer_overlap(self): + """Test handling when reviewers own multiple modules""" + modules = {"Module1", "Module2", "Module3"} + module_owners = { + "Module1": ["shared", "owner1"], + "Module2": ["shared", "owner2"], + "Module3": ["owner3"], + } - # Verify no reviewers were assigned - self.assertFalse(self.assign_reviewers_called) + # Run multiple times to test randomness + total_reviewers_counts = [] + for _ in range(10): + reviewers, _, _ = assign_reviewers.gather_reviewers( + modules, module_owners, per_module_limit=1) + total_reviewers_counts.append(len(reviewers)) + + # Should see both 2 and 3 reviewers due to random selection of 'shared' + self.assertTrue(any(count == 2 for count in total_reviewers_counts)) + self.assertTrue(any(count == 3 for count in total_reviewers_counts)) + + def test_module_coverage_edge_cases(self): + """Test edge cases in module coverage""" + module_owners = { + "Module1": ["alice", "bob"], + "Module2": ["bob"], # Only bob owns this + "Module3": ["charlie"], + } - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_existing_reviewers_skip(self, mock_run, mock_load_json): - """Test that assignment is skipped when reviewers already exist""" - # Setup mocks - self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "existing_user1\nexisting_user2\n" + # Case 1: PR author owns a module entirely + modules = {"Module1", "Module2", "Module3"} + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + modules, module_owners, pr_author="bob", per_module_limit=2) + + self.assertEqual(module_assignments["Module2"], + []) # No eligible reviewers + self.assertEqual(module_assignments["Module1"], ["alice"]) + self.assertEqual(module_assignments["Module3"], ["charlie"]) + + # Case 2: All owners already assigned + existing = {"alice", "charlie"} + reviewers, module_assignments, _ = assign_reviewers.gather_reviewers( + {"Module1", "Module3"}, + module_owners, + pr_author="bob", + existing_reviewers=existing, + per_module_limit=2) + + self.assertEqual(len(reviewers), 0) + self.assertEqual(module_assignments["Module1"], []) + self.assertEqual(module_assignments["Module3"], []) + + # ========== Integration Tests ========== + + def _run_integration_test(self, + changed_files, + expected_reviewer_count=None, + expected_assigned=True, + pr_author=None, + existing_users="", + extra_assertions=None): + """Helper method to run integration tests with common setup""" + self.mock_changed_files = changed_files + self.mock_existing_users = existing_users self.mock_existing_teams = "" self.assign_reviewers_called = False - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + if pr_author: + os.environ["PR_AUTHOR"] = pr_author - # Run the main function (without force-assign) - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - # Verify no new reviewers were assigned - self.assertFalse(self.assign_reviewers_called) + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_force_assign_with_existing(self, mock_run, mock_load_json): - """Test force-assign flag with existing reviewers""" - # Setup mocks - self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "user1\n" # user1 is already assigned - self.mock_existing_teams = "" - self.assign_reviewers_called = False + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + self.assertEqual(self.assign_reviewers_called, expected_assigned) - # Run with force-assign flag - with patch('sys.argv', ['assign_reviewers.py', '--force-assign']): - assign_reviewers.main() + if expected_reviewer_count is not None and expected_assigned: + self.assertEqual(len(self.assigned_reviewers), + expected_reviewer_count) - # Verify reviewers were assigned, excluding already assigned ones - self.assertTrue(self.assign_reviewers_called) - self.assertNotIn("user1", - self.assigned_reviewers) # Should not re-assign - self.assertTrue( - all(r in ["user2", "user3"] for r in self.assigned_reviewers)) + if extra_assertions and expected_assigned: + extra_assertions(self) - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_pr_author_excluded(self, mock_run, mock_load_json): + def test_single_module_changed(self): + """Test PR with files from a single module""" + self._run_integration_test( + changed_files="cpp/file1.cpp\ncpp/file2.h\n", + expected_reviewer_count=2, + extra_assertions=lambda self: self.assertTrue( + all(r in ["user1", "user2", "user3"] + for r in self.assigned_reviewers))) + + def test_multiple_modules_changed(self): + """Test PR with files from multiple modules""" + self._run_integration_test( + changed_files="cpp/file1.cpp\ndocs/README.md\n", + expected_reviewer_count= + 3, # 2 from Generic Runtime, 1 from Documentation + extra_assertions=lambda self: self.assertTrue( + all(r in ["user1", "user2", "user3", "user9"] + for r in self.assigned_reviewers))) + + def test_no_files_or_unmapped(self): + """Test PR with no files or unmapped files""" + # No files + self._run_integration_test(changed_files="", expected_assigned=False) + + # Unmapped files + self._run_integration_test( + changed_files="unknown/file.txt\nrandom/path.py\n", + expected_assigned=False) + + def test_pr_author_excluded(self): """Test that PR author is excluded from reviewers""" - # Setup with PR author as a potential reviewer - os.environ["PR_AUTHOR"] = "user2" - + self._run_integration_test( + changed_files="cpp/file1.cpp\n", + pr_author="user2", + expected_reviewer_count=2, + extra_assertions=lambda self: self.assertNotIn( + "user2", self.assigned_reviewers)) + + def test_existing_reviewers_behavior(self): + """Test behavior with existing reviewers""" + # Should skip assignment when reviewers exist + self._run_integration_test( + changed_files="cpp/file1.cpp\n", + existing_users="existing_user1\nexisting_user2\n", + expected_assigned=False) + + # Force assign with existing reviewers self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "" + self.mock_existing_users = "user1\n" self.mock_existing_teams = "" self.assign_reviewers_called = False - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - # Verify author is not in assigned reviewers - self.assertTrue(self.assign_reviewers_called) - self.assertNotIn("user2", self.assigned_reviewers) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_reviewer_limit_zero(self, mock_run, mock_load_json): - """Test with reviewer limit set to 0 (no limit)""" - os.environ["REVIEWER_LIMIT"] = "0" - - self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with patch('sys.argv', ['assign_reviewers.py', '--force-assign']): + assign_reviewers.main() - # Verify all reviewers were assigned (no limit) self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), - 3) # All from Generic Runtime - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_dry_run_mode(self, mock_run, mock_load_json): - """Test dry-run mode doesn't execute commands""" - self.mock_changed_files = "cpp/file1.cpp\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + self.assertNotIn("user1", self.assigned_reviewers) - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Capture printed output + def test_special_modes(self): + """Test dry-run and error modes""" + # Dry run mode import io from contextlib import redirect_stdout - f = io.StringIO() - with redirect_stdout(f): - with patch('sys.argv', ['assign_reviewers.py', '--dry-run']): - assign_reviewers.main() - - output = f.getvalue() - - # Verify dry run message was printed and no actual assignment - self.assertIn("DRY RUN:", output) - self.assertIn("gh pr edit", output) - self.assertFalse(self.assign_reviewers_called) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_empty_pr_no_files(self, mock_run, mock_load_json): - """Test PR with no changed files""" - self.mock_changed_files = "" + self.mock_changed_files = "cpp/file1.cpp\n" self.mock_existing_users = "" self.mock_existing_teams = "" self.assign_reviewers_called = False - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() - - # Verify no reviewers were assigned - self.assertFalse(self.assign_reviewers_called) - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_subprocess_error_handling(self, mock_run, mock_load_json): - """Test error handling when subprocess commands fail""" - # Mock a subprocess error - mock_run.side_effect = subprocess.CalledProcessError( - 1, ["gh", "pr", "view"]) - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) - - # Run should exit with error code - with self.assertRaises(SystemExit) as cm: - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() - - self.assertEqual(cm.exception.code, 1) - - def test_map_modules_function(self): - """Test the pure map_modules function""" - changed_files = [ - "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt" - ] - - modules, unmapped_files = assign_reviewers.map_modules( - changed_files, self.module_paths) - - self.assertEqual(modules, {"Generic Runtime", "Documentation"}) - self.assertEqual(unmapped_files, ["unknown/file.txt"]) - - def test_gather_reviewers_function(self): - """Test the pure gather_reviewers function""" - modules = {"Generic Runtime", "Documentation"} - - # Test without exclusions - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners) - self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"}) - self.assertEqual(modules_without_owners, set()) - - # Test with author exclusion - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners, pr_author="user1") - self.assertEqual(set(reviewers), {"user2", "user3", "user9"}) - self.assertEqual(modules_without_owners, set()) - - # Test with existing reviewers exclusion - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners, existing_reviewers={"user2", "user9"}) - self.assertEqual(set(reviewers), {"user1", "user3"}) - self.assertEqual(modules_without_owners, set()) - - def test_modules_without_owners(self): - """Test modules that have no owners defined""" - modules = {"Generic Runtime", "NonExistent Module"} - - reviewers, modules_without_owners = assign_reviewers.gather_reviewers( - modules, self.module_owners) - - self.assertEqual(set(reviewers), {"user1", "user2", "user3"}) - self.assertEqual(modules_without_owners, {"NonExistent Module"}) - - def test_all_files_unmapped(self): - """Test when all files are unmapped""" - changed_files = ["unmapped/file1.txt", "another/file2.py"] - - modules, unmapped_files = assign_reviewers.map_modules( - changed_files, self.module_paths) - - self.assertEqual(modules, set()) - self.assertEqual(set(unmapped_files), - {"unmapped/file1.txt", "another/file2.py"}) - - def test_most_specific_module_mapping(self): - """Test that files map to the most specific module match""" - # Create overlapping module paths similar to the real config - overlapping_paths = { - "tensorrt_llm/": "LLM API/Workflow", - "tensorrt_llm/_torch/": "Torch Framework", - "tensorrt_llm/_torch/models/": "Torch Models", - "tensorrt_llm/_torch/models/modeling_llama.py": - "Torch Models Llama", - "tests/": "General Tests", - "tests/integration/": "Integration Tests", - "tests/integration/test_lists/": "Test Configuration", - } - - # Test individual files mapping to most specific modules - test_cases = [ - ("tensorrt_llm/api.py", "LLM API/Workflow"), - ("tensorrt_llm/_torch/utils.py", "Torch Framework"), - ("tensorrt_llm/_torch/models/bert.py", "Torch Models"), - ("tensorrt_llm/_torch/models/modeling_llama.py", - "Torch Models Llama"), - ("tests/unit_test.py", "General Tests"), - ("tests/integration/test_x.py", "Integration Tests"), - ("tests/integration/test_lists/config.json", "Test Configuration"), - ] - - for file, expected_module in test_cases: - modules, _ = assign_reviewers.map_modules([file], overlapping_paths) - self.assertEqual( - modules, {expected_module}, - f"File '{file}' should map to '{expected_module}'") - - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_module_with_no_owners(self, mock_run, mock_load_json): - """Test module that has no owners defined""" - # Add a module with no owners - module_owners_with_empty = self.module_owners.copy() - module_owners_with_empty["Empty Module"] = [] - - self.mock_changed_files = "empty/file.txt\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + f = io.StringIO() + with redirect_stdout(f): + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ({ - "empty/": "Empty Module" - } if "module-paths" in str(path) else module_owners_with_empty) + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ( + self.module_paths + if "module-paths" in str(path) else self.module_owners) - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with patch('sys.argv', ['assign_reviewers.py', '--dry-run']): + assign_reviewers.main() - # Verify no reviewers were assigned + output = f.getvalue() + self.assertIn("DRY RUN:", output) self.assertFalse(self.assign_reviewers_called) - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_files_with_special_characters(self, mock_run, mock_load_json): - """Test files with special characters in names""" - self.mock_changed_files = "cpp/file with spaces.cpp\ncpp/file[brackets].h\ncpp/file@special.cpp\n" - self.mock_existing_users = "" - self.mock_existing_teams = "" - self.assign_reviewers_called = False + def test_error_handling(self): + """Test various error handling scenarios""" + # Subprocess error + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ( - self.module_paths - if "module-paths" in str(path) else self.module_owners) + mock_run.side_effect = subprocess.CalledProcessError( + 1, ["gh", "pr", "view"]) + mock_load_json.side_effect = lambda path: self.module_paths - # Run the main function - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + with self.assertRaises(SystemExit) as cm: + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() + self.assertEqual(cm.exception.code, 1) - # Verify reviewers were assigned correctly despite special characters - self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), 3) + # Missing JSON file + with patch('assign_reviewers.load_json') as mock_load_json: + mock_load_json.side_effect = FileNotFoundError( + "module-paths.json not found") - @patch('assign_reviewers.load_json') - def test_json_file_not_found(self, mock_load_json): - """Test handling of missing JSON configuration files""" - mock_load_json.side_effect = FileNotFoundError( - "module-paths.json not found") + with self.assertRaises(FileNotFoundError): + with patch('sys.argv', ['assign_reviewers.py']): + assign_reviewers.main() - # Run should exit with error - with self.assertRaises(FileNotFoundError): + # Missing environment variable + del os.environ["PR_NUMBER"] + with self.assertRaises(KeyError): with patch('sys.argv', ['assign_reviewers.py']): assign_reviewers.main() - @patch('assign_reviewers.load_json') - @patch('subprocess.run') - def test_large_reviewer_pool(self, mock_run, mock_load_json): - """Test with a large number of potential reviewers""" - # Create a module with many owners + def test_edge_cases_integration(self): + """Test edge cases in full integration""" + # Files with special characters + self._run_integration_test( + changed_files= + "cpp/file with spaces.cpp\ncpp/file[brackets].h\ncpp/file@special.cpp\n", + expected_reviewer_count=2) + + # Large reviewer pool large_module_owners = self.module_owners.copy() large_module_owners["Large Module"] = [f"user{i}" for i in range(20)] @@ -489,35 +461,21 @@ def test_large_reviewer_pool(self, mock_run, mock_load_json): self.mock_existing_teams = "" self.assign_reviewers_called = False - mock_run.side_effect = self._mock_subprocess_run - mock_load_json.side_effect = lambda path: ({ - "large/": "Large Module" - } if "module-paths" in str(path) else large_module_owners) + with patch('subprocess.run') as mock_run, \ + patch('assign_reviewers.load_json') as mock_load_json: - # Run the main function with limit - with patch('sys.argv', ['assign_reviewers.py']): - assign_reviewers.main() + mock_run.side_effect = self._mock_subprocess_run + mock_load_json.side_effect = lambda path: ({ + "large/": "Large Module" + } if "module-paths" in str(path) else large_module_owners) - # Verify only 3 reviewers were selected (respecting REVIEWER_LIMIT) - self.assertTrue(self.assign_reviewers_called) - self.assertEqual(len(self.assigned_reviewers), 3) - self.assertTrue( - all(r in [f"user{i}" for i in range(20)] - for r in self.assigned_reviewers)) - - @patch('subprocess.run') - def test_missing_environment_variables(self, mock_run): - """Test behavior when required environment variables are missing""" - # Remove PR_NUMBER - if "PR_NUMBER" in os.environ: - del os.environ["PR_NUMBER"] - - # Should raise KeyError - with self.assertRaises(KeyError): with patch('sys.argv', ['assign_reviewers.py']): assign_reviewers.main() + self.assertTrue(self.assign_reviewers_called) + self.assertEqual(len(self.assigned_reviewers), 2) # Per-module limit + if __name__ == "__main__": - # Run tests with verbose output + import unittest unittest.main(verbosity=2) diff --git a/.github/workflows/auto-assign-reviewers.yml b/.github/workflows/auto-assign-reviewers.yml index d7bf4b7d7428..722598499351 100644 --- a/.github/workflows/auto-assign-reviewers.yml +++ b/.github/workflows/auto-assign-reviewers.yml @@ -34,7 +34,7 @@ jobs: PR_NUMBER: ${{ github.event.inputs.pr_number || github.event.pull_request.number }} PR_AUTHOR: ${{ github.event.pull_request.user.login || github.event.inputs.pr_author || '' }} GH_TOKEN: ${{ secrets.REVIEW_ASSIGNING_TOKEN }} - REVIEWER_LIMIT: '3' + PER_MODULE_REVIEWER_LIMIT: '2' run: | python3 .github/scripts/assign_reviewers.py \ ${{ github.event.inputs.dry_run == 'true' && '--dry-run' || '' }} \