Skip to content

Conversation

@LucasWilkinson
Copy link
Collaborator

@LucasWilkinson LucasWilkinson commented Oct 29, 2025

Fix incorrect definition of extends in #27367, more extensive unit tests, fix reordering bug when multiple swaps are involved + light refactor.

Credit to @ganyi1996ppo for finding the issue

Signed-off-by: Lucas Wilkinson <[email protected]>
Signed-off-by: Lucas Wilkinson <[email protected]>
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 introduces important fixes to the batch reordering logic, correcting the definitions of 'extend' and 'prefill' requests and fixing how permutations are applied. The changes improve correctness. However, I've identified a more fundamental issue in the partitioning algorithm that could still lead to incorrect ordering in some scenarios. Additionally, a new test case appears to have an expected output that doesn't align with the implementation's logic, which could mask bugs. My review includes critical feedback on the partitioning algorithm and a high-severity comment on the test case discrepancy.

@LucasWilkinson LucasWilkinson added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 29, 2025
@heheda12345 heheda12345 merged commit b5d7075 into vllm-project:main Oct 30, 2025
50 checks passed
MatthewBonanni pushed a commit to MatthewBonanni/vllm that referenced this pull request Oct 30, 2025
ilmarkov pushed a commit to neuralmagic/vllm that referenced this pull request Nov 7, 2025
ZhengHongming888 pushed a commit to ZhengHongming888/vllm that referenced this pull request Nov 8, 2025
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
eldarkurtic pushed a commit to eldarkurtic/vllm that referenced this pull request Nov 12, 2025
Signed-off-by: Lucas Wilkinson <[email protected]>
Signed-off-by: Eldar Kurtic <[email protected]>
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