Skip to content

[CI] Fix mypy for vllm/attention and vllm/compilation#26482

Closed
hmellor wants to merge 19 commits intovllm-project:mainfrom
hmellor:mypy-attention-compilation
Closed

[CI] Fix mypy for vllm/attention and vllm/compilation#26482
hmellor wants to merge 19 commits intovllm-project:mainfrom
hmellor:mypy-attention-compilation

Conversation

@hmellor
Copy link
Copy Markdown
Member

@hmellor hmellor commented Oct 9, 2025

This change brings us closer to not needing a custom mypy check. Part of #26533

This change brings us closer to not needing a custom `mypy` check.

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 aims to fix mypy issues to enable stricter type checking. While most changes correctly add or refine type hints, I've identified a few regressions. Specifically, a logic change in vllm/attention/selector.py removes the fallback to a default attention backend, which could break existing setups. Additionally, the logic for determining the default compilation level in vllm/config/vllm.py appears to be inverted with respect to the enforce_eager flag. Lastly, a type hint in vllm/v1/attention/backends/utils.py was changed incorrectly, which could lead to runtime errors. These issues are of high and critical severity and should be addressed.

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@mergify mergify bot added the tpu Related to Google TPUs label Oct 9, 2025
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>
else:
assert self.compilation_config.level >= CompilationLevel.NO_COMPILATION
assert self.compilation_config.level <= CompilationLevel.PIECEWISE
assert self.compilation_config.level <= 3
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed because level is Field(default=None, ge=0, le=3) so Pydantic will catch it

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@mergify
Copy link
Copy Markdown

mergify bot commented Oct 9, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @hmellor.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 9, 2025
@hmellor hmellor enabled auto-merge (squash) October 17, 2025 08:52
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 17, 2025
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>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Comment on lines +306 to +307
if not (self.model_config is None or self.model_config.enforce_eager):
self.compilation_config.mode = CompilationMode.VLLM_COMPILE
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic seems to have changed here. Did V0 deprecation proceed to a point that we can make these changes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, V0 is already gone

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've lived under a rock for too long, you're right - using the v0 engine just imports the v1 engine it looks like.

Some of the tests were relying on this behavior, but as long as they pass we're good

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any tests do rely on V0 defaults, I'll update them

@mergify
Copy link
Copy Markdown

mergify bot commented Oct 23, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @hmellor.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 23, 2025
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@mergify
Copy link
Copy Markdown

mergify bot commented Nov 11, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @hmellor.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Nov 11, 2025
Copy link
Copy Markdown
Member

@yewentao256 yewentao256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the pre-commit and conflicts so that we can land this PR

@ProExpertProg
Copy link
Copy Markdown
Collaborator

@hmellor are you still planning to merge this PR?

@hmellor
Copy link
Copy Markdown
Member Author

hmellor commented Dec 1, 2025

I merged from main locally a few days ago, but have been focusing on Transformers v5 the past couple of weeks.

Would you like to take over the compilation part?

@mergify mergify bot added the cpu Related to CPU backends label Dec 17, 2025
@hmellor hmellor closed this Jan 27, 2026
auto-merge was automatically disabled January 27, 2026 20:30

Pull request was closed

@github-project-automation github-project-automation bot moved this to Done in NVIDIA Jan 27, 2026
@hmellor hmellor deleted the mypy-attention-compilation branch January 27, 2026 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cpu Related to CPU backends needs-rebase nvidia ready ONLY add when PR is ready to merge/full CI is needed tpu Related to Google TPUs v1

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants