Skip to content

Conversation

@MatthewBonanni
Copy link
Contributor

@MatthewBonanni MatthewBonanni commented Nov 26, 2025

Purpose

Now that #29342 has landed, many of the TYPE_CHECKING and lazy imports are no longer necessary. This PR updates them throughout the codebase

Test Plan

Recommend to run all CI tests (ready-run-all-tests)

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: Matthew Bonanni <mbonanni@redhat.com>
@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.

@mergify mergify bot added nvidia rocm Related to AMD ROCm speculative-decoding labels Nov 26, 2025
@mergify mergify bot added v1 tpu Related to Google TPUs labels Nov 26, 2025
@mergify mergify bot added the kv-connector label Nov 26, 2025
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 is a large-scale refactoring that removes lazy imports and TYPE_CHECKING blocks related to attention backends throughout the codebase. The changes are consistent and correctly applied, moving imports to the top level and updating type hints accordingly. This cleanup improves code readability and structure. I have reviewed all the changes and found no issues. The refactoring appears to be safe and well-executed.

@MatthewBonanni
Copy link
Contributor Author

cc @mgoin

Signed-off-by: Matthew Bonanni <mbonanni@redhat.com>
Copy link
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.

Thanks for the work! Let's run CI and see if we can pass

@yewentao256 yewentao256 added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 26, 2025
Signed-off-by: Matthew Bonanni <mbonanni@redhat.com>
@hmellor
Copy link
Member

hmellor commented Nov 26, 2025

It's worth noting that the TYPE_CHECKING guard isn't always a workaround for circular imports. It can save time at runtime by not importing heavy modules that are only used for type checking.

Copy link
Collaborator

@LucasWilkinson LucasWilkinson left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for doing this!

@github-project-automation github-project-automation bot moved this to In review in NVIDIA Nov 27, 2025
Copy link
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.

LGTM, thanks for the work!

@yewentao256 yewentao256 merged commit fc1d8be into vllm-project:main Nov 27, 2025
67 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in NVIDIA Nov 27, 2025
@NickLucche
Copy link
Collaborator

NickLucche commented Nov 27, 2025

It's worth noting that the TYPE_CHECKING guard isn't always a workaround for circular imports. It can save time at runtime by not importing heavy modules that are only used for type checking.

yeah I am not really sure it was worth changing 40 files for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kv-connector nvidia ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm speculative-decoding tpu Related to Google TPUs v1

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants