[Frontend] Bad words sampling parameter #5986

Alvant · 2024-06-29T12:33:11Z

PR Checklist (Click to Expand)

Thank you for your contribution to vLLM! Before submitting the pull request, please ensure the PR meets the following criteria. This helps vLLM maintain the code quality and improve the efficiency of the review process.

PR Title and Classification

Only specific types of PRs will be reviewed. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

[Bugfix] for bug fixes.
[CI/Build] for build or continuous integration improvements.
[Doc] for documentation fixes and improvements.
[Model] for adding a new model or improving an existing model. Model name should appear in the title.
[Frontend] For changes on the vLLM frontend (e.g., OpenAI API server, LLM class, etc.)
[Kernel] for changes affecting CUDA kernels or other compute kernels.
[Core] for changes in the core vLLM logic (e.g., LLMEngine, AsyncLLMEngine, Scheduler, etc.)
[Hardware][Vendor] for hardware-specific changes. Vendor name should appear in the prefix (e.g., [Hardware][AMD]).
[Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.

Code Quality

The PR need to meet the following code quality standards:

We adhere to Google Python style guide and Google C++ style guide.
Pass all linter checks. Please use format.sh to format your code.
The code need to be well-documented to ensure future contributors can easily understand the code.
Include sufficient tests to ensure the project to stay correct and robust. This includes both unit tests and integration tests.
Please add documentation to docs/source/ if the PR modifies the user-facing behaviors of vLLM. It helps vLLM user understand and utilize the new features or changes.

Notes for Large Changes

Please keep the changes as concise as possible. For major architectural changes (>500 LOC excluding kernel/data/config/test), we would expect a GitHub issue (RFC) discussing the technical design and justification. Otherwise, we will tag it with rfc-required and might not go through the PR.

What to Expect for the Reviews

The goal of the vLLM team is to be a transparent reviewing machine. We would like to make the review process transparent and efficient and make sure no contributor feel confused or frustrated. However, the vLLM team is small, so we need to prioritize some PRs over others. Here is what you can expect from the review process:

After the PR is submitted, the PR will be assigned to a reviewer. Every reviewer will pick up the PRs based on their expertise and availability.
After the PR is assigned, the reviewer will provide status update every 2-3 days. If the PR is not reviewed within 7 days, please feel free to ping the reviewer or the vLLM team.
After the review, the reviewer will put an action-required label on the PR if there are changes required. The contributor should address the comments and ping the reviewer to re-review the PR.
Please respond to all comments within a reasonable time frame. If a comment isn't clear or you disagree with a suggestion, feel free to ask for clarification or discuss the suggestion.

Thank You

Finally, thank you for taking the time to read these guidelines and for your interest in contributing to vLLM. Your contributions make vLLM a great tool for everyone!

Alvant · 2024-06-29T12:38:42Z

vllm/sampling_params.py

@@ -28,6 +28,77 @@ class SamplingType(IntEnum):
 to sample from."""


+class NoBadWordsLogitsProcessor:


I am not sure if this is the right file for the class. Still I thought it could be placed here near the LogitsProcessor type (the one just above). But if there is a better place for the processor class, I will be ready to move it)

Yes I think that if we include this it should go in a different file.

Alvant · 2024-06-29T12:43:03Z

vllm/sampling_params.py

+            if len(bad_word_ids) == 1:  # 1-token words already processed
+                continue
+
+            if len(bad_word_ids) > len(past_tokens_ids) + 1:


This differs from the original inequality here: https://github.com/huggingface/transformers/blob/main/src/transformers/generation/logits_process.py#L1131

if len(sequence_ids) > input_ids.shape[1]: # the sequence is longer than the context, ignore continue

I may be wrong, but it seemed we should allow for one token to be generated (so + 1).

njhill · 2024-07-01T17:21:39Z

I'm unsure whether or not it makes sense to support this. I know it's an option in transfomers but it was added very early on and the implementation seems limited/clunky to me. Wouldn't it make more sense for the bad words to be a list of strings rather than token sequences?

Alvant · 2024-07-02T13:14:09Z

I'm unsure whether or not it makes sense to support this. I know it's an option in transfomers but it was added very early on and the implementation seems limited/clunky to me. Wouldn't it make more sense for the bad words to be a list of strings rather than token sequences?

@njhill

Yes, I absolutely agree that this "list of lists of token ids" structure is not very friendly and easy to use 😅 Sure, list of strings will be more convenient. I believe I can change that. Just wanted to clarify some questions before actually making some changes.

If we make bad words as list of strings (and call it, for example, just bad_words), we will lose compatibility with transformers interface. If people come to vLLM after transformers, they might already now about bad_words_ids parameter and how to use it. And they will look for this parameter in SamplingParams attributes.

So, the main and only question is actually the following — should we keep transformers' "clumsy" bad_words_ids option together with more friendly bad_words? should we support list of lists of ids structure or only list of strings?

Oh, and one more point.

Currently, SamplingParams has stop_token_ids parameter (list of token ids). If we make bad_words as list of strings, would it not bring some "heterogeneity"? (one parameter is about token ids, another one is about strings) If bad_words_ids is about token ids, then it is quite consistent with stop_token_ids.

Hmm, just noticed, SamplingParams actually has another parameter — stop, which is like stop_token_ids, but consisting of strings...

Ok, I agree that introducing bad_words instead of bad_words_ids is overall a good idea) At the moment, I do not see a way to change the behavior easily (it seems unlikely that a logits_preprocessor will do the thing as we will not know token ids beforehand), but I will definitely look into this in the coming days) I am just afraid that, after changes, it will be a completely different PR 🙂

njhill · 2024-07-02T21:50:32Z

My view on this kind of thing is to collect some concrete requirements / use cases and base on that. I.e. avoid adding things with hypothetical benefit. Would be good to see some explicit examples of how/where this functionality is used, and that should then also inform what kind of thing makes the most sense w.r.t. the various options being discussed.

Alvant · 2024-07-04T14:01:22Z

I looked through some examples and use cases.

First, it seems that bad_words_ids (lists of token sequences) is already a requested feature. Apart from the issue which this PR is linked to, I also found this already closed issue in vLLM (where the proposed solution works only for single tokens). In TensorRT-LLM, they already have this feature, also as lists of token ids. In OpenAI request parameters, there is one called "logit_bias" which is like a generalization of bad_words_ids, and also works on a token level.

However, seems like it would be indeed better to have a list of words and not token sequences. Because people always have to use tokenizers in order to get token ids. This is just stated in the documentation on the transformers docs site. Usually, these tokenizer things lead to questions (not very friendly "low level" functionality).

I also found a few mentions of bad words list (not token ids) functionality in other repositories. For example, in TensorRT-LLM Backend, there is already an option to provide a list of bad words as strings. In outlines, there are plans to add this feature.

So, I updated the PR: now there is bad_words parameter, not bad_words_ids.

However, to some extent, this complicated the implementation. So that I am currently not 100% sure that it is just a "Frontend" related feature 😅

@njhill could you please take a look and share your thoughts on the updated code?

Points which are to be resolved (if the general idea would seem OK, I will fix this):

NoBadWordsLogitsProcessor is still in sampling_params.py file
AsyncLLMEngine would need corresponding updates

P.S.

For the history, this is the PR which added bad_words_ids to the transformers library: huggingface/transformers#3367.

Alvant · 2024-07-04T14:19:30Z

Forgot to add, in vLLM, there is already something like bad words ids thing: logit_bias_logits_processor, which is added for the compatibility with OpenAI request params.

Alvant · 2024-07-11T07:22:14Z

vllm/engine/async_llm_engine.py

@@ -293,6 +293,44 @@ async def process_model_inputs_async(

        return self.input_processor(llm_inputs)

+    async def process_model_params_async(


Currently, async method is not tested. Checked only sync version and made async one based on it.

Alvant · 2024-07-11T07:23:28Z

vllm/logits_process.py

+
+    def __call__(
+        self,
+        past_tokens_ids: Union[List[int], Tuple[int]],


Merged main recently. And, surprisingly, past_token_ids appeared as Tuple (previously it was List).

Alvant · 2024-07-11T07:28:52Z

@njhill 🥺 👉👈

Since last time, also did:

Move all logits process stuff to separate file
Make the appropriate process-bad-words-param changes to the AsyncLLMEngine
(And update and merge main, resolve some conflicts & fix some things)

Alvant · 2024-07-17T22:16:46Z

@njhill just reminding to take a look at the PR when you have time.

P.S. If you don't mind, I plan to do such reminders once a week for some time.

P.P.S. There is a new code conflict now. I am planning to resolve it as part of future work on the PR after it has been reviewed and commented on.

njhill · 2024-07-24T16:37:44Z

Apologies @Alvant and thanks for the reminder, I miss the GH notifications sometimes because I'm subscribed to so many things. I'll take another look at this in the next day or two.

Alvant · 2024-08-03T14:38:24Z

@njhill, it seems we need one more reminder :)

Alvant · 2024-08-11T21:02:33Z

@njhill

from vllm import LLM

model = LLM("openai-community/gpt2")

request_outputs = model.generate(
    "Will the 'Bad words' pull request be reviewed anytime soon?.."
)
output = request_outputs[0].outputs[0].text

print(output)  # "I don't know, my friend, I don't know..."

P.S.

The growing number of conflicts over time inspires me with a kind of terror that covers my skin with goosebumps. However, the belief that the request will still be merged after all is still alive in me...

Alvant · 2024-08-12T19:55:25Z

@njhill

I had an idea in my head. You said that you can sometimes miss GH notifications because you get so many of them from lots of sources. So, maybe if I ping you a little more often, the likelihood of you noticing will be higher. To check this out, now I'm going to ping you not once a week, but.... once a day! I'm announcing a Seven-Day Ping Marathon. And it starts... today!

Seven-Day Ping Marathon, Day 1

@njhill, ping

Alvant · 2024-08-13T20:16:35Z

Seven-Day Ping Marathon, Day 2

1 + 1 = 2
"Hello" + " world!" = "Hello world!"
Red + Blue = Violet

#5986 PR + @njhill = Reviewed #5986 PR = @Alvant happy

Alvant · 2024-08-14T20:29:20Z

Seven-Day Ping Marathon, Day 3

@njhill


  _                             
 |_)  _  ._ _  o ._   _|  _  ._ 
 | \ (/_ | | | | | | (_| (/_ |

Alvant · 2024-08-15T19:02:46Z

@njhill

Levenshtein says that there are only four steps from "reminder" to "review"...

Seven-Day Ping Marathon, Day 4

Alvant · 2024-08-16T20:40:50Z

@njhill

PR's alone, with no review,
Has also conflicts quite a few.
But I do like it anyway
And hope it'll be merged someday...

Seven-Day Ping Marathon, Day 5

cadedaniel · 2024-08-16T21:10:09Z

Hi @Alvant . Please remember that vLLM committers are not paid and it is unfair to treat them with these kind of reminders. Regardless of whether this PR is mergable or not, this behavior will not get it merged. Thanks for your understanding.

Alvant · 2024-08-16T22:06:12Z

Hello, @cadedaniel. Of course I understand. I'm sorry. All I needed now was at least a sign that the request was remembered. When you don't get any response, not a word, for a few weeks, it's starting to be a bit worrying. I hope you understand too) And your reply gives me new hope that the request will get some progress in the future (I can wait, I just want to be sure that I don't wait in vain). Thanks!

Alvant · 2024-09-27T19:51:41Z

Better an inglorious end than a miserable existence

hwang136 · 2024-10-07T08:14:04Z

I desperately need this feature. @Alvant could you open it again? you are saving the world. I appreciate your diligence and dedication.

Alvant · 2024-10-07T22:27:42Z

@hwang136 Wow, your words are just melting my heart) Thank you! Glad to hear that someone really needs the feature)

Sure, I can open the PR again. After all, it's just a matter of a click of a button)

I think I'm also going to update the code in this branch (merge all latest vLLM updates). So as to make it somewhat easier to use for everyone who needs it and might want to try.

However, I cannot guarantee that this code will ever be merged into the main vLLM repo (in the near or however distant future)

…llm-project#8914) Signed-off-by: Alvant <[email protected]>

…-project#9614) Signed-off-by: Alvant <[email protected]>

…#9393) Signed-off-by: Alex-Brooks <[email protected]> Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Alvant <[email protected]>

Co-authored-by: DarkLight1337 <[email protected]> Signed-off-by: Alvant <[email protected]>

Signed-off-by: Alvant <[email protected]>

…ti-image (vllm-project#9626) Signed-off-by: mgoin <[email protected]> Signed-off-by: Alvant <[email protected]>

…t#9628) Signed-off-by: mgoin <[email protected]> Signed-off-by: Alvant <[email protected]>

Signed-off-by: Vinay Damodaran <[email protected]> Signed-off-by: Alvant <[email protected]>

Co-authored-by: Cyrus Leung <[email protected]> Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Alvant <[email protected]>

Signed-off-by: Alvant <[email protected]>

Signed-off-by: Woosuk Kwon <[email protected]> Signed-off-by: Alvant <[email protected]>

Co-authored-by: Zhuohan Li <[email protected]>

Alvant · 2024-10-26T07:22:57Z

Wow, seems I fucked up this branch trying to fix this goddamit DCO with all this rebase-all-your-commits-and-fix-a-million-conflicts-afterwards routine. Hmm, guess I'll close this PR again 😅 And create a new one from scratch... But without any DCO this time, no 😅

Alvant · 2024-10-26T07:58:01Z

DCO signed (probably), but all is messed up (for sure). Closing this PR in favour of a new one: #9717. Which contains all the commits. (But not DCO signatures. No, thanks.)

Alvant · 2024-10-26T08:36:25Z

@hwang136, hello!

I updated the code. Now the branch seems compatible with the latest vLLM. Hope I didn't break anything in the process) I only checked manually the "bad words" functionality itself (by running the corresponding tests).

However... while trying to fix all the conflicts (not only the code-related conflicts but also the ones preventing this PR from being merged), I accidentally messed up the PR 😅 So I closed it again (now once and for all 🫡). Here is a new one: #9717. It contains all the code. Only misses the comments and discussion. Which will continue to be available here, for history)

Alvant · 2024-10-26T09:11:51Z

Well, I guess, until some further notice, I am done here)

Alvant · 2024-10-26T09:13:34Z

Just in case, sorry for notifications anybody who appeared linked to this PR due to my failure to fix DCO)

Alvant · 2024-10-26T19:48:46Z

@hwang136

I just wanted to say that the code is merged. So, "bad words" should now be available as a sampling parameter (at least if you build vLLM from source — after a while it should be in pip install vllm as well).

Alvant commented Jun 29, 2024

View reviewed changes

Alvant mentioned this pull request Jun 29, 2024

It seems that SamplingParams doesnt support the bad_words_ids parameter when generating #986

Closed

Alvant commented Jul 11, 2024

View reviewed changes

Alvant changed the title ~~[Frontend] Add bad_words_ids sampling parameter~~ [Frontend] Bad words sampling parameter Jul 15, 2024

DarkLight1337 mentioned this pull request Jul 24, 2024

[Frontend] New allowed_token_ids decoding request parameter #6753

Merged

Alvant closed this Sep 27, 2024

Alvant reopened this Oct 7, 2024

Alvant requested a review from WoosukKwon as a code owner October 24, 2024 08:23

mgoin and others added 13 commits October 26, 2024 10:13

[Misc] Separate total and output tokens in benchmark_throughput.py (v…

c2a264e

…llm-project#8914) Signed-off-by: Alvant <[email protected]>

[torch.compile] Adding torch compile annotations to some models (vllm…

358a773

…-project#9614) Signed-off-by: Alvant <[email protected]>

[Frontend] Enable Online Multi-image Support for MLlama (vllm-project…

4c8aa4d

…#9393) Signed-off-by: Alex-Brooks <[email protected]> Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Alvant <[email protected]>

[Model] Add Qwen2-Audio model support (vllm-project#9248)

c3293da

Co-authored-by: DarkLight1337 <[email protected]> Signed-off-by: Alvant <[email protected]>

[CI/Build] Add bot to close stale issues and PRs (vllm-project#9436)

7d9fb11

Signed-off-by: Alvant <[email protected]>

[Bugfix][Model] Fix Mllama SDPA illegal memory access for batched mul…

3775ce6

…ti-image (vllm-project#9626) Signed-off-by: mgoin <[email protected]> Signed-off-by: Alvant <[email protected]>

[Bugfix] Use "vision_model" prefix for MllamaVisionModel (vllm-projec…

cb6364d

…t#9628) Signed-off-by: mgoin <[email protected]> Signed-off-by: Alvant <[email protected]>

[Bugfix]: Make chat content text allow type content (vllm-project#9358)

1b6de73

Signed-off-by: Vinay Damodaran <[email protected]> Signed-off-by: Alvant <[email protected]>

[XPU] avoid triton import for xpu (vllm-project#9440)

7f1a962

Co-authored-by: Cyrus Leung <[email protected]> Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Alvant <[email protected]>

[Bugfix] Fix PP for ChatGLM and Molmo (vllm-project#9422)

a0ac193

Signed-off-by: Alvant <[email protected]>

[V1][Bugfix] Clean up requests when aborted (vllm-project#9629)

fa75d40

Signed-off-by: Woosuk Kwon <[email protected]> Signed-off-by: Alvant <[email protected]>

[core] simplify seq group code (vllm-project#9569)

50cd76a

Co-authored-by: Zhuohan Li <[email protected]>

fix code style

72d8a53

Alvant force-pushed the feature/bad_words_ids branch from 7c0c60c to 72d8a53 Compare October 26, 2024 07:18

Alvant requested review from mgoin, simon-mo, robertgshaw2-redhat, tlrmchlsmth, LiuXiaoxuanPKU, KuntaiDu, DarkLight1337 and ywang96 as code owners October 26, 2024 07:18

Alvant mentioned this pull request Oct 26, 2024

[Frontend] Bad words sampling parameter #9717

Merged

Alvant closed this Oct 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Frontend] Bad words sampling parameter #5986

[Frontend] Bad words sampling parameter #5986

Alvant commented Jun 29, 2024

Alvant Jun 29, 2024

njhill Jul 1, 2024

Alvant Jun 29, 2024

njhill commented Jul 1, 2024

Alvant commented Jul 2, 2024 •

edited

Loading

njhill commented Jul 2, 2024

Alvant commented Jul 4, 2024

Alvant commented Jul 4, 2024

Alvant Jul 11, 2024

Alvant Jul 11, 2024

Alvant commented Jul 11, 2024

Alvant commented Jul 17, 2024 •

edited

Loading

njhill commented Jul 24, 2024

Alvant commented Aug 3, 2024

Alvant commented Aug 11, 2024

Alvant commented Aug 12, 2024

Alvant commented Aug 13, 2024

Alvant commented Aug 14, 2024

Alvant commented Aug 15, 2024 •

edited

Loading

Alvant commented Aug 16, 2024

cadedaniel commented Aug 16, 2024

Alvant commented Aug 16, 2024

Alvant commented Sep 27, 2024

hwang136 commented Oct 7, 2024 •

edited

Loading

Alvant commented Oct 7, 2024

Alvant commented Oct 26, 2024 •

edited

Loading

Alvant commented Oct 26, 2024

Alvant commented Oct 26, 2024 •

edited

Loading

Alvant commented Oct 26, 2024

Alvant commented Oct 26, 2024

Alvant commented Oct 26, 2024 •

edited

Loading

		@@ -28,6 +28,77 @@ class SamplingType(IntEnum):
		to sample from."""


		class NoBadWordsLogitsProcessor:

		@@ -293,6 +293,44 @@ async def process_model_inputs_async(

		return self.input_processor(llm_inputs)

		async def process_model_params_async(

[Frontend] Bad words sampling parameter #5986

[Frontend] Bad words sampling parameter #5986

Conversation

Alvant commented Jun 29, 2024

PR Title and Classification

Code Quality

Notes for Large Changes

What to Expect for the Reviews

Thank You

Alvant Jun 29, 2024

Choose a reason for hiding this comment

njhill Jul 1, 2024

Choose a reason for hiding this comment

Alvant Jun 29, 2024

Choose a reason for hiding this comment

njhill commented Jul 1, 2024

Alvant commented Jul 2, 2024 • edited Loading

njhill commented Jul 2, 2024

Alvant commented Jul 4, 2024

Alvant commented Jul 4, 2024

Alvant Jul 11, 2024

Choose a reason for hiding this comment

Alvant Jul 11, 2024

Choose a reason for hiding this comment

Alvant commented Jul 11, 2024

Alvant commented Jul 17, 2024 • edited Loading

njhill commented Jul 24, 2024

Alvant commented Aug 3, 2024

Alvant commented Aug 11, 2024

Alvant commented Aug 12, 2024

Alvant commented Aug 13, 2024

Alvant commented Aug 14, 2024

Alvant commented Aug 15, 2024 • edited Loading

Alvant commented Aug 16, 2024

cadedaniel commented Aug 16, 2024

Alvant commented Aug 16, 2024

Alvant commented Sep 27, 2024

hwang136 commented Oct 7, 2024 • edited Loading

Alvant commented Oct 7, 2024

Alvant commented Oct 26, 2024 • edited Loading

Alvant commented Oct 26, 2024

Alvant commented Oct 26, 2024 • edited Loading

Alvant commented Oct 26, 2024

Alvant commented Oct 26, 2024

Alvant commented Oct 26, 2024 • edited Loading

Alvant commented Jul 2, 2024 •

edited

Loading

Alvant commented Jul 17, 2024 •

edited

Loading

Alvant commented Aug 15, 2024 •

edited

Loading

hwang136 commented Oct 7, 2024 •

edited

Loading

Alvant commented Oct 26, 2024 •

edited

Loading

Alvant commented Oct 26, 2024 •

edited

Loading

Alvant commented Oct 26, 2024 •

edited

Loading