Repository navigation
MOE: add AITER_MOE_FORCE_BF16_ACT to force bf16 activations (opt-in) - #3593
Conversation
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
zufayu
left a comment
There was a problem hiding this comment.
CI passed (Black fixed, Standard Tests all green on MI300X + MI35X). LGTM.
zufayu
left a comment
There was a problem hiding this comment.
CI passed (Black fixed, Standard Tests all green on MI300X + MI35X). LGTM.
|
This PR was merged (c006439) and then reverted via #4225 — flagging why, since the timeline looks confusing from the outside. To be clear: the revert is not because anything was broken. The change is a harmless default-off opt-in. It was reverted because these MoE routing env vars are being treated as debug / internal-only tech debt rather than stable feature API — tracked in #4226, along with the ~14 other The underlying need is legitimate — if you need an fp4/fp8 activation-precision fallback for your deployment, please re-approach it through the config/API direction in #4226 rather than a new env var, and we'll help land it that way. Thanks for the contribution, and sorry for the churn. |
…/inferred finding discipline From a full day of real reviews (aiter#3802/#4098/#2565/#4029/#3593/#4171): - grep .cu+.cuh+.h, not just diff files, before claiming missing sync/branch - classify CI failures as infra-flake vs real regression before blaming the PR - tag findings [verified]/[inferred]; never ship an unconfirmed root cause
…pe + HK9) aiter maintainers generally reject new permanent env-var flags: AITER_MOE_FORCE_BF16_ACT (#3593) was reverted by #4225. Reconcile with D2 by scoping D2 to a *temporary rollback kill-switch* only, and upgrade HK9 from 📝 "document it" to⚠️ "prefer auto-derivation; permanent knobs face pushback". ATOM unchanged — it manages env vars via atom/envs.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…error, D10b FlyDSL (#4171) * review-pr skill v2: dispatch gap rule, arch-string FP fix, P5 timing error, D10b FlyDSL Key changes based on 191-PR test sweep (88 aiter + 93 ATOM): - B4 (new): "New dispatch value not handled by all paths, no warning" Highest-frequency finding (~18% of PRs). Covers: new dtype/arch/flag added to a multi-way dispatch where some branches silently fall through to wrong behavior with no assert/warning. Includes FP self-check to filter upstream guards. Old B4 (over-conservative assert) moved to B7. - C4 (new): "New GPU arch string literal in dispatch condition" FP self-check front-loaded: search unchanged lines first; if same arch string already exists → skip (pre-existing style). Fires only on genuinely new introductions. - D10b (new): "FlyDSL arith.bitcast requires arith.unwrap() on operand" Raw DSL value passed to arith.bitcast causes JIT type error at first invocation of that shape/dtype combo. Silent in Python, crashes at kernel JIT time. Real example: aiter#3944 bf16/f16 output path. - P5 (new): "Benchmark timing excludes one-time setup cost" Distinct from P1 (missing numbers). Covers: shuffle_weight, first-call JIT compile, precompile excluded from timing window → claimed speedup is actually a regression. Real examples: aiter#4166 (1.14x claim → 0.83x actual), aiter#3944 (7 precompile kernels on live stream). - Step 3 classification: wired B4/P5/D10b into trigger lines so new constexpr/routing-flag PRs and FlyDSL PRs auto-prompt the right checks. - Step 8 output format: findings now require three parts (Problem + Impact + Action verb). "Author must" / "Reviewer should ask" required; no verb = do not include in output. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * review-pr skill: B6→parametric family, G1b, D1b, C4 FlyDSL exemption - B6 restructured into a 6-row parametric table (param-discard, param-removed, repr-key, arch-discard, dispatch-silent, rename) — unified mental model for all API propagation incompleteness sub-types - G1b: blocking queue.get() without timeout in production serving code - D1b: Python-side UnboundLocalError from conditional variable assignment (ATOM#860 real example; static analysis blind spot) - C4: add FlyDSL capability guard exemption — get_gfx() inside a kernel-specific wrapper is not centralized dispatch hardcoding (aiter#3870) - Step 3 classification: new trigger entries for G1b and D1b Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * review-pr: add cross-file verification + CI-failure triage + verified/inferred finding discipline From a full day of real reviews (aiter#3802/#4098/#2565/#4029/#3593/#4171): - grep .cu+.cuh+.h, not just diff files, before claiming missing sync/branch - classify CI failures as infra-flake vs real regression before blaming the PR - tag findings [verified]/[inferred]; never ship an unconfirmed root cause * review-pr: add E4 (downstream CI coverage / ci:all) + E5 (stable-API owner sign-off) + Step-3 trigger E4: downstream tests (ATOM/SGLang/vLLM) are skip-by-default; a change to a downstream-consumed op can pass aiter CI and break downstream after merge (aiter#3459). Adds dispatch-reachability + model->label triage. E5: behavior/signature/dispatch changes to stable core APIs (fused_moe/mla/attention/quant/jit-core) need de-facto owner sign-off before merge, not a post-merge revert (aiter#3593). * review-pr: add global 🔴 gate — must name concrete triggering input before firing 🔴 Generalizes the per-rule FP self-check (present on B4/C4/B7) into a mandatory gate over all 🔴 findings, closing the gap on rules like D9 that omit one. A 🔴 with no demonstrable triggering shape/scale/input must be downgraded or dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: fix P5 — #4166 was a false positive, not a real example aiter#4166 preshuffles the static weight once outside the timing loop and honestly reports geomean 0.69x; it never claimed 1.14x. The old example taught the amortization error (charging a one-time cost per call). Reframe P5 to fire only on costs that recur per call / per cold start, keep #4166 as a counter-example, and drop the unverifiable #3944 specifics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: Step 6 — add structural verification for AI-generated code The description-smell table only pre-filters; AI fails structurally. Add six action-forcing checks that each yield a verified/inferred finding: hallucinated -symbol sweep, twin divergence (copy-paste half-adapted), claim/comment↔code + number provenance, safety theater, test-calibrated-to-pass, magic constant without derivation. Motivated by this skill's own fabricated-1.14x P5 bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: close Step-3 routing gaps + add FP self-checks + staleness guards ① Route D6/D7 (fake fns), D8 (contiguous), D9 (int32), D4 (invariant reversal), B7 (over-conservative assert), F1 (double HBM) into Step 3 — they were only reachable by scanning all of Section D. ② Add FP self-checks to B2 (tl.load: is the dim provably aligned?) and C2/C3 (search unchanged lines before firing a "new" hardcode), matching C4/D9. ③ Staleness guards on E4 label→model map and P2 production shapes — a stale snapshot produces confidently-wrong recommendations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: fix B6 (API-modification rule) — route it + widen public-API scope B6 (API propagation) was the rule most about API changes yet had NO Step-3 entry, reachable only by scanning Section B. Route it via a new "API signature change" row + the refactor/rename row. Also fix param-removed/rename scope: a public-op param removal/rename breaks cross-repo consumers (ATOM/SGLang/vLLM), not just same-file call sites — widened, and linked to E1/E5 for public contracts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: add FP self-check to B6 rename/param-removed (found by live test on #4227) Live-testing the skill on aiter#4227 showed B6's rename/param-removed rows had no own FP self-check: a rename shielded by a same-named wrapper + fc_name would read as a 🔴 break if followed verbatim (only the global 🔴 gate rescued it). Add a self-check to confirm no compatibility shim (same-named wrapper / alias / fc_name binding pin) before firing, with #4227 as the real non-example. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: E5 — reviewer must proactively @-mention the API owner, not wait Strengthen E5's action from passive "get sign-off" to active: the reviewer must post a PR comment @-mentioning the de-facto owner (@valarLip for fused_moe) with a one-line summary + explicit approval request before merge. Finding is unresolved until the owner has been actively pinged and responded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: reflect that aiter rejects permanent env-var knobs (D2 scope + HK9) aiter maintainers generally reject new permanent env-var flags: AITER_MOE_FORCE_BF16_ACT (#3593) was reverted by #4225. Reconcile with D2 by scoping D2 to a *temporary rollback kill-switch* only, and upgrade HK9 from 📝 "document it" to⚠️ "prefer auto-derivation; permanent knobs face pushback". ATOM unchanged — it manages env vars via atom/envs.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: root <root@hjbog-srdc-24.amd.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Adds an opt-in environment variable,
AITER_MOE_FORCE_BF16_ACT, that forcesbf16 activations for the
per_1x32(MXFP4) SwiGLU MoE paths infused_moe_.By default the activation dtype is auto-selected:
bf16for small M,fp4x2for large M (prefill)fp8on gfx950 for large MMake the per_1x32 SwiGLU bf16→fp8 activation threshold configurable via
AITER_MOE_BF16_FP8_BOUND (default 512) to avoid the gfx950 fp8 prefill regression for MXFP4 w4a16. This flag lets them opt in without code changes.
Behavior
AITER_MOE_FORCE_BF16_ACT=1→q_dtype_a = bf16on theper_1x32SwiGLUpaths, for both small and large M.
0(default) → no change; the existing auto-selection logic runsexactly as before.
The flag is process-scoped and default-off, so it has no impact on existing
users, other models, quant types, or architectures.
Trade-off
Forcing bf16 activations skips the fp4x2/fp8 fast paths, trading throughput for
activation precision. It is intentionally opt-in so only deployments that need
it pay that cost.
Test plan
per_1x32 SwiGLU separated/interleaved cases (bf16/fp4x2/fp8 as before).
AITER_MOE_FORCE_BF16_ACT=1: verified the MoE dispatch stays on the bf16activation path for both decode (small M) and prefill (large M) on gfx950
with an MXFP4 W4A16 model; no fp4x2/fp8 activation path taken.