-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Platform] Add platform pluggable framework #11222
base: main
Are you sure you want to change the base?
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 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 do one of these:
🚀 |
df89a80
to
65b83e4
Compare
6b0a018
to
48a49bd
Compare
b43b61c
to
3ec575e
Compare
f31d075
to
1cb1934
Compare
189a199
to
a4b6e8c
Compare
a4b6e8c
to
beeecbf
Compare
Test OK on CPU device. Not sure the TPU CI failure related to this PR. Will take a look farther later. |
vllm/platforms/__init__.py
Outdated
current_platform = OpenVinoPlatform() | ||
else: | ||
current_platform = UnspecifiedPlatform() | ||
current_platform = CurrentPlatform() |
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.
I think you will lose type information this way since the methods of Platform
aren't annotated in CurrentPlatform
.
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.
yeah, this is a quick work to make the plugin run. Will fill up the code format、code comment、docstrings etc. later
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.
Is there a reason why you created this CurrentPlatform
class? I think it would be simpler to just make refresh_current_platform
a function in the global namespace and rename the current global _current_platform
to current_platform
.
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.
current_platform
is loaded in many place when import vllm
. Once it's imported by other module, the value in the other module would not be changed even though it's changed in platform
. Here is an example:https://gist.github.com/wangxiyuan/5af186668ccc41ec1d47694146a8fd1f
There are some way to fix the problem:
- do not import
current_platform
whenimport vllm
. This way will change a lot of file, it's not recommend.
The file need be changed.
# modified: vllm/config.py
# modified: vllm/distributed/parallel_state.py
# modified: vllm/engine/arg_utils.py
# modified: vllm/executor/ray_utils.py
# modified: vllm/model_executor/guided_decoding/__init__.py
# modified: vllm/model_executor/models/registry.py
# modified: vllm/model_executor/utils.py
# modified: vllm/spec_decode/metrics.py
# modified: vllm/usage/usage_lib.py
# modified: vllm/utils.py
# modified: vllm/worker/model_runner_base.py
# modified: vllm/worker/worker_base.py
- Like this PR does, create a dynamic load mechanism
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.
Thanks for the explanation! I understand now. Can you add this explanation to the docstring for future readers?
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.
sure, will update in next patch set
beeecbf
to
464e184
Compare
464e184
to
2ec32cd
Compare
2ec32cd
to
1006d7c
Compare
1006d7c
to
edbcfc8
Compare
Tested the latest code on my Tesla T4 GPU, everything works fine. Thx! |
11da4fd
to
701d1f7
Compare
Signed-off-by: wangxiyuan <[email protected]>
701d1f7
to
d6f685a
Compare
Signed-off-by: wangxiyuan <[email protected]>
fbf37c1
to
75eedc0
Compare
This PR is ready for review now. @DarkLight1337 @simon-mo @youkaichao |
See RFC: #11162
This PR is the first step of the RFC. More PR will be added until reach the RFC goal.
What is Done:
PlatformRegister
class. Provide some APIs for plugin to register and initialize out-of-tree platformNext Step:
platform hard code
in common placeHow to use:
vllm_new_device_plugin
.