Skip to content

[FIX] Fix shape mismatch for swapped sequences when logprobs > 0#1971

Closed
derange-alembic wants to merge 2 commits intovllm-project:mainfrom
derange-alembic:fix_swap_seq_logprobs_shape_mismatch
Closed

[FIX] Fix shape mismatch for swapped sequences when logprobs > 0#1971
derange-alembic wants to merge 2 commits intovllm-project:mainfrom
derange-alembic:fix_swap_seq_logprobs_shape_mismatch

Conversation

@derange-alembic
Copy link
Copy Markdown

When the logprobs of a sequence is set to be larger than 0 and it was swapped out using recomputation policy, the _get_logprobs() in sampler.py will encounter IndexError: shape mismatch: indexing tensors could not be broadcast together with shapes due to the obliviousness of the output_token_ids.

@derange-alembic
Copy link
Copy Markdown
Author

This issue is also reported in #1847.

@derange-alembic derange-alembic marked this pull request as ready for review December 7, 2023 23:31
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.

This looks good to me. cc @WoosukKwon @zhuohan123

@WoosukKwon WoosukKwon self-requested a review December 10, 2023 03:25
@WoosukKwon
Copy link
Copy Markdown
Collaborator

WoosukKwon commented Dec 15, 2023

BTW, I found another bug when running a batch with n = 2.

  File "/home/wskwon/workspace/vllm/vllm/core/scheduler.py", line 284, in schedule
    scheduler_outputs = self._schedule()
  File "/home/wskwon/workspace/vllm/vllm/core/scheduler.py", line 142, in _schedule
    assert seq_group.num_seqs() == 1, (
AssertionError: Waiting sequence group should have only one prompt sequence.

I think this happens when one of the two sequences finish earlier and the other one is preempted and resumed using recomputation.

This was fixed by #2186

Comment on lines +592 to +596
# Swapped seqs have output tokens.
output_tokens = sampling_metadata.seq_data[
seq_ids[0]].output_token_ids
group_prompt_logprobs: PromptLogprobs = [None]
for token_id in prompt_tokens[1:]:
for token_id in prompt_tokens[1:] + output_tokens:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is output_tokens used for prompt logprobs?

@richardzhuang0412
Copy link
Copy Markdown

For prompt_logprobs I think the issue still remains

@AetherPrior
Copy link
Copy Markdown

AetherPrior commented Feb 26, 2024

Hi, I've raised a new issue (#3032), as the codebase has changed since then, and the issue still persists.
CC: @derange-alembic , @Yard1 , @WoosukKwon

@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you!

@github-actions github-actions bot added the stale Over 90 days of inactivity label Oct 30, 2024
@mergify
Copy link
Copy Markdown

mergify bot commented Oct 30, 2024

This pull request has merge conflicts that must be resolved before it can be
merged. @derange-alembic please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 30, 2024
@github-actions github-actions bot added unstale Recieved activity after being labelled stale and removed stale Over 90 days of inactivity labels Nov 3, 2024
@hmellor hmellor added stale Over 90 days of inactivity and removed unstale Recieved activity after being labelled stale labels Jan 28, 2025
@hmellor hmellor closed this Jan 28, 2025
jinyouzhi pushed a commit to jinyouzhi/vllm that referenced this pull request Sep 26, 2025
…1971)

Signed-off-by: Youlei Yang <youlei.yang@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase stale Over 90 days of inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants