-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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] Pass type checking in vllm/inputs #11680
Conversation
Signed-off-by: Tobias Pitters <[email protected]>
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can do one of these:
🚀 |
Signed-off-by: Tobias Pitters <[email protected]>
Signed-off-by: Tobias Pitters <[email protected]>
Signed-off-by: Tobias Pitters <[email protected]>
Signed-off-by: Tobias Pitters <[email protected]>
Signed-off-by: Tobias Pitters <[email protected]>
Signed-off-by: Tobias Pitters <[email protected]>
vllm/inputs/data.py
Outdated
@@ -176,6 +177,26 @@ class TokenInputs(TypedDict): | |||
""" | |||
|
|||
|
|||
def is_token_inputs( | |||
inputs: Union[TokenInputs, | |||
"MultiModalInputsV2"]) -> TypeGuard[TokenInputs]: |
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 we use TypeIs
here? Maybe it can avoid having to type ignore the rest of the code.
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.
done
vllm/inputs/data.py
Outdated
|
||
def is_multimodal_inputs( | ||
inputs: Union[TokenInputs, "MultiModalInputsV2"] | ||
) -> TypeGuard["MultiModalInputsV2"]: |
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.
Same comment
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.
done
Signed-off-by: Tobias Pitters <[email protected]>
Signed-off-by: Tobias Pitters <[email protected]>
return inputs.get("prompt") | ||
|
||
assert_never(inputs) | ||
assert_never(inputs) # type: ignore[arg-type] |
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.
Hmm, if we still need to ignore this line, then I think there isn't much point in adding those two functions...
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.
We need the functions here: https://github.com/vllm-project/vllm/pull/11680/files#diff-e55f6ffbb4ac8db75ad60f0b92d2ab311493184b32bb039c2d991742f53f5c56R332 to narrow down the types. Assert_never won't work since mypy expects a "Never" object here and this is still an open issue (as mentioned in the description). I just thought, since we need the functions anyway, I consistently use them everywhere but can revert where they are not used if you prefer that.
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.
I mean that even after you added is_token_inputs
and is_multimodal_inputs
, you still have type ignore on the assert_never
lines. In that case, what is the benefit of adding those two functions?
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 point is that we need to narrow down the functions to MultiModalInputsV2 here since TokenInputs does not have the "mm_hashes" key and this results then in a mypy error. So to narrow the type (actually we just need one function to narrow down to MultiModalInputsV2) I added the function and then added the other to handle these cases consistently.
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.
Running mypy on the code prior to this PR, apart from assert_never
I only get one additional error (vllm/inputs/data.py:314
). So, I think it's better to just type ignore that extra line rather than using these two functions.
Btw, pyright passes on this file without any changes needed.
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.
alright, I just removed the functions and ignored the line.
This reverts commit 5986992. Signed-off-by: Tobias Pitters <[email protected]>
Signed-off-by: Tobias Pitters <[email protected]>
2a2b31c
to
085a5c2
Compare
Signed-off-by: Tobias Pitters <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Tobias Pitters <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Tobias Pitters <[email protected]>
6560143
to
2b7b4ad
Compare
Signed-off-by: Tobias Pitters <[email protected]>
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, thanks for your contribution and patience!
Supports #3680 (link existing issues this PR will resolve)
I ignored the
assert_never
lines since this seems like an open mypy issue: