Skip to content

[BUG] RuntimeError: deque mutated during iteration in abort_seq_group#2371

Merged
Yard1 merged 1 commit intovllm-project:mainfrom
chenxu2048:deque_mutated_during_iteration
Jan 12, 2024
Merged

[BUG] RuntimeError: deque mutated during iteration in abort_seq_group#2371
Yard1 merged 1 commit intovllm-project:mainfrom
chenxu2048:deque_mutated_during_iteration

Conversation

@chenxu2048
Copy link
Copy Markdown
Contributor

Hi, we found that #2290 introduced a bug in function abort_seq_group. Attempting to mutate a deque during iteration will raise an exception.

Here is the sample code to reproduce the bug:

from vllm import LLMEngine, EngineArgs, SamplingParams
model = LLMEngine.from_engine_args(EngineArgs(model="meta-llama/Llama-2-7b-hf"))
model.add_request("0", "", SamplingParams())
model.add_request("1", "", SamplingParams())
model.abort_request(["0", "1"])

@NadavShmayo
Copy link
Copy Markdown
Contributor

NadavShmayo commented Jan 8, 2024

Nice catch,
won't it be simpler to solve this by converting the deque to a list when iterating over it?
I believe the code could stay the same as before except for:

for seq_group in list(state_queue):

@chenxu2048
Copy link
Copy Markdown
Contributor Author

Nice catch, won't it be simpler to solve this by converting the deque to a list when iterating over it? I believe the code could stay the same as before except for:

for seq_group in list(state_queue):

Hi, @NadavShmayo. Thanks for your comments and nice clean solution.

But, here are two reasons why we chose an extra list:

  1. I think we should avoid removing elements while iterating over both List and Deque. The previous code with reversed() may work correctly, but it's dangerous.
  2. Using list(state_queue) to copy state_queue introduces overhead. In general, I think aborted_groups is shorter than the queue itself.

FYI

Copy link
Copy Markdown
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

@Yard1 Yard1 merged commit 48cf1e4 into vllm-project:main Jan 12, 2024
xjpang pushed a commit to xjpang/vllm that referenced this pull request Jan 14, 2024
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Jan 18, 2024
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Feb 13, 2024
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?

1. MTP supports V1 scheduler 
2. Refactor attn metadata build
### Does this PR introduce _any_ user-facing change?


### How was this patch tested?


- [x] v0.9.1-dev 
- [x] A3 [TP16] [DP4 TP4] 
- [x] A3 1P1D

Signed-off-by: xuyexiong <xuyexiong@huawei.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