Skip to content

Conversation

@yewentao256
Copy link
Member

@yewentao256 yewentao256 commented Oct 17, 2025

Purpose

Part of #26533

FIx mypy for vllm/v1/core and vllm/v1/engine

@hmellor CC

Test

pre-commit run --hook-stage manual mypy-3.10 -a
Found 31 errors in 9 files (checked 249 source files)
pre-commit run --hook-stage manual mypy-3.10 -a
Run mypy for Python 3.10.................................................Passed

Signed-off-by: yewentao256 <[email protected]>
Copy link
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 introduces fixes to address mypy errors in vllm/v1/core and vllm/v1/engine. The changes primarily involve adding type hints, explicit casts, and safer attribute access patterns to improve type safety and satisfy mypy checks.

I've found one critical issue in vllm/v1/engine/llm_engine.py where the new logic for processing request outputs can lead to data loss by incorrectly filtering a list of mixed-type results. I've provided a detailed comment and a suggested fix for this issue.

Other changes appear correct and effectively address the mypy errors.

Copy link

@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

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR.

Signed-off-by: yewentao256 <[email protected]>
@yewentao256 yewentao256 added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 17, 2025
@yewentao256
Copy link
Member Author

@hmellor CC


async def is_tracing_enabled(self) -> bool:
return self.observability_config.otlp_traces_endpoint is not None
return self.observability_config.otlp_traces_endpoint is not None # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Why does the type need to be ignored here? Should this not always return a bool?

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is vllm/v1/engine/async_llm.py:661: error: Item "None" of "ObservabilityConfig | None" has no attribute "otlp_traces_endpoint" [union-attr]

    async def is_tracing_enabled(self) -> bool:
        assert self.observability_config is not None
        return self.observability_config.otlp_traces_endpoint is not None

I think this is not very elegant so just #type ignore

Comment on lines 470 to 473
if hasattr(self.model_executor, "save_tensorized_model"):
self.model_executor.save_tensorized_model( # type: ignore[attr-defined]
tensorizer_config=tensorizer_config,
)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably error if we if the model executor doesn't have this method

Copy link
Member Author

@yewentao256 yewentao256 Oct 21, 2025

Choose a reason for hiding this comment

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

Just to confirm, you meant we should raise an error here in case the model executor doesn't have this method?

@mergify
Copy link

mergify bot commented Oct 21, 2025

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

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 21, 2025
Signed-off-by: yewentao256 <[email protected]>
@mergify mergify bot removed the needs-rebase label Oct 21, 2025
@yewentao256
Copy link
Member Author

Hi @hmellor, wondering is there anything else we should update?

@hmellor hmellor enabled auto-merge (squash) October 30, 2025 10:11
@hmellor hmellor merged commit c01f6e5 into main Oct 30, 2025
53 checks passed
@hmellor hmellor deleted the wentao-fix-mypy-v1 branch October 30, 2025 11:32
@DarkLight1337
Copy link
Member

This PR is failing pre-commit on main

@NickLucche
Copy link
Collaborator

@DarkLight1337 this wasn't force-merged right? Can we still break pre-commit with a clean-merge..?

@DarkLight1337
Copy link
Member

DarkLight1337 commented Oct 30, 2025

It wasn't. But this PR was behind main when it was merged.

MatthewBonanni pushed a commit to MatthewBonanni/vllm that referenced this pull request Oct 30, 2025
@njhill
Copy link
Member

njhill commented Oct 30, 2025

😢

@yewentao256
Copy link
Member Author

@DarkLight1337 Is there any way we can avoid this? A clean merge shouldn't cause trouble to CI

@hmellor
Copy link
Member

hmellor commented Oct 30, 2025

It happens when a PR changes a pre-commit rule.

  1. PR1 makes a change to a pre-commit rule (like this PR with mypy) and CI starts
  2. PR2 makes a change that would fail this new rule but the CI starts in parallel without the new rule applied
  3. Both PRs merge cleanly and now pre-commit fails on main

The best way to prevent this is to always merge from main before attempting to merge a PR which changes pre-commit rules.

ilmarkov pushed a commit to neuralmagic/vllm that referenced this pull request Nov 7, 2025
ZhengHongming888 pushed a commit to ZhengHongming888/vllm that referenced this pull request Nov 8, 2025
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
eldarkurtic pushed a commit to eldarkurtic/vllm that referenced this pull request Nov 12, 2025
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants