Skip to content

fix(ring_attn): adapt FA3 varlen wrappers to flash_attn_3 3.0.0 kwargs - #2352

Merged
hallerite merged 2 commits into
mainfrom
fix/fa3-ring-attn-kwargs
Apr 24, 2026
Merged

fix(ring_attn): adapt FA3 varlen wrappers to flash_attn_3 3.0.0 kwargs#2352
hallerite merged 2 commits into
mainfrom
fix/fa3-ring-attn-kwargs

Conversation

@hallerite

@hallerite hallerite commented Apr 24, 2026

Copy link
Copy Markdown
Member

Summary

flash_attn_3 3.0.0 (pytorch-cu128-test, picked up in #2234 on 2026-04-09) renamed the kernel's causal argument to is_causal and split window_size into window_size_left / window_size_right. The FA3 ring-attn wrappers in _fa3_varlen_forward / _fa3_varlen_backward still update the params dict with the old causal key, which leaves the new is_causal default in place and passes both — triggering:

RuntimeError: flash_attn_3::_flash_attn_backward() expected at most 22 argument(s) but received 23

on the first backward pass for any model+config that routes through these wrappers (cp > 1 with FA3, impl='custom').

Fix

Pick the correct key based on what the installed kernel's signature exposes. Supports both the newer 3.0.0 naming and the older 3.0.0b1 naming without pinning the kernel version, so the repo stays robust as the wheel index rolls.

Validation

Observed the failure on a Qwen/Qwen3.5-35B-A3B RL config (cp=2, attn=flash_attention_3, impl=custom). With the patch applied, the forward pass succeeds end-to-end (loss computed, 0 flash_attn_backward errors in trainer log). Backward uses the identical kwarg-passing pattern, so the forward validation covers both paths by construction.

🤖 Generated with Claude Code


Note

Medium Risk
Touches the low-level FlashAttention-3 forward/backward wrapper argument passing used in distributed ring attention; a mismatch with the installed kernel signature would fail at runtime or change attention masking/windowing behavior.

Overview
Updates the FA3 ring-attention varlen wrappers (_fa3_varlen_forward/_fa3_varlen_backward) to match newer flash_attn_3 kwarg names by passing is_causal instead of causal and splitting window_size into window_size_left/window_size_right.

Removes the previous conditional window_size kwarg injection, relying on the new explicit left/right window parameters when calling _flash_attn_forward and _flash_attn_backward.

Reviewed by Cursor Bugbot for commit 66b277f. Bugbot is set up for automated code reviews on this repo. Configure here.

flash_attn_3 3.0.0 (released on the pytorch-cu128-test index, picked up
in PR #2234 on 2026-04-09) renamed the kernel's `causal` argument to
`is_causal` and split `window_size` into `window_size_left` /
`window_size_right`. The FA3 ring-attn wrapper here still updates the
params dict with the old `causal` key, which leaves the new `is_causal`
default in place and passes both — triggering:

    RuntimeError: flash_attn_3::_flash_attn_backward() expected at most
    22 argument(s) but received 23

on the first backward pass for any model+config that routes through
`_fa3_varlen_forward` / `_fa3_varlen_backward` (cp > 1 with FA3, custom
impl). Forward hit it first in our Qwen3.5 run; backward has the same
pattern.

Fix: pick the correct key based on what the installed kernel's signature
exposes. Supports both the newer 3.0.0 naming and the older 3.0.0b1
naming without pinning the kernel version.

Validated against flash_attn_3 3.0.0 (pytorch-cu128-test) by running a
Qwen/Qwen3.5-35B-A3B RL config with cp=2 + FA3 + custom impl — the
forward pass now succeeds end-to-end (loss computed); backward uses the
same kwarg-passing pattern.
@hallerite
hallerite marked this pull request as ready for review April 24, 2026 13:48
The repo pins flash_attn_3 via pytorch-cu128-test (3.0.0), and uv.lock
resolves exactly that. The older 3.0.0b1 kernel is unreachable through
any supported install path, so keeping conditional branches for its
`causal` / `window_size` argument names is dead code.

Collapse to the single 3.0.0 API: `is_causal` + `window_size_left` +
`window_size_right` set unconditionally. One way to do it.

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 66b277f. Configure here.

"causal": causal,
"is_causal": causal,
"window_size_left": window_size[0],
"window_size_right": window_size[1],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale default-args keys break older FA3 versions

Medium Severity

The PR description claims support for both old (3.0.0b1) and new (3.0.0) FA3 kwarg naming, but the implementation unconditionally hardcodes the new-style keys (is_causal, window_size_left, window_size_right). When get_default_args populates params from an older kernel that uses causal and window_size, those stale keys remain after the update() call, producing a dict with both old and new keys. This causes the same "too many arguments" RuntimeError this PR aims to fix — just in the reverse direction. The code needs to detect which keys params already contains and use matching names, or remove stale keys after the update.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 66b277f. Configure here.

hallerite added a commit that referenced this pull request Apr 24, 2026
Cherry-picked from PR #2352 (main branch). flash_attn_3 3.0.0 renamed
`causal` -> `is_causal` and split `window_size` into
`window_size_left` / `window_size_right`. Required for Qwen3.5 MoE or
any cp>1 + FA3 + custom-impl config to train without crashing the
first backward pass.

@Jackmin801 Jackmin801 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.

nice catch! lgtm

@hallerite
hallerite merged commit 20454ec into main Apr 24, 2026
9 checks passed
@hallerite
hallerite deleted the fix/fa3-ring-attn-kwargs branch April 24, 2026 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants