KEMBAR78
Fix get_max_num_running_seqs for waiting and swapped seq groups by zhuohan123 · Pull Request #1068 · vllm-project/vllm · GitHub
Skip to content

Conversation

@zhuohan123
Copy link
Member

This is the actual correct fix for the issues in #1034.

cc @Yard1

Copy link
Collaborator

@Yard1 Yard1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Collaborator

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

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

Left minor comments.

Comment on lines +253 to +254
# that are not finished yet.
return self.num_unfinished_seqs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The semantic of the function will change because this also counts swapped sequences. Is this expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intended. get_max_num_running_seqs is being called for a swapped sequence when the swapped sequence is about to be swapped back to the running queue. This means these swapped sequences will soon become running sequences. This is the same logic as waiting sequences.

Copy link
Collaborator

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

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

LGTM

@zhuohan123 zhuohan123 merged commit f029ef9 into main Sep 18, 2023
@zhuohan123 zhuohan123 deleted the fix_get_max_num_running_seqs branch October 16, 2023 21:02
amy-why-3459 pushed a commit to amy-why-3459/vllm that referenced this pull request Sep 15, 2025
### What this PR does / why we need it?
Adjust concurrency group for each npu workflow
- for pd and benchmarks share the static-08-01, so only one job can runs
on
- other job one PR/schedule should have only 1 job runs

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
CI passed

Signed-off-by: Yikun Jiang <yikunkero@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants