Skip to content

Conversation

@njhill
Copy link
Member

@njhill njhill commented Oct 9, 2025

It looks like this must have been missed during refactoring done in #23569 before it was merged.

@benchislett

It looks like this must have been missed during refactoring of vllm-project#23569 before it was merged.

Signed-off-by: Nick Hill <[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 aims to remove the unused prev_sampled_token_ids_invalid_indices field, which is a good cleanup. However, the change as presented in the diff appears to introduce a critical bug by failing to filter out invalid requests from prev_req_id_to_index. This could lead to incorrect behavior in asynchronous scheduling. I've provided a comment with a code suggestion to fix this by applying the necessary filtering. With that change, this PR should be good to merge.

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 9, 2025
Copy link
Collaborator

@benchislett benchislett left a comment

Choose a reason for hiding this comment

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

thanks!

@njhill njhill enabled auto-merge (squash) October 9, 2025 18:35
@njhill njhill merged commit 2e54db4 into vllm-project:main Oct 9, 2025
51 checks passed
@njhill njhill deleted the rm-unused-field branch October 9, 2025 20:58
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Dhruvilbhatt pushed a commit to Dhruvilbhatt/vllm that referenced this pull request Oct 14, 2025
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
0xrushi pushed a commit to 0xrushi/vllm that referenced this pull request Oct 26, 2025
0xrushi pushed a commit to 0xrushi/vllm that referenced this pull request Oct 26, 2025
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