diff --git a/.github/workflows/claude_review.yml b/.github/workflows/claude_review.yml index 98fe4eac964..fbc708ce556 100644 --- a/.github/workflows/claude_review.yml +++ b/.github/workflows/claude_review.yml @@ -4,102 +4,70 @@ on: issue_comment: types: [created] +permissions: + contents: read + pull-requests: write + issues: write + id-token: write + jobs: # ────────────────────────────────────────────────────────────────── # Light review: quick pass for obvious bugs, typos, and test gaps # Trigger: /claude review # ────────────────────────────────────────────────────────────────── light-review: - name: Claude Light Review if: | github.event_name == 'issue_comment' && github.event.issue.pull_request && contains(github.event.comment.body, '/claude review') - runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: write - issues: write - id-token: write - env: - GH_TOKEN: ${{ github.token }} - REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.issue.number }} - steps: - - name: Get PR head commit - id: get-pr-head-commit - run: | - echo "sha=$(gh pr view $PR_NUMBER --repo $REPO --json headRefOid -q .headRefOid)" | tee -a $GITHUB_OUTPUT - - - name: Checkout repository - uses: actions/checkout@v6 - with: - fetch-depth: 1 - ref: ${{ steps.get-pr-head-commit.outputs.sha }} - - - name: React to trigger comment - run: | - gh api repos/$REPO/issues/comments/${{ github.event.comment.id }}/reactions \ - --method POST \ - -f content='eyes' - - - name: Run Claude Light Review - uses: anthropics/claude-code-action@v1 - env: - ANTHROPIC_BASE_URL: ${{ secrets.NVIDIA_INFERENCE_URL }} - CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS: "1" - DISABLE_PROMPT_CACHING: "1" - with: - anthropic_api_key: ${{ secrets.NVIDIA_INFERENCE_KEY }} - trigger_phrase: "/claude review" - show_full_output: true - claude_args: | - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Read" - --model "${{ vars.CLAUDE_MODEL }}" - prompt: | - REPO: ${{ env.REPO }} - PR NUMBER: ${{ env.PR_NUMBER }} - - Mandatory workflow — never skip or reorder: - 1. Read the PR diff first (gh pr diff). - 2. Based on the changed files and areas, identify relevant skills from skills//SKILL.md. - Common skill names: build-and-dependency, testing, cicd, linting-and-formatting, run-on-slurm, - nightly-sync, create-issue, respond-to-issue, split-pr, onboard-gb200-1node-tests. - 3. Read the SKILL.md files for all relevant areas using the Read tool. - 4. Only then perform the review using the skill context. - - You are doing a light code review. Keep it concise and actionable. - - Focus ONLY on: - - Critical bugs or logic errors - - Typos in code, comments, or strings - - Missing or insufficient test coverage for changed code - - If the PR adds a new feature or significant functionality without corresponding tests, suggest adding tests - - If the PR fixes a bug that was not caught by an existing unit test, suggest adding a regression test to prevent recurrence - - Outdated or inaccurate documentation affected by the changes - - New direct global process group access in `megatron/core` production code - - Flag added calls to `parallel_state.get_*_group()` or directly imported - `get_*_group()` helpers unless they are in `parallel_state.py`, - `process_groups_config.py`, initialization/bootstrap code that materializes a - `ProcessGroupCollection`, tests, docs, or an explicitly documented migration fallback - - Prefer passing a `ProcessGroupCollection` or explicit - `torch.distributed.ProcessGroup` from the caller - - Do NOT comment on: - - Style preferences or formatting - - Minor naming suggestions - - Architectural opinions or refactoring ideas - - Performance unless there is a clear, measurable issue - - Only use inline ```suggestion blocks for simple, self-contained line replacements (typos, - renames, single-line fixes). For structural changes that add, remove, or reorganize blocks - of code (e.g. adding a new function, inserting a YAML step, reordering logic), use a - top-level PR comment with a code block showing the proposed change instead — inline - suggestions cannot express insertions or multi-block restructuring and will break the code - if applied. - - It's perfectly acceptable to not have anything to comment on. - If you do not have anything to comment on, approve the PR with: gh pr review $PR_NUMBER --repo $REPO --approve --body "LGTM" + uses: NVIDIA-NeMo/FW-CI-templates/.github/workflows/_claude_review.yml@v1.7.1 + with: + trigger_phrase: "/claude review" + model: ${{ vars.CLAUDE_MODEL }} + prompt: | + Mandatory workflow — never skip or reorder: + 1. Read the PR diff first (gh pr diff). + 2. Based on the changed files and areas, identify relevant skills from skills//SKILL.md. + Common skill names: build-and-dependency, testing, cicd, linting-and-formatting, run-on-slurm, + nightly-sync, create-issue, respond-to-issue, split-pr, onboard-gb200-1node-tests. + 3. Read the SKILL.md files for all relevant areas using the Read tool. + 4. Only then perform the review using the skill context. + + You are doing a light code review. Keep it concise and actionable. + + Focus ONLY on: + - Critical bugs or logic errors + - Typos in code, comments, or strings + - Missing or insufficient test coverage for changed code + - If the PR adds a new feature or significant functionality without corresponding tests, suggest adding tests + - If the PR fixes a bug that was not caught by an existing unit test, suggest adding a regression test to prevent recurrence + - Outdated or inaccurate documentation affected by the changes + - New direct global process group access in `megatron/core` production code + - Flag added calls to `parallel_state.get_*_group()` or directly imported + `get_*_group()` helpers unless they are in `parallel_state.py`, + `process_groups_config.py`, initialization/bootstrap code that materializes a + `ProcessGroupCollection`, tests, docs, or an explicitly documented migration fallback + - Prefer passing a `ProcessGroupCollection` or explicit + `torch.distributed.ProcessGroup` from the caller + + Do NOT comment on: + - Style preferences or formatting + - Minor naming suggestions + - Architectural opinions or refactoring ideas + - Performance unless there is a clear, measurable issue + + Only use inline ```suggestion blocks for simple, self-contained line replacements (typos, + renames, single-line fixes). For structural changes that add, remove, or reorganize blocks + of code (e.g. adding a new function, inserting a YAML step, reordering logic), use a + top-level PR comment with a code block showing the proposed change instead — inline + suggestions cannot express insertions or multi-block restructuring and will break the code + if applied. + + It's perfectly acceptable to not have anything to comment on. + If you do not have anything to comment on, approve the PR with: gh pr review $PR_NUMBER --repo $REPO --approve --body "LGTM" + secrets: + NVIDIA_INFERENCE_URL: ${{ secrets.NVIDIA_INFERENCE_URL }} + NVIDIA_INFERENCE_KEY: ${{ secrets.NVIDIA_INFERENCE_KEY }} # ────────────────────────────────────────────────────────────────── # Strict review: comprehensive Megatron-LM focused analysis @@ -108,191 +76,147 @@ jobs: # Trigger: /claude strict-review # ────────────────────────────────────────────────────────────────── strict-review: - name: Claude Strict Review if: | github.event_name == 'issue_comment' && github.event.issue.pull_request && contains(github.event.comment.body, '/claude strict-review') - runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: write - issues: write - id-token: write - env: - GH_TOKEN: ${{ github.token }} - REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.issue.number }} - steps: - - name: Get PR info - id: pr-info - run: | - PR_DATA=$(gh pr view $PR_NUMBER --repo $REPO --json headRefOid,baseRefName) - echo "sha=$(echo $PR_DATA | jq -r .headRefOid)" >> $GITHUB_OUTPUT - echo "base_ref=$(echo $PR_DATA | jq -r .baseRefName)" >> $GITHUB_OUTPUT - - - name: Checkout repository - uses: actions/checkout@v6 - with: - fetch-depth: 1 - ref: ${{ steps.pr-info.outputs.sha }} - - - name: Fetch base branch for diff analysis - run: git fetch origin ${{ steps.pr-info.outputs.base_ref }} - - - name: React to trigger comment - run: | - gh api repos/$REPO/issues/comments/${{ github.event.comment.id }}/reactions \ - --method POST \ - -f content='eyes' - - - name: Run Claude Strict Review - uses: anthropics/claude-code-action@v1 - env: - ANTHROPIC_BASE_URL: ${{ secrets.NVIDIA_INFERENCE_URL }} - CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS: "1" - DISABLE_PROMPT_CACHING: "1" - with: - anthropic_api_key: ${{ secrets.NVIDIA_INFERENCE_KEY }} - trigger_phrase: "/claude strict-review" - show_full_output: true - claude_args: | - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Bash(git diff:*),Bash(git show:*),Bash(git log:*),Read" - --model "${{ vars.CLAUDE_MODEL }}" - prompt: | - REPO: ${{ env.REPO }} - PR NUMBER: ${{ env.PR_NUMBER }} - BASE REF: origin/${{ steps.pr-info.outputs.base_ref }} - - Mandatory workflow — never skip or reorder: - 1. Read the PR diff first (gh pr diff). - 2. Based on the changed files and areas, identify relevant skills from skills//SKILL.md. - Common skill names: build-and-dependency, testing, cicd, linting-and-formatting, run-on-slurm, - nightly-sync, create-issue, respond-to-issue, split-pr, onboard-gb200-1node-tests. - 3. Read the SKILL.md files for all relevant areas using the Read tool. - 4. Only then perform the review using the skill context. - - You are performing a strict, comprehensive code review on a **Megatron-LM** Pull Request. - Megatron-LM is NVIDIA's large-scale distributed training framework for LLMs. - Review the diff with a focus on **implementation correctness**, **training performance**, and **backward compatibility**. - - ## Review Procedure - - 1. Get PR metadata: `gh pr view $PR_NUMBER --repo $REPO --json title,body,baseRefName,headRefName,files,additions,deletions,changedFiles,author` - 2. Get the full diff: `gh pr diff $PR_NUMBER --repo $REPO` - - For large PRs (>50 files), prioritize source code over config/lock/auto-generated files. - 3. For each significant changed file, read the full file for surrounding context. - 4. Trace data flow and dtype through computation paths to verify correctness. - 5. For each newly introduced variable/argument/field, verify it has a meaningful runtime use path (see Mandatory Check below). - 6. Post findings as inline comments with severity and category tags. - - ## Critical Issues (Must Fix) - - ### Implementation Correctness - - **dtype handling**: Verify operations use the correct dtype at each computation stage — explicit casts must be present at mixed-precision boundaries (e.g. fp16 compute → fp32 accumulation → fp16 output) - - **Loss scaling logic**: Verify DynamicLossScaler changes correctly detect inf/nan, adjust scale factor, and skip optimizer steps — incorrect logic causes training divergence or silent underflow - - **Reduction operations**: Verify reductions (sum, mean, allreduce) use correct dtype, reduction dimension, and normalization factor — wrong dimension or missing fp32 upcast produces silently wrong gradients - - **Normalization layers**: Verify LayerNorm/RMSNorm compute variance and mean on the correct dimension, with correct epsilon placement and upcast before rsqrt - - **Attention computation**: Verify QK^T scaling factor, softmax input dtype, causal mask application, and dropout placement match the intended algorithm - - **Residual connections**: Verify the correct tensor is added (pre-norm vs post-norm) with appropriate dtype for accumulation - - **Optimizer updates**: Verify state updates follow the correct formula — momentum/variance update order, bias correction, weight decay application - - **Gradient clipping**: Verify norm computation uses correct parameter set, norm type (L2 vs inf), and fp32 dtype - - **Embedding/output layer**: Verify weight tying is correctly wired, logit projection uses the right matrix, and output dtype matches expectation - - **MoE routing/aux loss**: Verify expert routing logic (top-k selection, capacity enforcement, token dropping) and auxiliary loss computation follow the intended algorithm - - ### Correctness - - **Tensor parallel**: Incorrect scatter/gather or allreduce placement — silent wrong results across TP ranks - - **Pipeline parallel**: Wrong microbatch scheduling, missing send/recv synchronization, incorrect grad accumulation across pipeline stages - - **Sequence parallel**: Incorrect sequence dimension partitioning or missing allgather/reduce-scatter in SP regions - - **Context parallel**: Incorrect KV cache partitioning or ring attention implementation errors - - **Expert parallel**: Token routing/dispatch errors across EP ranks, incorrect capacity factor handling - - **Gradient accumulation**: Missing no_sync() context or incorrect division factor when accumulating across microbatches - - **Checkpoint save/load**: State dict key mismatch, missing optimizer states, incorrect RNG state restoration — causes silent divergence after resume - - **RNG state management**: Incorrect random seed handling across TP/PP/DP ranks, causing correlated dropout masks or data sampling - - ## Important Issues (Should Fix) - - ### Training Performance - - **Unnecessary CPU-GPU sync**: .item(), .cpu(), torch.cuda.synchronize(), Python-side tensor value checks in training loop — kills throughput - - **Redundant communication**: Allreduce/allgather that could be fused, overlapped with compute, or eliminated - - **Memory inefficiency**: Missing activation checkpointing on memory-heavy layers, unnecessary tensor clones or .contiguous() calls - - **Communication-computation overlap**: Missed opportunities to overlap allreduce with backward, or allgather with forward - - **Kernel launch overhead**: Python loops over small ops that should be fused into a single kernel - - **CUDA graph compatibility**: Dynamic shapes, Python-side conditionals on tensor values, host-device sync inside captured region - - ### Backward Compatibility - - **Config/argument changes**: Renamed or removed arguments without deprecation path — breaks existing training scripts - - **Checkpoint format changes**: Modified state dict keys/structure without migration logic — makes existing checkpoints unloadable - - **Default value changes**: Changed defaults for training hyperparameters or parallelism settings — silently alters behavior for users relying on defaults - - **API contract changes**: Changed function signatures, return types, or side effects in megatron/core/ without backward-compat shim - - **Model architecture changes**: Altered layer ordering, initialization, or normalization placement — existing pretrained weights become incompatible - - ### Megatron Core Process Group Usage - - In `megatron/core` production code, treat new direct reads of global process groups - from `parallel_state` as review findings unless they are clearly compatibility-only. - - Flag added calls to `parallel_state.get_*_group()` or directly imported - `get_*_group()` helpers when the surrounding code could instead receive a - `ProcessGroupCollection` or explicit `torch.distributed.ProcessGroup` from its caller. - - Do not flag `megatron/core/parallel_state.py`, `megatron/core/process_groups_config.py`, - tests, docs, initialization/bootstrap code that materializes a `ProcessGroupCollection` - from MPU globals, or explicitly documented migration fallbacks. - - This guidance is advisory and targets Megatron Core library code; do not apply it to - `megatron/training` or other training-loop code unless the PR opts into that migration. - - ### Mandatory Check: Unused New Variables / Arguments - - For each changed file, list newly added identifiers (function args, config fields, locals). - - Verify each has a meaningful read/use path — not just declaration/docstring or discard assignment (_ = new_arg). - - Use Grep to search for usage beyond declaration sites. - - Treat placeholder discard patterns as findings unless explicitly documented as temporary migration shim. - - If usage is intentionally deferred, flag and request explicit TODO + migration note. - - ## Suggestions (Nice to Have) - - ### Naming - - Name must describe what the thing *is*, not what it's *used for* - - No abbreviations in parallel/distributed code — use full names (token_dispatcher, routing_map, comm_manager, world_size) - - Naming consistency within scope for variables serving the same role - - ### Function/Method Decomposition - - Functions over ~50 lines mixing data collection, reduction, computation, and I/O should be split - - Non-trivial logic blocks embedded in a method with different primary purpose should be extracted - - ### Simplification - - Redundant operations (e.g. .reshape(()) on 0-dim tensor, two-step constructions where one suffices) - - Setup constant across training should not run on every forward pass — move to __init__ - - Dead complexity that doesn't achieve its stated purpose - - Unnecessary intermediate aliases adding indirection with no abstraction value - - ### Other - - Stale, imprecise, or misleading comments/docstrings — a wrong docstring is worse than none - - Missing shape/dtype assertions at parallelism boundaries - - ## What NOT to Comment On - - Style/formatting issues (leave to linters) - - Test code that is reasonably clear - - Clearly intentional design decisions by the author - - Pure refactoring that preserves identical behavior (verify via diff) - - Findings invalidated by deeper analysis — drop them entirely rather than hedging - - ## Comment Format - - Prefix each comment with severity and category tag: - - `**[CRITICAL Implementation]**`, `**[CRITICAL Correctness]**` - - `**[IMPORTANT Performance]**`, `**[IMPORTANT Compatibility]**` - - `**[SUGGESTION Naming]**`, `**[SUGGESTION Simplification]**` - - For each finding, explain: (1) what the issue is, (2) why it matters (impact/risk), (3) specific suggestion for fix. - - Only use inline ```suggestion blocks for simple, self-contained line replacements (typos, - renames, single-line fixes). For structural changes that add, remove, or reorganize blocks - of code, use a top-level PR comment with a code block showing the proposed change instead. - - ## Completion - - After posting all inline comments, post a summary PR comment: - - List total findings by severity (CRITICAL: N, IMPORTANT: N, SUGGESTION: N) - - Highlight the most impactful findings - - Overall assessment of the PR's risk level - - If no significant issues are found, approve the PR: - gh pr review $PR_NUMBER --repo $REPO --approve --body "Strict review passed — no significant issues found. LGTM" + uses: NVIDIA-NeMo/FW-CI-templates/.github/workflows/_claude_review.yml@v1.7.1 + with: + trigger_phrase: "/claude strict-review" + model: ${{ vars.CLAUDE_MODEL }} + prompt: | + Mandatory workflow — never skip or reorder: + 1. Read the PR diff first (gh pr diff). + 2. Based on the changed files and areas, identify relevant skills from skills//SKILL.md. + Common skill names: build-and-dependency, testing, cicd, linting-and-formatting, run-on-slurm, + nightly-sync, create-issue, respond-to-issue, split-pr, onboard-gb200-1node-tests. + 3. Read the SKILL.md files for all relevant areas using the Read tool. + 4. Only then perform the review using the skill context. + + You are performing a strict, comprehensive code review on a **Megatron-LM** Pull Request. + Megatron-LM is NVIDIA's large-scale distributed training framework for LLMs. + Review the diff with a focus on **implementation correctness**, **training performance**, and **backward compatibility**. + + ## Review Procedure + + 1. Get PR metadata: `gh pr view $PR_NUMBER --repo $REPO --json title,body,baseRefName,headRefName,files,additions,deletions,changedFiles,author` + 2. Get the full diff: `gh pr diff $PR_NUMBER --repo $REPO` + - For large PRs (>50 files), prioritize source code over config/lock/auto-generated files. + 3. For each significant changed file, read the full file for surrounding context. + 4. Trace data flow and dtype through computation paths to verify correctness. + 5. For each newly introduced variable/argument/field, verify it has a meaningful runtime use path (see Mandatory Check below). + 6. Post findings as inline comments with severity and category tags. + + ## Critical Issues (Must Fix) + + ### Implementation Correctness + - **dtype handling**: Verify operations use the correct dtype at each computation stage — explicit casts must be present at mixed-precision boundaries (e.g. fp16 compute → fp32 accumulation → fp16 output) + - **Loss scaling logic**: Verify DynamicLossScaler changes correctly detect inf/nan, adjust scale factor, and skip optimizer steps — incorrect logic causes training divergence or silent underflow + - **Reduction operations**: Verify reductions (sum, mean, allreduce) use correct dtype, reduction dimension, and normalization factor — wrong dimension or missing fp32 upcast produces silently wrong gradients + - **Normalization layers**: Verify LayerNorm/RMSNorm compute variance and mean on the correct dimension, with correct epsilon placement and upcast before rsqrt + - **Attention computation**: Verify QK^T scaling factor, softmax input dtype, causal mask application, and dropout placement match the intended algorithm + - **Residual connections**: Verify the correct tensor is added (pre-norm vs post-norm) with appropriate dtype for accumulation + - **Optimizer updates**: Verify state updates follow the correct formula — momentum/variance update order, bias correction, weight decay application + - **Gradient clipping**: Verify norm computation uses correct parameter set, norm type (L2 vs inf), and fp32 dtype + - **Embedding/output layer**: Verify weight tying is correctly wired, logit projection uses the right matrix, and output dtype matches expectation + - **MoE routing/aux loss**: Verify expert routing logic (top-k selection, capacity enforcement, token dropping) and auxiliary loss computation follow the intended algorithm + + ### Correctness + - **Tensor parallel**: Incorrect scatter/gather or allreduce placement — silent wrong results across TP ranks + - **Pipeline parallel**: Wrong microbatch scheduling, missing send/recv synchronization, incorrect grad accumulation across pipeline stages + - **Sequence parallel**: Incorrect sequence dimension partitioning or missing allgather/reduce-scatter in SP regions + - **Context parallel**: Incorrect KV cache partitioning or ring attention implementation errors + - **Expert parallel**: Token routing/dispatch errors across EP ranks, incorrect capacity factor handling + - **Gradient accumulation**: Missing no_sync() context or incorrect division factor when accumulating across microbatches + - **Checkpoint save/load**: State dict key mismatch, missing optimizer states, incorrect RNG state restoration — causes silent divergence after resume + - **RNG state management**: Incorrect random seed handling across TP/PP/DP ranks, causing correlated dropout masks or data sampling + + ## Important Issues (Should Fix) + + ### Training Performance + - **Unnecessary CPU-GPU sync**: .item(), .cpu(), torch.cuda.synchronize(), Python-side tensor value checks in training loop — kills throughput + - **Redundant communication**: Allreduce/allgather that could be fused, overlapped with compute, or eliminated + - **Memory inefficiency**: Missing activation checkpointing on memory-heavy layers, unnecessary tensor clones or .contiguous() calls + - **Communication-computation overlap**: Missed opportunities to overlap allreduce with backward, or allgather with forward + - **Kernel launch overhead**: Python loops over small ops that should be fused into a single kernel + - **CUDA graph compatibility**: Dynamic shapes, Python-side conditionals on tensor values, host-device sync inside captured region + + ### Backward Compatibility + - **Config/argument changes**: Renamed or removed arguments without deprecation path — breaks existing training scripts + - **Checkpoint format changes**: Modified state dict keys/structure without migration logic — makes existing checkpoints unloadable + - **Default value changes**: Changed defaults for training hyperparameters or parallelism settings — silently alters behavior for users relying on defaults + - **API contract changes**: Changed function signatures, return types, or side effects in megatron/core/ without backward-compat shim + - **Model architecture changes**: Altered layer ordering, initialization, or normalization placement — existing pretrained weights become incompatible + + ### Megatron Core Process Group Usage + - In `megatron/core` production code, treat new direct reads of global process groups + from `parallel_state` as review findings unless they are clearly compatibility-only. + - Flag added calls to `parallel_state.get_*_group()` or directly imported + `get_*_group()` helpers when the surrounding code could instead receive a + `ProcessGroupCollection` or explicit `torch.distributed.ProcessGroup` from its caller. + - Do not flag `megatron/core/parallel_state.py`, `megatron/core/process_groups_config.py`, + tests, docs, initialization/bootstrap code that materializes a `ProcessGroupCollection` + from MPU globals, or explicitly documented migration fallbacks. + - This guidance is advisory and targets Megatron Core library code; do not apply it to + `megatron/training` or other training-loop code unless the PR opts into that migration. + + ### Mandatory Check: Unused New Variables / Arguments + - For each changed file, list newly added identifiers (function args, config fields, locals). + - Verify each has a meaningful read/use path — not just declaration/docstring or discard assignment (_ = new_arg). + - Use Grep to search for usage beyond declaration sites. + - Treat placeholder discard patterns as findings unless explicitly documented as temporary migration shim. + - If usage is intentionally deferred, flag and request explicit TODO + migration note. + + ## Suggestions (Nice to Have) + + ### Naming + - Name must describe what the thing *is*, not what it's *used for* + - No abbreviations in parallel/distributed code — use full names (token_dispatcher, routing_map, comm_manager, world_size) + - Naming consistency within scope for variables serving the same role + + ### Function/Method Decomposition + - Functions over ~50 lines mixing data collection, reduction, computation, and I/O should be split + - Non-trivial logic blocks embedded in a method with different primary purpose should be extracted + + ### Simplification + - Redundant operations (e.g. .reshape(()) on 0-dim tensor, two-step constructions where one suffices) + - Setup constant across training should not run on every forward pass — move to __init__ + - Dead complexity that doesn't achieve its stated purpose + - Unnecessary intermediate aliases adding indirection with no abstraction value + + ### Other + - Stale, imprecise, or misleading comments/docstrings — a wrong docstring is worse than none + - Missing shape/dtype assertions at parallelism boundaries + + ## What NOT to Comment On + - Style/formatting issues (leave to linters) + - Test code that is reasonably clear + - Clearly intentional design decisions by the author + - Pure refactoring that preserves identical behavior (verify via diff) + - Findings invalidated by deeper analysis — drop them entirely rather than hedging + + ## Comment Format + + Prefix each comment with severity and category tag: + - `**[CRITICAL Implementation]**`, `**[CRITICAL Correctness]**` + - `**[IMPORTANT Performance]**`, `**[IMPORTANT Compatibility]**` + - `**[SUGGESTION Naming]**`, `**[SUGGESTION Simplification]**` + + For each finding, explain: (1) what the issue is, (2) why it matters (impact/risk), (3) specific suggestion for fix. + + Only use inline ```suggestion blocks for simple, self-contained line replacements (typos, + renames, single-line fixes). For structural changes that add, remove, or reorganize blocks + of code, use a top-level PR comment with a code block showing the proposed change instead. + + ## Completion + + After posting all inline comments, post a summary PR comment: + - List total findings by severity (CRITICAL: N, IMPORTANT: N, SUGGESTION: N) + - Highlight the most impactful findings + - Overall assessment of the PR's risk level + + If no significant issues are found, approve the PR: + gh pr review $PR_NUMBER --repo $REPO --approve --body "Strict review passed — no significant issues found. LGTM" + secrets: + NVIDIA_INFERENCE_URL: ${{ secrets.NVIDIA_INFERENCE_URL }} + NVIDIA_INFERENCE_KEY: ${{ secrets.NVIDIA_INFERENCE_KEY }}