[CI/Build] mypy: Resolve some errors from checking vllm/engine #9267
Conversation
russellb
requested review from
WoosukKwon,
zhuohan123,
youkaichao,
alexm-neuralmagic,
comaniac and
njhill
as code owners
|
👋 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:
🚀 |
DarkLight1337
added
the
ready
|
@njhill @DarkLight1337 Thank you both for the close review! I will let you know once I have addressed the feedback. |
russellb
force-pushed
the
mypy
branch
2 times, most recently
from
ac36d1b to
eb164bd
Compare
|
marked as a draft to fix some things - will mark ready for review again when done |
I had all directories listed in `tools/mypy.sh` to ensure they worked with `--follow-imports skip`. This change includes the last tweaks necessary for all of those to pass. As a next step, remove the ones that are already running with `--follow-imports silent` based on the config in `pyproject.toml`. Following commits will start resolving errors that occur when adding more coverage here. Signed-off-by: Russell Bryant <[email protected]>
This fixes 27 of the 100 errors that occur when running:
mypy --follow-imports silent vllm/engine
It is submitted as a checkpoint to make it easier to review.
Signed-off-by: Russell Bryant <[email protected]>
`backend` was used in two different places in this function as two different types. There was no error here, but `mypy` didn't like it. Signed-off-by: Russell Bryant <[email protected]>
The code here previously raised an error in mypy because the `forward` method may not exist on the decorated class. Add explicit validation of this requirement. Signed-off-by: Russell Bryant <[email protected]>
In the support_torch_compile() decorator, mypy doesn't like the lines overriding class methods. The code looks fine to me, so just ignore the warnings here. Signed-off-by: Russell Bryant <[email protected]>
Several places there are assumptions that a type is a more specific type than the generic one specified in function parameters. The parameters are set because it's based on a more generic parent class, so we can't override them here. Signed-off-by: Russell Bryant <[email protected]>
russellb
added
ready
|
looks like there's a real failure in a metrics test. I'm looking at it now. |
mypy pointed out that a couple of variables are Optional (could be None), but we are adding them to a float or int. Ensure that we don't try to add None. Signed-off-by: Russell Bryant <[email protected]>
| @@ -92,7 +93,7 @@ class RequestOutput: | |||
| def __init__( | |||
| self, | |||
| request_id: str, | |||
| prompt: Optional[str], | |||
| prompt: Optional[PromptType], | |||
There was a problem hiding this comment.
Sorry for the after-the-fact comment and for missing in review .. I think this one might not be the right change ... it breaks things in other places and is also part of the API so probably shouldn't change in this way.
I think probably the reason it was changed was due to the recent beam_search logic which still needs a lot of work and I think incorrectly is assigning a var of PromptType to this.
I can address this in another PR that I'm working on right now.
There was a problem hiding this comment.
@njhill sorry about that. I think it was OK at one point, and then when I rebased on main some new errors started happening and I didn't fix them before merge. Let me know if you'd like me to just revert this if you want it fixed quickly.
There was a problem hiding this comment.
Thanks @russellb no problem I have a PR almost ready
This reverts a recent typing change that was made in vllm-project#9267 which was due to another recent incorrect change in the "external" beam search implementation. It would be a breaking change to the type of RequestOutput.prompt in the API (and caused some typing breakages to the front-end API code which references this field). I have made some related changes/fixes to the beam search impl including properly returning logprobs, but it still needs quite a bit more work overall (for subsequent PRs).
…project#9267) Signed-off-by: Russell Bryant <[email protected]> Signed-off-by: charlifu <[email protected]>
…project#9267) Signed-off-by: Russell Bryant <[email protected]> Signed-off-by: Vinay Damodaran <[email protected]>
…project#9267) Signed-off-by: Russell Bryant <[email protected]> Signed-off-by: Alvant <[email protected]>
…project#9267) Signed-off-by: Russell Bryant <[email protected]> Signed-off-by: Amit Garg <[email protected]>
…project#9267) Signed-off-by: Russell Bryant <[email protected]> Signed-off-by: qishuai <[email protected]>
…project#9267) Signed-off-by: Russell Bryant <[email protected]> Signed-off-by: Sumit Dubey <[email protected]>
…project#9267) Signed-off-by: Russell Bryant <[email protected]>
…project#9267) Signed-off-by: Russell Bryant <[email protected]> Signed-off-by: Maxime Fournioux <[email protected]>
…project#9267) Signed-off-by: Russell Bryant <[email protected]> Signed-off-by: Tyler Michael Smith <[email protected]>
This PR is a chunk of mypy checking progress.
I got to where I knew everything would pass when running
mypywith--follow-imports skip. (though some new issues have crept back in since I passed this point)I then started working on expanding the coverage for what can be checked
using
--follow-imports silent. Ongoing progress will come in chunks to make iteasier to review. I started with 100 errors and it's down to 60 right now.
Part of issue #3680