Skip to content

Conversation

@LucasWilkinson
Copy link
Collaborator

@LucasWilkinson LucasWilkinson commented Nov 14, 2025

Fix AssertionError: DCP not support reorder_batch_threshold > 1 now. caused by #27363

Simply removing the assert; this assert has resulted in more false positives than true positives causing unnecessary thrash. The GPU model runner should not be responsible for tracking support of attention backends and ensuring they are advertising correct reorder batch thresholds; this would be better enforced via something like: #28750

Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
@mergify mergify bot added the v1 label Nov 14, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses an AssertionError related to reorder_batch_threshold in Decode Context Parallelism (DCP) mode by removing the problematic assertion from GPUModelRunner. Your reasoning that this check is brittle due to its reliance on a global environment variable (VLLM_ATTENTION_BACKEND) is correct. The responsibility for advertising backend capabilities should indeed lie with the backends themselves, and this change aligns with that principle. The logic within each AttentionMetadataBuilder appears to be the more robust place for such enforcement. This removal simplifies the code, improves modularity, and resolves the issue of false positive assertion failures. The change is approved.

@LucasWilkinson LucasWilkinson added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 15, 2025
@LucasWilkinson LucasWilkinson enabled auto-merge (squash) November 15, 2025 21:22
@LucasWilkinson LucasWilkinson merged commit be263f7 into vllm-project:main Nov 15, 2025
48 checks passed
ym820 pushed a commit to ym820/vllm that referenced this pull request Nov 16, 2025
… > 1 now.` (vllm-project#28751)

Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
Signed-off-by: ym820 <yikai.mao@outlook.com>
geodavic pushed a commit to geodavic/vllm that referenced this pull request Nov 16, 2025
… > 1 now.` (vllm-project#28751)

Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
Signed-off-by: George D. Torres <gdavtor@gmail.com>
bwasti pushed a commit to bwasti/vllm that referenced this pull request Nov 17, 2025
… > 1 now.` (vllm-project#28751)

Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
Signed-off-by: Bram Wasti <bwasti@meta.com>
bringlein pushed a commit to bringlein/vllm that referenced this pull request Nov 26, 2025
… > 1 now.` (vllm-project#28751)

Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
… > 1 now.` (vllm-project#28751)

Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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