Repository navigation
[Misc][Platform] Check aligned KV block sizes against every attention backend - #58457
Conversation
jayzuccarelli
left a comment
There was a problem hiding this comment.
I worked on this path in #48313, so I read it against the phase 1 code from #49845. Two things:
-
_check_aligned_block_sizevalidates with each backend'ssupports_block_size, which only asks whether the size is a multiple of one of that backend's kernel sizes. The failure it pre-empts comes fromselect_common_block_size, which needs one kernel size every backend accepts that also divides the manager block. Those can disagree: with exact sizes[128]and[64], 128 passes the new check andselect_common_block_size(128, ...)still raisesNo common block size for 128. Validating withselect_common_block_sizeitself (re-raising with the backend list) would make the early and late errors agree. Phase 1's_preferred_block_size_for_backendshas the same gap, so the helper could be shared. -
In
_align_heterogeneous_kv_block_size,smallest_kernel_blocknow feeds bothblock_alignmentandrequired_page. The cross-backend LCM makes sense for the rounding step, butrequired_pageis the padded spec's page at its own smallest kernel block, which comes from the primary backend. WithFLASHINFER_MLA_SPARSE([32, 64]) plus the CUDA indexer ([64]) the LCM is 64 instead of 32, sorequired_pagedoubles and the block size goes 64 to 128 at a 2x padded/primary ratio, and 128 to 256 at ~3.6x (nvfp4 primary, bf16 skip layers). Nothing fails, sinceselect_common_block_sizestill picks 64, but it changes behavior for DSA models with skip layers. Keepingrequired_pageonbackend_cls's own smallest block and using the granularity only forblock_alignmentwould avoid that.
1e7fe72 to
3b737ef
Compare
|
@jayzuccarelli Thanks, both are right and both are now addressed in On (2): you are correct that this was a behavior change I introduced. The padded spec is packed against the primary backend alone, as the function's own docstring says, so widening its page to the cross-backend LCM inflated it exactly as you describe. On (1): also correct, and the mismatch is sharper than I realized. On sharing it with phase 1: I agree the same gap is there, but I left For transparency: I cannot run vLLM's test suite in my environment, so CI is the check on the new tests. Lint is clean at the pinned ruff version. |
|
Thanks for turning these around so fast. One correction to my own point (1), which I should have caught: at runtime Same reason on phase 1: I'd hold off on switching it. Your |
yewentao256
left a comment
There was a problem hiding this comment.
Thanks for the work!
Please do the following:
- shrink the diff, idealy <100 LOC
Also take a look at these AI generated comments:
I think `_check_aligned_block_size()` is validating at the wrong abstraction level.
The new code calls `select_common_block_size(block_size, backend_classes)` across all non-SSM backends globally, but at runtime kernel block sizes are selected per KV cache group. Different groups can therefore legitimately use different physical kernel block sizes.
For example, if one group uses a backend that supports only 32 and another uses one that supports only 48, a manager block size of 96 is valid: the two groups can use kernel block sizes 32 and 48 respectively. A global `select_common_block_size(96, [backend32, backend48])` would reject this because there is no single kernel block size common to both backends.
Conversely, `select_common_block_size()` only looks at `get_supported_kernel_block_sizes()` and bypasses backend-specific `supports_block_size()` overrides. So a backend that explicitly requires the manager block itself to be exactly 16 could incorrectly accept a manager block of 64 simply because 16 divides 64.
It seems safer for this early validation step to keep checking `supports_block_size(block_size)` independently for each backend, and leave common physical kernel-size selection to `prepare_kernel_block_sizes()`, where the actual KV cache grouping is known.
The new exact-backend tests also seem inconsistent with the current implementation.
`_mock_backend(..., exact=True)` enforces exactness through `supports_block_size()`, but `_check_aligned_block_size()` no longer calls that method. For example, an exact `[16]` backend with manager block size 64 would still pass `select_common_block_size()` because 16 divides 64, so the expected `ValueError` should not be raised with the current code.
I think these tests are actually highlighting why the early check should continue to respect `supports_block_size()`.
3b737ef to
a6ab16d
Compare
|
@yewentao256 @jayzuccarelli Thanks, both of you. You're right, and my last push was wrong in two ways. Checking with I've rebuilt it smaller, now +72/-11:
|
|
|
a6ab16d to
e444ada
Compare
|
@jayzuccarelli Thanks. On (1), agreed: the check is now a shared |
| sizes = [ | ||
| min(s.base if isinstance(s, MultipleOf) else s for s in supported) | ||
| for b in backend_classes | ||
| if (supported := b.get_supported_kernel_block_sizes()) | ||
| ] | ||
| return math.lcm(*sizes) if sizes else 1 |
There was a problem hiding this comment.
Please take a look at this AI generated comment:
Is taking the minimum advertised size from each backend sufficient here? `get_supported_kernel_block_sizes()` represents alternative supported sizes, and `supports_block_size()` may also implement exact-size behavior.
For example, if backend A supports `[32, 48]` and backend B supports exactly `[48]`, Phase 1 can validly choose `48`. This helper would compute `lcm(32, 48) = 96`, potentially bumping an already-valid block size to `96`, which B then rejects.
Could we avoid collapsing each backend's supported set to its minimum, and instead choose an aligned candidate that is explicitly accepted by all backends?e444ada to
a0b8439
Compare
|
@yewentao256 Thanks, the AI comment is right, and it's a regression my LCM rounding introduced. With |
| pre_block_size = vllm_config.cache_config.block_size | ||
| cls._align_hybrid_block_size(vllm_config, backend_classes[0]) | ||
| cls._check_aligned_block_size(vllm_config, backend_classes, pre_block_size) |
There was a problem hiding this comment.
Shall we support xpu as well?
There was a problem hiding this comment.
Yes, done in d333dfd. XPUPlatform.update_block_size_for_backend calls super() first, so the check ran before its GDN step rounds the block size up to a multiple of 64. That rounding was unchecked, so it now runs the same check on its result. Added test_xpu_gdn_rounding_rejected_by_a_sibling_backend_raises, which skips without vllm_xpu_kernels, like the existing XPU tests. Also rebased onto current main. Still +99/-0.
Phase 1 (vllm-project#49845) picks a block size every backend supports. The hybrid and heterogeneous-KV alignment steps that follow still round by the first backend's kernel block only, so they can land on a size a sibling backend rejects, which today surfaces late in select_common_block_size(). Leave alignment unchanged and validate its result: if alignment changed the block size, every backend's supports_block_size() must accept it, otherwise raise immediately naming the rejecting backends. CpuPlatform overrides update_block_size_for_backend and aligns directly, so it calls the same check. XPUPlatform rounds the block size up to a multiple of 64 for GDN after super() has run the check, so it checks that rounding too. Rounding by every backend (the LCM of each backend's smallest kernel block) was tried and regresses valid configs: with [32, 48] and an exact [48] backend, phase 1 correctly picks 48 and that rounding bumps it to 96. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Divy <divy@coralbricks.ai>
a0b8439 to
d333dfd
Compare
yewentao256
left a comment
There was a problem hiding this comment.
LGTM, thanks for the work!
|
/ci run |
|
❌ This PR is 150 commits behind upstream |
|
✅ @divyvasal, CI is now available for this PR.
|
|
/ci run |
|
✅ Triggered Buildkite CI #92487 for commit |
Purpose
Follow-up to #49845. Phase 1 there picks a KV cache block size every attention
backend accepts. The hybrid and heterogeneous-KV alignment steps that run
afterwards still round by the first backend's kernel block only, as
@Sanjays2402 pointed out in review of #49845, so they can land on a size a
sibling backend rejects. Today that surfaces late, as the opaque
select_common_block_size()error at worker setup.This PR does not change how alignment rounds. It validates the result: if
alignment changed the block size, every backend's
supports_block_size()mustaccept it, otherwise it raises immediately and names the rejecting backends. I
don't know of a model configuration that hits this today, so this is hardening
rather than a fix for an observed failure.
Why not round by every backend
An earlier revision rounded by the LCM of each backend's smallest kernel block.
That regresses valid configurations: with a backend supporting
[32, 48]and anexact-size
[48]backend, phase 1 correctly picks 48, and the LCM roundingbumps it to 96, which the exact-size backend rejects. Choosing an aligned size
every backend accepts would need a candidate search inside the mamba and indexer
alignment logic, which is a larger change. Thanks @yewentao256 and
@jayzuccarelli for working through this.
The check is per backend rather than a common-size check across all backends.
Kernel sizes are chosen per KV cache group at runtime, so a global check can
reject configurations that work, and it would bypass backends' exact-size
overrides of
supports_block_size().Changes
update_block_size_for_backend()records the block size before alignment andcalls a new
_check_aligned_block_size()afterwards.CpuPlatform.update_block_size_for_backend()overrides the base method andaligns directly, so it calls the same check.
XPUPlatform.update_block_size_for_backend()rounds the block size up to amultiple of 64 for GDN after
super()has run the check, so it checks thatrounding too.
Test Plan
Added to
tests/v1/worker/test_gpu_model_runner.py:test_alignment_rejected_by_a_sibling_backend_raises, run for both the baseand CPU platforms: an alignment that lands on 32 beside an exact-size
[16]backend raises, naming that backend.
test_xpu_gdn_rounding_rejected_by_a_sibling_backend_raises: XPU's GDNrounding from 16 to 64 beside an exact-size
[16]backend raises. It skipswithout
vllm_xpu_kernels, like the existing XPU tests.Test Result
Ran the three tests locally in a CPU environment with the XPU kernel modules
stubbed: all pass, and the XPU test fails with the XPU check removed. Full
pre-commit is clean.
🤖 Generated with Claude Code