Skip to content

[Bugfix][Test] Rotate KV into RHT domain in the INT4 unified attn test - #62881

Merged
jikunshang merged 1 commit into
vllm-project:mainfrom
dudilester:fix-int4-unified-attention-test-rht
Nov 4, 2026
Merged

jikunshang merged 1 commit into
vllm-project:mainfrom
dudilester:fix-int4-unified-attention-test-rht

Conversation

@dudilester

@dudilester dudilester commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Fix the INT4 case of test_triton_unified_attention_per_token_head_scale. It filled the INT4 cache with unrotated K/V and compared the result against a reference that didn't match what unified_attention_int4 computes. The test now writes the cache in the RHT domain, like reshape_and_cache_int4, so it actually checks the INT4 attention kernel.

Follow-up to #40835. Removes the Intel CI deselect of [int4-16-128-num_heads0-seq_lens1]. Downstream report: intel/intel-xpu-backend-for-triton#8358.

Not a duplicate: no open PR or issue fixes this test. #62113 edits the same test but keeps the INT4 reference as is and widens the Intel deselect to two INT4 cases. See Details for how the two PRs combine.

Claims

  • The kernel output and the reference now agree (correlation 1.0000 vs ≈ 0.00 before). The test no longer passes or fails depending on the RNG stream.
  • INT4 uses the same atol=rtol=5e-2 as INT8/FP8, down from 0.5. This also removes the Hopper-only atol=1.1.
  • The Intel CI deselect for this case is removed.
  • Test-only change. No kernel or runtime code is modified.

Validation

Test commands run:

pytest -q tests/quantization/test_per_token_kv_cache.py
# plus a probe that reruns the INT4 attention test over 200 seeds
# (set_random_seed(seed)) and records corr(output_q, output_ref),
# max |output_q - output_ref|, and how many seeds fail atol=rtol=5e-2.

Results:

Platform Test Case corr(out_q, ref) worst abs diff seeds failing
Intel BMG (Arc Pro B65, and a G21 board) before seq_lens0 ≈ 0.00 0.94 —
before seq_lens1 ≈ 0.00 1.79 fails with the default seed
after, tol 5e-2 seq_lens0 1.0000 0.006 0/200
after, tol 5e-2 seq_lens1 1.0000 0.012 0/200
NVIDIA RTX 3060 Ti (sm_86) before, tol 0.5 seq_lens1 +0.004 1.89 62/200
after, tol 5e-2 seq_lens0 1.0000 0.015 0/200
after, tol 5e-2 seq_lens1 1.0000 0.021 0/200

CI coverage:

  • The H200 MIG "Quantization" job runs this file, so CI covers the removed Hopper special case and the hadacore RHT path.
  • The Intel quantization job now runs the formerly deselected case.

Not validated locally:

  • Hopper and the hadacore path. On the RTX 3060 Ti the RHT ran through the Triton Hadamard kernel; the H200 job above covers both.
  • ROCm.

Details

Root cause. reshape_and_cache_int4 stores K_s = H·D·k (RHT, with H·H = d·I, D·D = I). unified_attention_int4 relies on that layout:

  • it rotates q' = H·D·q and uses softmax_scale / d, so q'·K_s / d = q·k;
  • it applies D·H(·) / d to the output, which undoes the rotation of V.

The test wrote unrotated k, v into the cache. As a result the kernel attended over D·H·k / d and D·H·v / d. Those entries have std ≈ 1/√128, so output_q ≈ 0, which is uncorrelated with output_ref.

The check |out_q − ref| ≤ 0.5 + 0.5·|ref| then reduces to roughly |ref| ≤ 1:

  • kv_len 128 (ref std ≈ 0.15) almost always passes;
  • kv_len 64/32 (ref std ≈ 0.25–0.3) fails on 1–2 elements whenever a |ref| tail exceeds 1.

That explains the reported abs diff just above 1.0 and relative diff of exactly 1.0. The failure is deterministic per seed but differs across platforms because their RNG streams differ, hence the Hopper atol=1.1 and the Intel deselect.

Fix. Rotate K/V with single_rht (rounding through bf16, like reshape_and_cache_int4) before quantizing. Then inverse-rotate the dequantized reference back to the original domain, as test_reshape_and_cache_per_token_head and test_per_token_head_round_trip_accuracy already do. The original-domain values stay around N(0,1), so the reference attention is still meaningful.

Tolerance. The remaining error comes from bf16 rounding and accumulation order only. The worst case is 0.021 over 200 seeds on CUDA, 2.4× under 5e-2. CUDA's error is larger than XPU's because the Triton Hadamard path rotates in bf16. The reference divides by head_size, which equals largest_power_of_2_divisor(head_size) for the parametrized head_size=128, consistent with the rest of the file.

Interaction with #62113. The two PRs only conflict textually, on adjacent lines:

Whichever PR merges second should keep that assert and drop all test_triton_unified_attention_per_token_head_scale[int4-*] deselects.

AI assistance (Claude Code) was used to investigate, write and validate this change. I reviewed every changed line and the validation results.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill. (Mandatory for agents, optional for humans).

  • AI assistance was used during the creation of this PR.

  • Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.

  • Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.

  • Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.

  • Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify Bot added ci/build intel-gpu Related to Intel GPU bug Something isn't working labels Oct 21, 2026
@github-actions

Copy link
Copy Markdown

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment /ci run for upstream CI or /amd-ci run for AMD CI only whenever CI signals are needed.

Once the PR is approved or has the ready label, the PR author can also use the corresponding /ci run, /ci retry, and /ci cancel commands, or their /amd-ci variants. New commits do not start upstream CI automatically.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

Agent Guidelines

IMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban.

🚀

@mergify

mergify Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @dudilester.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Oct 27, 2026
@dudilester
dudilester force-pushed the fix-int4-unified-attention-test-rht branch from dcd2061 to 1ce3e15 Compare October 28, 2026 10:36
@dudilester
dudilester requested a review from a team October 28, 2026 10:36
@mergify mergify Bot removed the needs-rebase label Oct 28, 2026

@jikunshang jikunshang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. as long as CI can pass.

@jikunshang

Copy link
Copy Markdown
Member

/ci run

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

❌ This PR is 551 commits behind upstream main. The bot could not merge upstream main into this branch (the merge commit did not appear in time). No new CI build was started. Either merge or rebase onto the latest main yourself and rerun /ci run, or use /ci run --allow-stale to test this branch as is.

@dudilester

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

❌ This PR is 555 commits behind upstream main. The bot could not merge upstream main into this branch (API returned 422: expected head sha didn’t match current head ref.). No new CI build was started. Either merge or rebase onto the latest main yourself and rerun /ci run, or use /ci run --allow-stale to test this branch as is.

…ntion test

test_triton_unified_attention_per_token_head_scale filled the INT4 cache
with unrotated K/V. unified_attention_int4 assumes the cache holds
RHT-rotated data, as written by reshape_and_cache_int4: it rotates Q and
inverse-rotates the output. The kernel output was therefore uncorrelated
with the reference. The test passed only while every element stayed
within atol=rtol=0.5, and failed on 1-2 elements for some seeds and
platforms. This is why Hopper needed atol=1.1 and Intel CI deselected
the int4 seq_lens1 case.

Rotate K/V with single_rht before the asymmetric quantization, and
inverse-rotate the dequantized reference back to the original domain,
like the other INT4 tests in this file. Both runs now see the same KV,
so INT4 uses the same 5e-2 tolerance as INT8/FP8. Drop the Hopper
special case and the Intel CI deselect.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Dudi Lester <dudi.lester@intel.com>
@dudilester
dudilester force-pushed the fix-int4-unified-attention-test-rht branch from 7e6869b to 7f88221 Compare November 4, 2026 12:12
@dudilester

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #99919 for commit 7f88221f35b6.

@jikunshang jikunshang added the verified Run pre-commit for new contributors without triggering other tests label Nov 4, 2026
@jikunshang
jikunshang merged commit ebf2d13 into vllm-project:main Nov 4, 2026
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ci/build intel-gpu Related to Intel GPU verified Run pre-commit for new contributors without triggering other tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants