-
Notifications
You must be signed in to change notification settings - Fork 399
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
Add CFG to vllm serving #517
Conversation
outlines/serve/serve.py
Outdated
@@ -122,6 +127,8 @@ async def stream_results() -> AsyncGenerator[bytes, None]: | |||
# Sets default for the model (`facebook/opt-125m`) | |||
engine = AsyncLLMEngine.from_engine_args(engine_args) | |||
|
|||
_adapt_tokenizer(engine.engine.tokenizer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you calling this function here? The result is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tokenizer is changed inside the function anyways. I now assigned it to the tokenizer though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah makes sense. It's not needed however as vLLM handles tokenisation on its end during encoding/decoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed because in here
Line 345 in fde61a8
self.generation += self.tokenizer.decode([token_id])[0] |
Also I just realized that this change makes a breaking change to the library. If it's fine by the project directors its fine, if not we might need a change.
for example we can call
_adapt_tokenizer
inside __init__
functions of the logit processors
012a0e9
to
281c0af
Compare
Thank you for your contributions! I added some documentation before merging. |
Hi,
This pull request adds support for CFG in vllm serving.