[Misc][BE] Turn on strict type coverage for vllm/compilation#31756
Conversation
There was a problem hiding this comment.
Code Review
This pull request commendably enables strict mypy checking for the vllm/compilation module, a significant step towards improving code quality and maintainability. The changes are extensive and well-executed, adding necessary type hints and configurations. I have identified one area for improvement in vllm/compilation/decorators.py concerning the use of TypeVar, which, if addressed, would further enhance type safety and eliminate the need for several type: ignore suppressions.
|
This pull request has merge conflicts that must be resolved before it can be |
4e43ee8 to
34c18c8
Compare
34c18c8 to
251bdb3
Compare
| @@ -100,6 +100,12 @@ ignore_missing_imports = true | |||
| check_untyped_defs = true | |||
| follow_imports = "silent" | |||
|
|
|||
| [[tool.mypy.overrides]] | |||
| module = "vllm.compilation.*" | |||
There was a problem hiding this comment.
note for reviewer: This is not strict = True because this would cause cascading strict imports to other modules from follow_imports when checked with pre-commit only
Instead we default to a few sensible options here
251bdb3 to
9e82cb8
Compare
ProExpertProg
left a comment
There was a problem hiding this comment.
I see a ton of type[ignore]s - what are those for? Can we not fix them somehow? Because to me I think the noise of those ignores reduces the benefits of strict type checking
| hash_str: str = safe_hash( | ||
| str(factors).encode(), usedforsecurity=False | ||
| ).hexdigest()[:10] |
There was a problem hiding this comment.
We should just use the utils for hashing here that we use for other compile_hash functions (I think sha256)
There was a problem hiding this comment.
This should be the same util I believe (safe_hash)- we are just making the type checker happy by saying it is str type (since we run hexdigest()) and we reformat it due changing the line length
Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
9e82cb8 to
7194132
Compare
|
|
||
|
|
||
| def ignore_torch_compile(cls: _T) -> _T: | ||
| def ignore_torch_compile(cls: type[_T]) -> type[_T]: |
There was a problem hiding this comment.
Gemini bot caught a good bug here - _T should be type[_T] since it is the cls type
| return True | ||
| tp_size = get_tensor_model_parallel_world_size() | ||
| return compile_range.is_single_size() and compile_range.end % tp_size == 0 | ||
| return bool(compile_range.is_single_size() and compile_range.end % tp_size == 0) |
There was a problem hiding this comment.
how is this not already bool haha
There was a problem hiding this comment.
with skip follow_imports mode, it gets treated as Any by type checker - I know it is a bit silly, but imo the benefits of having strict type checking outweighs the extra syntax
zou3519
left a comment
There was a problem hiding this comment.
seems reasonable to me. @ProExpertProg thoughts on having strict typing? I don't think we need to block on resolving the type ignores before turning on strict typing
|
Hi @Lucaskabela, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
…oject#31756) Signed-off-by: Lucas Kabela <lucaskabela@meta.com> Signed-off-by: mohammad najafi <mohammad.najafi@amd.com>
…oject#31756) Signed-off-by: Lucas Kabela <lucaskabela@meta.com> Signed-off-by: 陈建华 <1647430658@qq.com>
…oject#31756) Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
|
May I ask, what is the purpose of enabling The current goal we have for |
…oject#31756) Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
Purpose
This PR follows up
And adds strict type checking to our linter/pyproject.toml, turning it on in the
compilationrepo as a test pilotTest Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Note
Turns on strict mypy for
vllm/compilationand updates pre-commit to run--stricton that directory.[tool.mypy.overrides]forvllm.compilation.*inpyproject.toml(disallow untyped/incomplete defs, warn return any)tools/pre_commit/mypy.pyto detect strict paths and run separate mypy invocations for strict and non-strict groupsvllm/compilation/*(e.g., explicit variable annotations/returns, boolean casts,Nonehandling, and targetedtype: ignoreon decorators/union attrs)Written by Cursor Bugbot for commit 251bdb337dcd680d68673cd8755c230fb9e07862. This will update automatically on new commits. Configure here.
Note
Turns on strict type checking for the compilation package and adapts tooling to enforce it.
vllm.compilation.*inpyproject.toml(disallow untyped/incomplete defs, warn onAnyreturns)tools/pre_commit/mypy.pyto detect strict paths and run separate mypy invocations (--strictforvllm/compilation, non-strict elsewhere)vllm/compilation/*(explicit annotations, temporary vars for returns, boolean casts,Nonechecks, and scopedtype: ignoreon decorators/union attrs)No functional behavior changes; changes are to satisfy stricter typing.
Written by Cursor Bugbot for commit 9e82cb8ae68eeb2f549c28a86c851178a5b7cfed. This will update automatically on new commits. Configure here.