-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[mypy] Enable following imports for entrypoints #7248
[mypy] Enable following imports for entrypoints #7248
Conversation
…ode in the process
@rkooo567 please review when you have time. |
@petersalas can you help take a look why the audio entrypoints test now fails to pass pydantic validation? |
@DarkLight1337 can you elaborate a bit? I pulled the branch down but the audio tests seem to pass. Is there a call stack with the failure I can look at? |
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.
very cool! have the first iteration
@@ -164,16 +168,15 @@ def _parse_chat_message_content_parts( | |||
for part in parts: | |||
part_type = part["type"] | |||
if part_type == "text": | |||
text = cast(ChatCompletionContentPartTextParam, part)["text"] | |||
text = _TextParser.validate_python(part)["text"] |
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.
can you explain to me why we are doing this here instead of cast?
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.
Imo this way is a bit more readable and also provides additional validation.
n=1 if n is None else n, | ||
best_of=best_of, | ||
presence_penalty=0.0 | ||
if presence_penalty is None else presence_penalty, |
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.
what is it exactly for? (also doesn't thits have duplicated logics with post_init? should we remove them?)
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 is so that I can pass None
as the arguments to SamplingParams
without having to go to this file and lookup the default values.
The error is thrown server-side while iterating through each part in a message: https://github.com/vllm-project/vllm/blob/main/vllm/entrypoints/chat_utils.py#L164 I'm using pydantic 2.7.1 and pydantic_core 2.18.2, if that helps. Odd that it's not happening in CI now. |
Maybe this is a bug in Pydantic. I updated to pydantic 2.8.2 and pydantic-core 2.20.1 and don't get this error anymore. Sorry for bothering you! |
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.
lgtm!
Co-authored-by: Woosuk Kwon <[email protected]> Co-authored-by: Fei <[email protected]>
Co-authored-by: Woosuk Kwon <[email protected]> Co-authored-by: Fei <[email protected]> Signed-off-by: Alvant <[email protected]>
This PR enables
follow-imports=silent
forvllm.entrypoints
. It also cleans up some of the type annotations related to tokenizer (Union[PreTrainedTokenizer, PreTrainedTokenizerFast]
is now replaced withAnyTokenizer
).Partially addresses #3680.