-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
[platforms] enable platform plugins #11602
Merged
Merged
Changes from 30 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
10badce
refactor out common code
youkaichao 010bb3f
refactor
youkaichao 5ddbaa9
lazy init
youkaichao 5cdd326
add dummy platform tests
youkaichao 645b824
add tests
youkaichao 7b0864f
fix format
youkaichao a14c442
add docs
youkaichao d1c9015
lazy import
youkaichao db87b06
fast check
youkaichao 62469ad
add trace info
youkaichao 1c431dc
keep lazy
youkaichao 4dd2a73
keep lazy
youkaichao 6f8f7b4
keep lazy
youkaichao 5f6d883
keep lazy
youkaichao 715beda
keep lazy
youkaichao c2d8a5d
keep lazy
youkaichao 7def53a
keep lazy
youkaichao 2a66010
keep lazy
youkaichao 44f6a75
keep lazy
youkaichao d57b4e0
keep lazy
youkaichao 16e2eb5
keep lazy
youkaichao a50162f
keep lazy
youkaichao 0fa9293
fix lint
youkaichao 1a69264
fix lint
youkaichao 74f3bb6
add comments
youkaichao c91fa49
explicit params
youkaichao 49e2006
add more tests
youkaichao d379ef2
remove confusing is_platform
youkaichao aae2de5
update doc
youkaichao 8f43a03
improve comments
youkaichao 590f07a
soft fix
youkaichao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
from setuptools import setup | ||
|
||
setup( | ||
name='vllm_add_dummy_platform', | ||
version='0.1', | ||
packages=['vllm_add_dummy_platform'], | ||
entry_points={ | ||
'vllm.platform_plugins': [ | ||
"dummy_platform_plugin = vllm_add_dummy_platform:dummy_platform_plugin" # noqa | ||
] | ||
}) |
5 changes: 5 additions & 0 deletions
5
tests/plugins/vllm_add_dummy_platform/vllm_add_dummy_platform/__init__.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
from typing import Optional | ||
|
||
|
||
def dummy_platform_plugin() -> Optional[str]: | ||
return "vllm_add_dummy_platform.dummy_platform.DummyPlatform" |
5 changes: 5 additions & 0 deletions
5
tests/plugins/vllm_add_dummy_platform/vllm_add_dummy_platform/dummy_platform.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
from vllm.platforms.cuda import CudaPlatform | ||
|
||
|
||
class DummyPlatform(CudaPlatform): | ||
device_name = "DummyDevice" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
def test_platform_plugins(): | ||
# simulate workload by running an example | ||
import runpy | ||
current_file = __file__ | ||
import os | ||
example_file = os.path.join( | ||
os.path.dirname(os.path.dirname(os.path.dirname(current_file))), | ||
"examples", "offline_inference.py") | ||
runpy.run_path(example_file) | ||
|
||
# check if the plugin is loaded correctly | ||
from vllm.platforms import _init_trace, current_platform | ||
assert current_platform.device_name == "DummyDevice", ( | ||
f"Expected DummyDevice, got {current_platform.device_name}, " | ||
"possibly because current_platform is imported before the plugin" | ||
f" is loaded. The first import:\n{_init_trace}") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
I make this new test fast check, because I assume many PRs will break it easily by importing
current_platform
too early. Adding it as fast check gives people quick signal that they should lazy importcurrent_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.
Yeah, I think the problem now is that it's hard for users to know when the
current_platform
can be imported globally or lazily in future develop work. I know that all the action early thancurrent_platform
initiation should be lazy import. while for other developers, once they hit this kind of problem, it's a little complex and hard for them to debug. Any note or check can be added? Otherwise the PR LGTM.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 prefer the approach in #11222 where we essentially lazily evaluate the attributes of
current_platform
(rather thancurrent_platform
itself) even if it's imported early.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 have to point out that my PR #11222 has a disadvantage that
current_platform
should not be called directly in global namespace, for example https://github.com/vllm-project/vllm/pull/11222/files#diff-3bb4c705122f0ac754ce571780b1d75d5a8d444b2c99581385c1adfaeb8562e2R302The
__getattr__
design in this PR is a nice solution to fix the global namespace usage. https://github.com/vllm-project/vllm/pull/11602/files#diff-b353a4069aa142efe66166225c15f17b977a93fd3fc1d64482e18da127e420b8R195So maybe another way is to combine them together, so that
current_platform
can be imported early and can be used in global namespace as well.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.
As @wangxiyuan pointed out, #11222 has a silent correctness issue. There are some code using
@current_platform.inference_mode()
to decorate functions, e.g.vllm/vllm/worker/worker_base.py
Line 77 in dba4d9d
and in that PR, it directly resolves to
Platform.inference_mode
, even if we resolve the platform later to be a specific 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.
In that case, I suggest only exporting a
get_current_platform
method to indicate to other developers thatcurrent_platform
is lazy evaluated.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.
The goal of this PR (platform plugins), is to make sure, when a platform plugin is registered and should be activated, all usage of
current_platform
should resolve to that platform.To assure this goal, I added the test https://github.com/vllm-project/vllm/pull/11602/files#diff-b147065bfd9d20787e4c9d353c862732e9d1d797d5297ca36f6b131914d2cec1 , and also make sure the
current_platform
is resolved only once.if they import
current_platform
too early, then this test will fail, and the error message will tell them which line to blame.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.
@DarkLight1337 I think
get_current_platform
still cannot solve the@current_platform.inference_mode()
issue because it is indeed used during module import time. To make it work with platform plugins, we have to change the code to lazy import anyway.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, I'm just suggesting the name change to make it more obvious that
current_platform
isn't available immediately.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'm OK with this. Looks clear enough