-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
[Misc] Avoid unnecessary import #21106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: wangxiyuan <[email protected]>
There was a problem hiding this 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 refactors the codebase to avoid early imports of model_executor, which is a good practice for improving startup time and resolving potential import cycle issues, especially for platform-specific builds like vllm-ascend.
The changes in vllm/entrypoints/openai/speech_to_text.py correctly use a lazy import inside a cached_property to defer the import of get_model_cls.
In vllm/lora/utils.py, the imports from model_executor are moved under a TYPE_CHECKING block, and the corresponding type hints are updated to use string forward references. This is a standard and correct way to handle type-only imports.
The changes are well-implemented and align with the stated goals. I don't see any issues. Good work!
Yikun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and the reason I also explained in #20900 (review) .
|
@DarkLight1337 Would you mind also taking a look? |
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
Signed-off-by: wangxiyuan <[email protected]> Signed-off-by: x22x22 <[email protected]>
Signed-off-by: wangxiyuan <[email protected]>
Signed-off-by: wangxiyuan <[email protected]>
Signed-off-by: wangxiyuan <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
Signed-off-by: wangxiyuan <[email protected]> Signed-off-by: Paul Pak <[email protected]>
Signed-off-by: wangxiyuan <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
Signed-off-by: wangxiyuan <[email protected]>
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Purpose
Similar with #20900, this PR clean up the unnecessary import (i.e. avoid to import model_executor too early) when vllm setup with online mode. This can fix the usage by vllm-ascend with
vllm servecommand.Test Plan
This PR is just for
importchange. Current test is enough.Test Result
All test should pass.
(Optional) Documentation Update