Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BugFix] Fix handling of stop strings and stop token ids #3672

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

njhill
Copy link
Member

@njhill njhill commented Mar 28, 2024

This addresses the following bugs:

  • Stop strings ends having to align with token boundaries
  • Stop string not being excluded properly from output when it spans multiple tokens and include_stop_str_in_output==False (primarily a problem when streaming output)
  • Incorrect output truncation when stopping due to a token in stop_token_ids that is a special token when skip_special_tokens==True

Fixes #3574
Fixes #3572
Fixes #2577
Fixes #3026

njhill added 2 commits April 10, 2024 12:36
This addresses the following bugs:
- Stop strings ends having to align with token boundaries
- Stop string not being excluded properly from output when it spans multiple tokens and include_stop_str_in_output==True
- Incorrect output truncation when stopping due to a token in stop_token_ids that is a special token when skip_special_tokens==True
@njhill njhill force-pushed the fix-stop-strings branch from caea3d4 to 7e5fa65 Compare April 10, 2024 11:39
@njhill njhill marked this pull request as ready for review April 10, 2024 12:27
Copy link

@dgoupil dgoupil 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 a lot for the tests

@njhill
Copy link
Member Author

njhill commented Apr 10, 2024

Thank you @dgoupil!

Copy link
Collaborator

@sroy745 sroy745 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 for the fix.

# Check if the sequence has generated the EOS token.
if ((not sampling_params.ignore_eos)
and seq.get_last_token_id() == seq.eos_token_id):
seq.status = SequenceStatus.FINISHED_STOPPED
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to set seq.stop_reason to eos_token_id here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sroy745 it's a good question. It was decided to keep the stop_reason as None in this case so that clients can know that it's due to EOS in simple cases without having to know the token ids. See discussion in #2976.

Copy link
Member

@ywang96 ywang96 left a comment

Choose a reason for hiding this comment

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

Left a few comments/questions - Thanks for the fix!

vllm/sequence.py Outdated Show resolved Hide resolved
vllm/engine/llm_engine.py Show resolved Hide resolved
vllm/engine/llm_engine.py Show resolved Hide resolved
@njhill
Copy link
Member Author

njhill commented Apr 11, 2024

Thanks alot for the review @ywang96! get_output_text_to_return() method name now updated

@njhill njhill merged commit e46a60a into vllm-project:main Apr 11, 2024
35 checks passed
@njhill njhill deleted the fix-stop-strings branch April 11, 2024 22:34
andy-neuma pushed a commit to neuralmagic/nm-vllm that referenced this pull request Apr 12, 2024
z103cb pushed a commit to z103cb/opendatahub_vllm that referenced this pull request Apr 22, 2024
Temirulan pushed a commit to Temirulan/vllm-whisper that referenced this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants