Skip to content

[KV Offload] Fix num_tokens_after_batch for different termination types - #49285

Merged
orozery merged 15 commits into
vllm-project:mainfrom
Alex-ai-future:fix/lazy-clip
Jul 26, 2026
Merged

orozery merged 15 commits into
vllm-project:mainfrom
Alex-ai-future:fix/lazy-clip

Conversation

@Alex-ai-future

@Alex-ai-future Alex-ai-future commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix incorrect num_tokens_after_batch computation in offloading scheduler by using num_computed_tokens for aborted requests (where partial KV data may exist) instead of always using num_tokens.

Problem

The original code used num_tokens for all finished requests:

if req.is_finished():
    num_tokens_after_batch = req.num_tokens

For aborted requests, num_computed_tokens < num_tokens (partial KV data). Using num_tokens would attempt to store chunks without actual KV data on GPU.

Solution

Distinguish aborted requests from other finished requests:

if req.status is RequestStatus.FINISHED_ABORTED:
    num_tokens_after_batch = req.num_computed_tokens
elif req.is_finished():
    num_tokens_after_batch = req.num_tokens

Rationale:

  • Aborted requests: Use num_computed_tokens (actual KV data), never store chunks without KV data
  • Other finished requests (EOS, length cap, repetition, error): Use num_tokens to handle async scheduling delay correctly

The clamp in storable_chunks (min(num_chunks, num_allocated_chunks)) remains as defense-in-depth, handling all block_ids edge cases (empty, partial capacity, etc.).

Test Plan

All existing tests pass (112 passed):

  • test_abort_queued_request_does_not_build_store_job ✅ (queued abort)
  • test_request_preemption ✅ (preempt handling)
  • test_last_block_offloaded_at_request_finish ✅ (normal EOS)
  • test_two_groups_full_and_sliding_window ✅ (multi-group)
  • All other offloading scheduler tests ✅

Impact

  • Semantic correctness: Aborted requests only store chunks with actual KV data
  • Simplicity: Minimal code change, handles the root cause
  • Safety: Clamp provides defense-in-depth for block_ids edge cases

AI Assistance

Used Claude for code analysis, root cause identification, and test validation. All changes reviewed and approved by the human author.

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

Alex-ai-future and others added 3 commits July 21, 2026 14:07
…equest_finished

Root-cause fix for issue vllm-project#49118, complementary to PR vllm-project#49146's clamp.

Changes:
1. preempt path: sync clear offload_keys with block_ids
   - Maintains invariant: len(offload_keys) * blocks_per_chunk <= len(block_ids)
   - Prevents offload_keys residue after preempt

2. request_finished: check block_ids before calling update_offload_keys
   - Only populate offload_keys if request was actually scheduled
   - Prevents offload_keys without corresponding block_ids for queued aborts

3. test: update test_abort_queued_request_does_not_build_store_job
   - Assert offload_keys is empty for never-scheduled request
   - Reflects the source fix in request_finished

Why this matters:
- preempt sync clear: defensive fix for invariant consistency
- request_finished check: source fix for queued abort (no longer need clamp)
- mid-prefill abort: still benefits from PR vllm-project#49146's clamp (prefix cache reuse)

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alex <jihuihuang@example.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
Signed-off-by: Alex <alex.tech.lab@outlook.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
Root-cause fix for the semantic issue: aborted requests may have
num_computed_tokens < num_tokens, but the code was using num_tokens
(entire prompt) for all finished requests, potentially storing chunks
without actual KV data (garbage data).

Fix: Use num_computed_tokens for FINISHED_ABORTED requests (only store
chunks with actual KV data), and keep num_tokens for other finished
states (EOS, etc.) to handle scheduling delay where num_computed_tokens
may not be updated yet.

This is a fundamental semantic fix that ensures:
- Aborted requests: only store chunks that were actually computed
- Normal EOS: still store all chunks (handle scheduling delay)

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
Signed-off-by: Alex <alex.tech.lab@outlook.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
With the fix to distinguish abort from EOS (using num_computed_tokens for
aborted requests), the min(num_chunks, num_allocated_chunks) clamp is no
longer needed:

- For abort: num_chunks = num_computed_tokens_chunks, which is always
  <= num_allocated_chunks (blocks are allocated for computed tokens)
- For EOS: num_chunks = num_tokens_chunks, which equals num_allocated_chunks
  (all blocks allocated when request finishes)

The clamp was a symptom fix for the root cause (not distinguishing abort
from EOS). With the root cause fixed, the clamp becomes unnecessary.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
Signed-off-by: Alex <alex.tech.lab@outlook.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
@Alex-ai-future

Copy link
Copy Markdown
Contributor Author

@orozery PTAL

Alex-ai-future and others added 2 commits July 21, 2026 14:31
Remove long comment from preempt path to satisfy 88-char line limit.

Signed-off-by: Alex <jihui.huang@daocloud.io>
Preempted requests clear block_ids and offload_keys together in the
next branch, so calling update_offload_keys() first just produces
stale hashes that get immediately discarded. Move the call into the
else branch so only non-preempted requests pay the cost.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alex <jihuihuang@users.noreply.github.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
Comment on lines +982 to +989
if req.status == RequestStatus.FINISHED_ABORTED:
# For aborted requests, only store chunks with actual KV data
# (num_computed_tokens), not the entire prompt (num_tokens).
num_tokens_after_batch = req.num_computed_tokens
elif req.is_finished():
# For other finished requests (EOS, etc.), use num_tokens to
# handle the case where num_computed_tokens hasn't been updated
# yet due to scheduling delay.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is the only change that is relevant.
All other changes can be reverted.

Comment thread vllm/distributed/kv_transfer/kv_connector/v1/offloading/scheduler.py Outdated
Comment thread vllm/distributed/kv_transfer/kv_connector/v1/offloading/scheduler.py Outdated
Comment thread vllm/distributed/kv_transfer/kv_connector/v1/offloading/scheduler.py Outdated
@Alex-ai-future Alex-ai-future changed the title [KV Offload] Fix semantic inconsistency between offload_keys and block_ids [KV Offload] Fix num_tokens_after_batch for different termination types Jul 22, 2026
@Alex-ai-future

Copy link
Copy Markdown
Contributor Author

@orozery
Thank you for your advice. I finally have a clear understanding and have modified it according to your requirements.

Distinguish normal termination (STOPPED/LENGTH_CAPPED/REPETITION) from
abnormal termination (ABORTED/ERROR) when computing num_tokens_after_batch:

- Normal termination: use num_tokens to handle async scheduling delay
- Abnormal termination: use num_computed_tokens (actual KV data)

Keep clamp in storable_chunks as defense-in-depth for block_ids edge cases.

Signed-off-by: Alex <jihui.huang@daocloud.io>
Co-authored-by: Claude <noreply@anthropic.com>
Comment on lines +933 to +934
else:
req_status.update_offload_keys()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Leftover unrelated change? Let's revert

Comment on lines +984 to +995
if req.status in (
RequestStatus.FINISHED_STOPPED,
RequestStatus.FINISHED_LENGTH_CAPPED,
RequestStatus.FINISHED_REPETITION,
):
# Normal termination: use num_tokens to handle scheduling
# delay with async scheduling.
num_tokens_after_batch = req.num_tokens
elif req.is_finished():
# Abnormal termination (abort/error): store all computed KV data.
# num_computed_tokens represents actual computed tokens.
num_tokens_after_batch = req.num_computed_tokens

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is more long and fragile than simply:

Suggested change
if req.status in (
RequestStatus.FINISHED_STOPPED,
RequestStatus.FINISHED_LENGTH_CAPPED,
RequestStatus.FINISHED_REPETITION,
):
# Normal termination: use num_tokens to handle scheduling
# delay with async scheduling.
num_tokens_after_batch = req.num_tokens
elif req.is_finished():
# Abnormal termination (abort/error): store all computed KV data.
# num_computed_tokens represents actual computed tokens.
num_tokens_after_batch = req.num_computed_tokens
if req.status is RequestStatus.FINISHED_ABORTED:
num_tokens_after_batch = req.num_computed_tokens
elif req.is_finished():
num_tokens_after_batch = req.num_tokens

Distinguish aborted requests from other finished requests when computing
num_tokens_after_batch:

- Aborted: use num_computed_tokens (actual KV data)
- Other finished (EOS, length cap, repetition, error): use num_tokens
  to handle async scheduling delay

Signed-off-by: Alex <jihui.huang@daocloud.io>
Co-authored-by: Claude <noreply@anthropic.com>
@orozery orozery added the ready ONLY add when PR is ready to merge/full CI is needed label Jul 22, 2026
@orozery
orozery merged commit 3f1d409 into vllm-project:main Jul 26, 2026
92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kv-connector ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants