review-pr

# ATOM PR Review ATOM is a ROCm/AMD GPU kernel optimization layer (MI300X/MI355X) that: - Consumes aiter ops (attention, MoE, GEMM, norm, quant) - Integrates with vLLM and SGLang as a plugin/backend - Provides custom MLA, sparse attention, TBO, and quantization fusion A change here can break inference for all models using the affected kernel path. --- ## Step 1 — Fetch ```bash PR=$1 REPO="ROCm/ATOM" gh pr view $PR --repo $REPO --json title,body,number,labels,files,author,reviews,comments > /tmp/pr_meta.json gh pr diff $PR --repo $REPO > /tmp/pr.diff # Linked issue ISSUE=$(cat /tmp/pr_meta.json | python3 -c " import json,re,sys body = json.load(sys.stdin).get('body','') or '' m = re.search(r'(?:fix|close|resolve)[s]?[: ]*#(\d+)', body, re.I) print(m.group(1) if m else '') ") [ -n "$ISSUE" ] && gh issue view $ISSUE --repo $REPO --json title,body > /tmp/pr_issue.json # Prior reviewer comments (top-level) cat /tmp/pr_meta.json | python3 -c " import json,sys d = json.load(sys.stdin) for r in d.get('reviews',[]): b = (r.get('body','') or '').strip() if b: print(f'[REVIEW {r[\"author\"][\"login\"]}] {b[:200]}') for c in d.get('comments',[]): b = (c.get('body','') or '').strip() if b: print(f'[COMMENT {c[\"author\"][\"login\"]}] {b[:200]}') " # Inline review comments (line-level — often more specific than top-level) gh api "repos/$REPO/pulls/$PR/comments" | python3 -c " import json,sys comments = json.load(sys.stdin) for c in comments: author = c.get('user',{}).get('login','') body = (c.get('body','') or '').strip() path = c.get('path','') line = c.get('line') or c.get('original_line','') if body and 'copilot' not in author.lower() and 'bot' not in author.lower(): print(f'[INLINE {author}] {path}:{line}') print(f' {body[:250]}') " 2>/dev/null ``` Read the diff and PR body before proceeding. **Cross-file verification — before reporting any kernel/dispatch finding.** The diff shows changed lines, not the whole story. Grep the entire symbol family (`.cu` + `.cuh` + `.h`, or the whole module), not just files in the diff — sync/fence/atomics or the "other half" of a scatter often live in a header, and dispatch/else-branch completeness must be read in the full function, not the hunk. A "no synchronization" or "missing branch" finding based only on the diff is how false positives happen. **Classify every CI failure before blaming the PR.** A red check is not automatically the PR's fault: - Read the failed step/service. An external `docs/readthedocs.com` build failing on a `.claude/`-only or code-only change is unrelated infra (those files do not feed the RTD sphinx docs) — do not treat it as a content failure (ATOM#1549). - Compare against main: if main fails the same job in the same window, it is baseline/flaky, not a regression introduced here. - Expired CI logs (`HTTP 410 Gone`) on old runs mean the failure is months-stale and meaningless against today's main — ask for a rebase + fresh run instead of quoting it. --- ## Step 2 — Semantic Understanding (answer before rules) **Q1 — What specifically changed computationally?** Not "improves MLA" — which kernel path, what data flow, what formula? _Answer:_ **Q2 — Hardware + model scope: which arch(es), which model family/families?** gfx942 / gfx950? DeepSeek V3 / V3-0324 / Kimi K2.5 / GPT-OSS / GLM? TP config? _Answer:_ **Q3 — Does this introduce or modify aiter op usage?** New `from aiter import X`? New kwargs on existing aiter calls? Removed aiter calls? _Answer:_ **Q4 — Performance claim: what is the mechanism?** Not "faster" — WHY? (fewer kernel launches, fused allreduce+norm, better tiling?) _Answer:_ **Q5 — Does the description explain WHY or only WHAT?** "Enable ar+norm+quant fusion" = surface. "Eliminates 2 intermediate HBM round-trips between allreduce, rmsnorm, and quant by calling the aiter fused kernel" = understanding. _Answer:_ --- ## Step 3 — PR Type Classification - [ ] **Performance optimization** → P1 (benchmark numbers), P5 (setup cost excluded?), P2 (production shapes), P3 (reproducible) - [ ] **New aiter op usage / aiter API change** → E1 (aiter dep), B5 (param added/removed/renamed propagation), B6 (new kwarg unhandled by all ATOM dispatch branches?), E3 (dead param) - [ ] **API signature change** (param added / removed / renamed in an ATOM function or base class, default or return type changed) → B5 (propagation to all receivers), E1 (aiter dep if aiter-side), E2 (plugin bridge sync) - [ ] **New constexpr / routing flag / new attribute key added** → B6 (do ALL dispatch branches handle the new value, or assert on it?), C3 (new arch string literal?) - [ ] **Model integration** (`atom/models/*.py`) → A2 (shared backbone coverage), Step 4 risk - [ ] **Dispatch logic change** → B1 (silent bypass), B2 (phase proxy), B6 (new value unhandled?), A3 (scope too broad) - [ ] **cudagraph / capture path** → B3 (cudagraph compatibility) - [ ] **Plugin / vLLM / SGLang interface** → E2 (bridge sync) - [ ] **Config / tuning** → A2 (other GPU archs), A3 (scope too broad) - [ ] **Bug fix** → A1 (sibling variant), HK3 (regression test); root cause explained? - [ ] **FP8 / quantization** → C1 (fnuz by dtype), C2 (dtype hardcoded) - [ ] **Weight transform / new weight attr** → F1 (double HBM pin) - [ ] **Async / multi-stream / weight prep** → G1 (stream sync missing), G1b (blocking queue.get without timeout in serving code) - [ ] **New if/elif dispatch with variable assignment** → D1b (UnboundLocalError on uninitialized path) - [ ] **New `@compile_ops` / `torch.library.custom_op`, or change to an op's return dtype/arity** → D7 (fake/abstract impl exists?), D6 (fake dtype/shape matches real op?) - [ ] **Kernel launcher / buffer-offset or index arithmetic (long-context or large-batch path)** → D9 (int32 overflow at production scale) - [ ] **New aiter / C-extension kernel call** → D8 (contiguous check) - [ ] **Removes or reverses a zero-init / assert / `.contiguous()` / documented invariant** → D4 (invariant reversal cited?) --- ## Step 4 — Backbone File Risk Assessment **What makes an ATOM file "backbone"?** Apply these questions to any file in the diff. ``` Q1 — Tier 1 test: Is this file executed on EVERY forward-pass request, regardless of which model is being served? (model_runner, engine_core, scheduler, config = YES) → YES → Tier 1 (system-critical: every inference is affected) Q2 — Tier 1 alt: Is this file a base class inherited by >2 production model implementations, so a bug here affects all of them even if the PR says "model-specific fix"? (deepseek_v2.py is base for DSv2/V3/V3-0324/Kimi = YES) → YES → Tier 1 Q3 — Tier 2 test: Does this file implement an op (attention, linear, norm, MoE) that is shared across >1 model family, where a correctness bug silently produces wrong results for all users of that op? (attention_mla.py, linear.py, moe.py = YES) → YES → Tier 2 Q4 — Tier 3 note: Is this a plugin bridge file (vllm/*.py, sglang/*.py)? → Tier 3 by blast radius, but HIGH VISIBILITY — only plugin users are affected, but those users see the API break immediately. Otherwise → Tier 3 (model-specific or kernel-specific). ``` Key difference from aiter: ATOM has no `import atom` — Tier 1 is defined by "executes on every request" or "base class for multiple model families", not by import chain. The table below is the current snapshot; Q1–Q4 classify new files not yet listed. Backbone files ranked by git commit frequency (2025–2026) and blast radius: | Tier | File | Git commits | Blast radius | Common failure mode | |------|------|-------------|-------------|---------------------| | **1** | `atom/model_engine/model_runner.py` | 158 | **ALL** inference — every forward pass | OOM, cudagraph break, wrong batch assembly | | **1** | `atom/config.py` | 91 | All models — config drives dispatch | Wrong model config silently changes kernel path | | **1** | `atom/models/deepseek_v2.py` | 68 | DSv2/V3/V3-0324/Kimi base class | Wrong MLA, OOM, accuracy drop for all DSv* | | **2** | `atom/model_ops/moe.py` | 69 | All MoE models | Wrong expert routing, double weight pinning | | **2** | `atom/model_engine/scheduler.py` | 68 | Request batching for all models | Stall, wrong decode/prefill split | | **2** | `atom/model_ops/attention_mla.py` | 54 | All MLA attention paths | Wrong KV, accuracy drop, crash | | **2** | `atom/model_ops/attentions/aiter_mla.py` | 52 | aiter MLA dispatch | Wrong kernel, wrong dtype | | **2** | `atom/model_ops/linear.py` | 49 | All linear layers (every model) | Wrong GEMM dispatch, wrong quant | | **2** | `atom/model_ops/attention_mha.py` | 46 | All MHA models | Wrong attention output | | **2** | `atom/model_ops/layernorm.py` | 32 | Norm + quant fusion path | Wrong scale, wrong dtype | | **2** | `atom/models/deepseek_v4.py` | 34 | DSv4 / Kimi-K2.5 specific | Wrong sparse MLA, SWA layout break | **Tier-1 special rule**: When `model_runner.py` or `config.py` is touched, ask: does the change interact with cudagraph capture? Any new Python control flow, dynamic tensor allocation, or attribute lookup inside the captured region will silently break cudagraph. **`deepseek_v2.py` special rule**: Base class for DSv2, DSv3, DSv3-0324, and Kimi. A bug here affects all four model families even if the PR says "Kimi-only fix". Check: is the changed method overridden in subclasses? If not, all variants are affected. **Mandatory backbone checks — must be answered before writing the verdict:** For **Tier 1** files (model_runner, config, deepseek_v2, scheduler): - [ ] List every function/method changed. Grep for callers: `grep -r 'def <name>' atom/models/ atom/model_engine/`. If any caller not mentioned in the PR exists, flag it. - [ ] For `deepseek_v2.py`: run `grep -rn 'def <changed_method>' atom/models/` — does any subclass override it differently? If not overridden → all DSv2/V3/V3-0324/Kimi are affected. - [ ] Is there an integration test (full forward pass, not unit test alone) exercising this path after the change? If not → `📝 HK3` - [ ] State explicitly: if this change is wrong, what breaks? (crash / silent wrong value / OOM / cudagraph break) and how would it be detected? For **Tier 2** files (moe, attention_mla, linear, aiter_mla, scheduler): - [ ] Which model families use this op/file? List them. Is at least one from each family tested? - [ ] Are production shapes covered? (TP=4, TP=8, decode single-token, prefill ISL≥4096) - [ ] Does the change affect the FP8 path, the BF16 path, or both? If both, are both tested? **AI code red flag — verbatim duplication across backbone files:** If the same algorithmic block appears in 2+ backbone files with only variable names changed (same formula, same comments, same structure), ask: was each file's invariant verified independently, or was the fix copy-pasted? See D5. --- ## Step 5 — Rule Checklist Six failure categories — work all six in order. Severity: 🔴 block / ⚠️ should fix / 📝 note. **🔴 gate — before firing any 🔴, write down the concrete input that triggers it.** Name the specific shape / scale / dtype / arch / value that makes the finding fire (e.g. "at `token_id` > 16M with H=32, D=128 the int32 product exceeds 2^31", or "when the new attribute is absent the `getattr` default silently drops shared-expert slots"). If you cannot state a concrete triggering case, the 🔴 is unproven — **downgrade to ⚠️ ("worth checking") or drop it.** A 🔴 that reads as a definite blocker but names no demonstrable triggering input is exactly how a false positive lands on a maintainer's PR. This gate applies to every rule below — including those whose own text omits an explicit FP self-check (e.g. D9): the same index expression is safe in a capped/small-batch path and unsafe only at a scale you must actually exhibit. | Category | Core question | Key triggers | |---|---|---| | **A. Coverage gaps** | Same bug elsewhere? Shared path other models? | `_opt`, `_prefill_opt`, `_v2`; shared backbone; broad condition | | **B. Silent bypass** | Every input reaches the right branch? | gated-off param; phase proxy (`max_q`); string alias | | **C. Hardcoded arch/dtype** | Does the constant break on another GPU or config? | `bf16` fixed; `fp8_e8m0` fixed; `gfx942` assumed | | **D. Uninitialized state** | Is the buffer clean before kernel launch? | `::empty()`+`atomic`; cudagraph dynamic allocation | | **E. Cross-repo sync** | Does the consumer know? | new aiter symbol; new param nobody passes; plugin bridge | | **F. Resource duplication** | Does the change double HBM silently? | new `_preshuffled`/`_quantized` weight alongside original | --- ### A — Coverage Gaps _"Fixed one path; the same bug lives in a sibling."_ **A1 — Sibling function/kernel not fixed** ⚠️ (🔴 if Tier-1/2 backbone) Fix changes address calc, bounds check, or data layout: scan same file for variants named `_opt`, `_prefill_opt`, `_decode`, `_v2`. Real example (aiter#3841): strided q_nope fix on decode kernel; `_prefill_opt` in same file had same bug. → `⚠️ A1: same bug may exist in [variant] — check function family in this file` **A2 — Change covers one model/GPU, shared path affects others** ⚠️ PR labeled "[MI308]" or "DSv4-only" but touches a backbone file shared with Kimi/DSv3/gfx950: - Special: `deepseek_v2.py` is the base class for DSv2, DSv3, DSv3-0324, and Kimi — a "Kimi-only fix" here affects all four. - If benchmark only shows one GPU arch, ask about the other. Real example (ATOM#1498): "[MI308]" backbone change still affects gfx950 (MI355X). → `⚠️ A2: [change] labeled [scope] but shared backbone [file] also affects [other models/archs]` **A3 — Activation condition broader than validated scope** ⚠️ New dispatch enables kernel for model family X, tested only on subcase Y. Real example (vLLM#16435): FusedMoE activated for wrong families → follow-up restrict PR needed. → `⚠️ A3: activation condition enables [X] but only [Y] was tested` --- ### B — Silent Bypass _"The code looks complete but certain inputs silently take the wrong path."_ **B1 — Dispatch gate with unchecked parameter** 🔴