[Rotary] Separate varlen lengths from cache capacity - #1204
[Rotary] Separate varlen lengths from cache capacity#1204taking-lying-flat wants to merge 4 commits into
Conversation
Use logical packed sequence lengths for rotary validation and FlashAttention launch parameters. Avoid per-layer left-padding scalar synchronizations and remove the redundant batch axis from varlen rotary grids.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
zhiyuan1i
left a comment
There was a problem hiding this comment.
Style issues:
-
P1: 10 layers have the dead branch
rope_cache_length = (q_len + seqlen_offset if cu_seqlens is None else rope_cache_length + seqlen_offset)— both arms equalrope_cache_length + seqlen_offsetsince initialization already branches on cu_seqlens. All 10 should berope_cache_length += seqlen_offset. -
P2:
rotary.py:188and NPUrotary.py:163hand-writecu_seqlens[1:] - cu_seqlens[:-1]— this PR just unifiedprepare_lens(cu_seqlens), reuse it. -
P2:
nsa.py:97—batch_max_seqlenis single-use (nsa doesn't go through flash attn), inline it like parallax/yoco. -
P2:
moba.py:177-180—rotary_cu_seqlensuses ternary,cu_seqlens_cpuuses if — pick one style. -
P2:
multiscale_retention.py:233/239,yoco.py:180/186— adjacent lines useq.shape[1]andq_leninterchangeably, unify toq_len.
Summary
Packed varlen currently overloads one
max_seqlenvalue with three different meanings: the RoPE cache capacity, the batch's logical maximum sequence length, and the configured context limit. This can make a 4K/32K packed batch allocate and launch as if it were 256K.This PR:
get_max_seqlen(cu_seqlens, cu_seqlens_cpu)helper;batch_max_seqlenfromrope_cache_lengthacross rotary-using attention layers;B > 1grid, logical cache capacity, and FlashAttention launch arguments with regression tests.Implementation checklist
max(diff(cu_seqlens)), not packedT.max()/.item()on per-sample CUDA offsets in each layer.(token chunk, head).Test plan
Hardware and environment: NVIDIA RTX A1000 Laptop GPU (4 GiB), PyTorch 2.13.0+cu130, CUDA 13.x, locally rebuilt
flash-attn==2.8.3.post1.pytest -q tests/modules/test_rotary.py tests/ops/utils/test_index.py tests/layers/test_attn_varlen_pack_layout.pytests/ops/test_attn.pyandtests/ops/test_moba.pyflash_attn_varlen_funccausal forward/backward smoke testpre-commit run --files <all changed files>The full large naive-reference configurations in
tests/ops/test_deltaformer.pyandtests/ops/test_nsa.pyexceed this GPU's 4 GiB capacity. Their smaller related packed-varlen forward/backward cases pass. Ascend changes were statically reviewed and linted but could not be executed without NPU hardware.Benchmark / NCU (kernel changes only)
Same RTX A1000 Laptop GPU before/after. NCU was not collected.
Packed prefill/cache capacity
BF16, rotary dim 128, eight 4K segments, 32K packed tokens, configured context 256K:
Left-padded decode synchronization
Real FlashAttention, FP16, 8 Attention layers, hidden size 128, 128-token prefill, 64 measured decode steps:
This intentionally small decode workload makes host synchronization visible. Larger models will spend a greater fraction in GEMMs, so the percentage improvement should not be extrapolated directly. The removed scalar synchronization itself measured approximately 13.8 / 76.2 / 286.7 microseconds per affected layer at batch sizes 1 / 8 / 32.
Dense rotary math and its grid are unchanged; the physical-grid change applies only when
cu_seqlensis present.Breaking changes
None. The new DeltaFormer maximum-length argument is optional, and the MLA
attention_maskdefault is backward-compatible.Checklist