Repository navigation
Add forward compatibility to get_mla_metadata_v1 - #4227
Merged
Merged
Conversation
Contributor
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Python binding for MLA metadata generation to be forward-compatible with legacy callers by reintroducing support for the older dtype_q / dtype_kv arguments while still supporting the newer per-component NoPE/RoPE dtype split.
Changes:
- Renamed the compiled binding entrypoint to
_get_mla_metadata_v1_impland bound it to the same compiled symbol viafc_name="get_mla_metadata_v1". - Added a
get_mla_metadata_v1Python wrapper that accepts legacydtype_q/dtype_kvaliases and maps them ontodtype_{q,kv}_{nope,rope}when the specific per-component dtype is not explicitly provided. - Replaced/updated the binding docstring to clarify inputs/outputs and the dtype behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ruanjm
changed the title
ruanjm
changed the title
ruanjm
added
ci:vllm
ci:kimi
amd-ruitang3
approved these changes
Jul 14, 2026
zufayu
pushed a commit
that referenced
this pull request
Jul 15, 2026
…e 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>
valarLip
pushed a commit
that referenced
this pull request
Jul 15, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ATT