[Misc][BE] Type coverage for vllm/compilation [3/3]#31748
[Misc][BE] Type coverage for vllm/compilation [3/3]#31748zou3519 merged 3 commits intovllm-project:mainfrom
Conversation
08aa1b1 to
c102a4b
Compare
There was a problem hiding this comment.
Code Review
This pull request focuses on improving type hint coverage across the vllm/compilation module, which is a valuable contribution to code quality and maintainability. The changes are extensive and well-executed, adding type hints to many functions and methods. I've identified one critical issue in vllm/compilation/collective_fusion.py where a replacement function in a pattern matcher returns a list of tensors instead of a single tensor, which will likely lead to a runtime error. Please address this issue.
|
This pull request has merge conflicts that must be resolved before it can be |
c102a4b to
06e6343
Compare
Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
06e6343 to
78350f9
Compare
|
If someone asks: this PR does refactor get_inputs to be a methods on some of these classes but doesn't require it in the base class |
| if find_spec("flashinfer"): | ||
| try: | ||
| import flashinfer.comm as flashinfer_comm | ||
|
|
||
| flashinfer_comm = ( | ||
| flashinfer_comm: ModuleType | None = ( # type: ignore[no-redef] | ||
| flashinfer_comm | ||
| if hasattr(flashinfer_comm, "trtllm_allreduce_fusion") | ||
| else None | ||
| ) | ||
| except ImportError: | ||
| flashinfer_comm = None | ||
| flashinfer_comm = None # type: ignore[assignment] | ||
| else: | ||
| flashinfer_comm = None | ||
| flashinfer_comm = None # type: ignore[assignment] | ||
|
|
There was a problem hiding this comment.
Can we just set flashinfer_comm=None at the start and avoid the type: ignores?
There was a problem hiding this comment.
If we set flashinfer_comm = None above this logic, we still need to add ignores for redef on L35 and L37 so not sure if it would save us much for code cleanliness
There was a problem hiding this comment.
I thought that would just be reassignment not definition? Also I see the import is the same as the var, let's rename the import to _flashinfer_comm?
Signed-off-by: Lucas Kabela <lucaskabela@meta.com> Signed-off-by: Tomer Natan <tbarnatan@computelab-frontend-8.nvidia.com>
Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
Signed-off-by: Lucas Kabela <lucaskabela@meta.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
| _ROPE_DICT: dict[tuple, RotaryEmbedding] = {} | ||
| _ROPE_DICT: dict[tuple[Any, ...], RotaryEmbedding] = {} | ||
|
|
||
| __all__ = ["RotaryEmbedding"] |
Signed-off-by: Lucas Kabela <lucaskabela@meta.com>
Purpose
We want to provide better type hint coverage in vllm/compilation to improve maintainability, readability, and reduce silent errors
This PR should be applied on top of #31744
Test Plan
mypy vllm/compilationTest Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Note
Improves type coverage and API clarity across compilation passes with minimal logic adjustments.
get_inputsmethods across fusion passes (activation, RMSNorm, attention, collective, sequence parallelism, ROCm/AIter)__init__,register,__call__,uuid, helpers likeempty_*) and use precise tuple/list return types in pattern/replacement fnswrap_trace_fn), reshape conversions, and first-return-only helpers with typingSourceInfo, mark dynamic/unbacked dims conditionally, and patch configs with typed contextsparallel_state, rotary embedding__all__and cache key typesmypy: Success on 28 files (no issues).
Written by Cursor Bugbot for commit 06e6343b18cf59113c80149a18f93b7ceeb988ac. This will update automatically on new commits. Configure here.
Note
Improves type coverage and API clarity across
vllm/compilationwith minimal logic changes.ParamSpec) and tighten signatures for__init__,register,__call__,uuid, and helper fns; pattern/replacement fns now return precise tuplesget_inputs()helpers for patterns and unify tracing viawrap_trace_fn, view→reshape conversions, and no-op permute cleanupNone), workspace setup, and one-shot size checks; minor no-op cleanup hook in sequence parallelismdistributed/parallel_stateandrotary_embedding(__all__, cache key types)Written by Cursor Bugbot for commit 78350f9. This will update automatically on new commits. Configure here.