Skip to content

fix(attention): accept optional explicit FlyDSL plans - #3052

Merged
XiaobingSuper merged 3 commits into
mainfrom
xiaobingsuper/aiter-pa-optional-plan-compat
Nov 10, 2026
Merged

XiaobingSuper merged 3 commits into
mainfrom
xiaobingsuper/aiter-pa-optional-plan-compat

Conversation

@XiaobingSuper

@XiaobingSuper XiaobingSuper commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Problem

ATOM detects aiter's explicit FlyDSL PA-plan API (ROCm/aiter#5809) by requiring work_plan to be keyword-only with no default. ROCm/aiter#7056 gave work_plan a None default for its MXFP4 path (other paths still require a plan), so current aiter main is rejected as predating #5809. On gfx1250, MiniMax-M3 has no gluon fallback and fails model startup.

Change

  • Probe presence only: plan_pa_decode, a keyword-only work_plan, and max_context_length. The default of work_plan is no longer checked: ATOM builds and passes the plan explicitly at every call site, so a default has no bearing on the call. This matches the other aiter capability probes (e.g. max_batch_size in the same file).
  • An aiter before #5809 (positional work_plan=None, no max_context_length) is still rejected.
  • The warning prints the observed pa_decode signature (or the import error) instead of always blaming an old aiter.

Validation

  • tests/test_paged_attention_dispatch.py + tests/test_minimax_m3_sparse_pa_floor.py: 182 passed on gfx1250, against both an aiter with the required work_plan and one with work_plan=None (post-#7056)
  • The probe test accepts required, None and other defaults, and rejects one fake per clause. Deleting any of the four clauses, or re-adding default is empty, default is None or default in (empty, None), fails it.
  • With the post-#7056 aiter on gfx1250 and ATOM_PA_FLYDSL=1: on main the probe is False and run_pa_decode raises "no gluon fallback"; with this PR, one BF16 decode through FlyDSL matches a torch reference (max abs err 0.002 at qlen=1, 0.003 at qlen=2)
  • aiter main (075695e8) pa_decode verified from source: keyword-only work_plan: PADecodePlan | None = None
  • Black and Ruff pass

Tracked while validating MiniMax-M3 TP1/DP4/EP4 on MI455X.

Final aiter #7205 keeps the explicit-plan API but gives work_plan a None default. Accept both required and optional-None signatures because ATOM always passes the plan explicitly.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every eligible PR before approval:

  • ✅ Pre Checkin: Black, Ruff, catalog schema validation, non-GPU unit tests

Heavy model tests:

  • ✅ Run after the PR is approved and Pre Checkin passes
  • ✅ Run immediately when an approval review is submitted
  • ✅ Can be requested before approval with labels
Label Tests
ci:full Run all heavy PR model tests: native ATOM, vLLM, and SGLang
ci:atom Run native ATOM model accuracy tests
ci:vllm Run ATOM vLLM OOT model accuracy tests
ci:sglang Run ATOM SGLang model accuracy tests

Heavy jobs are skipped when the PR is not approved and no matching ci:* label is present.
Add labels via the sidebar or gh pr edit 3052 --add-label <label>

@XiaobingSuper
XiaobingSuper marked this pull request as ready for review November 6, 2026 05:12
@XiaobingSuper
XiaobingSuper requested review from valarLip and yitingw1 and a balanced review from Copilot November 6, 2026 05:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@valar-cop valar-cop left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few small things..

any startup log or accuracy from the MiniMax-M3 TP1/DP4/EP4 run on gfx1250?

ci green.

Comment thread atom/model_ops/base_attention.py Resolved
Comment thread tests/test_paged_attention_dispatch.py Resolved
@yitingw1

yitingw1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

The fix is right and the breakage is real — I reproduced it against aiter origin/main (8e0000db3): the probe on main returns False for the installed signature, so FlyDSL is declined and gfx1250 has nothing to fall back to.

But I think the default clause should be deleted rather than widened, and the reason is in why aiter changed it.

aiter didn't make the plan optional — it made it optional for MXFP4. From origin/main:

# docstring
``work_plan`` is required and must be built with ``plan_pa_decode`` before calling decode.
``work_plan=None`` is accepted for MXFP4 only: one task per sequence

# body
if not isinstance(work_plan, PADecodePlan) and not (mxfp4 and work_plan is None):
    raise TypeError("work_plan must be a PADecodePlan")

So the two defaults map to post-#5809-pre-MXFP4 and post-#5809-post-MXFP4. Neither maps to "predates #5809" — an aiter that old has no work_plan and no max_context_length, and is already rejected by the other clauses. Running six signature shapes through the clause as it was, as this PR makes it, and with it removed:

signature main this PR removed
required (#5809 shape) True True True
optional_none (aiter main) False True True
sentinel (some third default) False False True
positional / missing max_context_length / missing work_plan False False False

The only row where this PR and removal differ is sentinel — i.e. all the clause still does is reject a default we haven't seen before, which is exactly the failure being fixed here. If aiter ever writes work_plan=_UNSET, this breaks again the same way, and again as a gfx1250 startup failure. ATOM passes a plan at every call site, so whether aiter supplies a default has no bearing on whether the call works.

base_attention.py:385, 150 lines down and probing the same aiter module, already does it the other way: "max_batch_size" not in inspect.signature(planner).parameters. So do every other aiter capability probe in the tree (gemma_norm in layernorm.py:764, scale_row_major in mhc_pre_delayed.py:55, max_split_per_batch in aiter_mla.py:128, num_valid_rows in topk_compat.py:21) — all presence-only. Line 239 is the only one that reads a default, and the only one that broke.

→ Change: drop the clause so the probe matches the idiom used in the rest of the file.

Neither accepted shape is pinned by a test. Two mutations, both green on test_paged_attention_dispatch.py + test_minimax_m3_sparse_pa_floor.py (182 passed, same as baseline):

  • remove the clause entirely → green
  • narrow it to plan.default is None, i.e. stop accepting the #5809 shape → green

no_bound is rejected for missing max_context_length, not for its default, so the required half of the new test's name has no fake behind it — it rests only on the trailing assertion against the installed aiter, which today is the optional shape.

→ Change: if the clause stays, add a def required(output, *a, work_plan, max_context_length=None) fake expecting True. If it goes, this need goes with it.

The warning misdiagnoses the failure it was just hit by. "this aiter predates the #5809 decode API" — here aiter was too new. On gfx1250 there is no fallback and startup fails, so that line is the only clue an operator gets.

→ Change: log which clause failed and the observed signature, instead of attributing every failure to an old aiter.

One more on the new test's docstring — "aiter may also allow callers to omit it" reads as a general loosening; per aiter's own docstring and body it holds for MXFP4 only, and omitting it elsewhere raises. Suggest something like "aiter gives it a default for its MXFP4 path; ATOM still always passes one."

Not verified: I have no gfx1250, so I did not reproduce the MiniMax-M3 startup failure or run TP1/DP4/EP4 — the data valar-cop is asking for. Everything above is at the signature level and does not exercise the kernel. I'm also not repeating valar-cop's two points; both still look open.

aiter #7056 defaults work_plan to None for MXFP4. ATOM always passes a
plan, so drop the default check and log the observed signature when the
probe fails.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copilot AI balanced review requested due to automatic review settings November 10, 2026 07:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@yitingw1

Copy link
Copy Markdown
Contributor

All four points are in f50114b4a, and the first one landed as removal rather than widening — thanks.

The new warning is a real improvement. It showed up in my own mutation output, which is a decent demonstration of it:

WARNING ... aiter lacks the #5809 explicit-plan decode API
(pa_decode signature: (output, query, work_plan=None)); using gluon.

Someone debugging a gfx1250 startup now sees the signature instead of being told their aiter is too old.

One thing: the test grew from 2 cases to 5, but it still pins only one of the four clauses. Deleting each clause in turn and running the file (baseline 169 passed):

clause with it deleted
hasattr(mod, "plan_pa_decode") 169 passed
plan is not None 169 passed
plan.kind is inspect.Parameter.KEYWORD_ONLY 169 passed
"max_context_length" in sig.parameters 1 failed

The reason is in the two cases that expect False:

def pre_5809(output, query, work_plan=None):      # no max_context_length
def no_bound(output, *a, work_plan, sinks=None):  # no max_context_length

Both are rejected for the missing max_context_length, never reaching the clause they look like they're about — pre_5809 is written positionally as if to exercise KEYWORD_ONLY, but it fails before that. So the suite reads as "five signature shapes covered" while establishing only that a missing max_context_length is refused.

That matters here more than it usually would: this probe's whole history is breaking because it checked one thing too many. If anyone tightens or relaxes the remaining three, nothing goes red.

→ Change: three one-line fakes, each isolating one clause, all expecting False:

def positional_plan(output, work_plan=None, max_context_length=None):  # kind
def no_plan(output, *a, max_context_length=None):                      # plan is not None
# plus one case with monkeypatch.delattr(mod, "plan_pa_decode")

Minor: in the ImportError branch sig stays None, so the warning reads "aiter lacks the ... API (pa_decode signature: None)" — which looks like the function exists with an odd signature rather than the module being absent. Worth separating the two, since diagnosis is the whole point of that line.

Give each rejected fake exactly one missing piece so every clause is
covered, and report an import failure as such in the warning.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copilot AI balanced review requested due to automatic review settings November 10, 2026 08:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@XiaobingSuper

Copy link
Copy Markdown
Contributor Author

@yitingw1 Thanks, both addressed in 210abf0:

  • Each rejected fake now misses exactly one piece (positional_plan, no_plan, no_bound, plus a case with plan_pa_decode deleted). Deleting any of the four clauses, or re-adding any of the three default checks, now fails the test.
  • An import failure now logs import failed: <error> instead of pa_decode signature: None.

@XiaobingSuper
XiaobingSuper merged commit 69bea52 into main Nov 10, 2026
62 checks passed
@XiaobingSuper
XiaobingSuper deleted the xiaobingsuper/aiter-pa-optional-plan-compat branch November 10, 2026 13:02
rdoshi-amd added a commit to rdoshi-amd/ATOM that referenced this pull request Nov 10, 2026
Brings in main through 03b584b (ROCm#3109 V4.1 gfx1250 single-launch decode
step, ROCm#3169, ROCm#3167, ROCm#3052, ROCm#3073, ROCm#3153, README news). One conflict, in
model_runner.py: ROCm#3109's step override (`_step_override` / `_served`) and
this branch's model head output (`compute_head`) sit side by side, in
__init__ and as two branches of postprocess's verdict (a tuple head
output; an overridden step's `verify_outputs`). The two are exclusive: an
overridden step returns no head output. The hi runner test's stand-in
runner gains ROCm#3109's `_served` and the stream argument of
`forward_done_event.record`.
ZhiweiYan-96 added a commit that referenced this pull request Nov 12, 2026
Resolve against #3052 by removing its version of the FlyDSL API probe as
well: _flydsl_explicit_plan_api, its gate and #3052's
test_explicit_plan_api_ignores_the_plan_default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants