-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
[CI] Fix mypy for vllm/engine and vllm/utils
#26540
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
base: main
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request focuses on fixing mypy pre-commit issues to enable stricter type checking for the vllm/engine module. The changes primarily involve adding explicit type casts, updating type hints, and using type: ignore to resolve type errors reported by mypy. The fixes are generally correct and achieve the goal of making the code mypy-compliant. However, I've identified two instances in vllm/multimodal/parse.py where using # type: ignore hides a potential runtime error. In these cases, the code does not handle None values that can appear in a list of multimodal data, which could lead to exceptions. I've suggested a more robust fix that handles these None values gracefully while also resolving the type error.
|
This pull request has merge conflicts that must be resolved before it can be |
|
This pull request has merge conflicts that must be resolved before it can be |
yewentao256
left a 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.
Please also fix the pre-commit issue and merge from main, so that we can get this landed
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
|
|
||
| # Setup Scheduler With Mock External Cache Hit. | ||
| BLOCK_SIZE = 4 | ||
| BLOCK_SIZE = 8 |
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.
Are these changes to block size intentional?
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 think so. To satisfy the type validation, block size has to be one of [1, 8, 16, 32, 64, 128] cc: @hmellor
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.
cc @heheda12345 @tlrmchlsmth is it ok to change these tests to use the correct block sizes?
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.
Before, we were not validating the block size but we did specify a list of valid block sizes. We should either update the test or update the possible valid block sizes
|
This pull request has merge conflicts that must be resolved before it can be |
yewentao256
left a 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.
Please take a look at the CI issue, try if we can reproduce it locally
|
Thanks! I can reproduce it offline. It seems using |
|
I'll take a look at the failures too |
|
Looks like I fixed this locally and forgot to push... One moment |
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Part of #26533
Fix mypy precommit issues under "vllm/engine" and "vllm/utils" and move them to "FILES"
CC: @hmellor @yewentao256
Original: