Skip to content
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 31 commits into from
Dec 30, 2024

Conversation

youkaichao
Copy link
Member

refactor of #11222 , enable oot-registered platforms.

Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Copy link

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@mergify mergify bot added documentation Improvements or additions to documentation ci/build labels Dec 29, 2024
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
- label: Plugin Tests (2 GPUs) # 40min
working_dir: "/vllm-workspace/tests"
num_gpus: 2
fast_check: true
Copy link
Contributor

Choose a reason for hiding this comment

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

if they import current_platform too early, then this test will fail, and the error message will tell them which line to blame.

I'm OK with this. Looks clear enough

@@ -265,13 +264,13 @@ def prepare_model_input(
"""
raise NotImplementedError

@current_platform.inference_mode()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is removed directly. It's useless now?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is moved to https://github.com/vllm-project/vllm/pull/11602/files#diff-1ca8953d4dab62fff08a34c8cf370d2a37aeb009526ab4fc5464a65e1d03b036R84 .

this line does nothing actually. no subclass calls this function.

Copy link
Contributor

@wangxiyuan wangxiyuan left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 197 to 198
# lazy init current_platform so that plugins can import vllm.platforms
# to inherit Platform without circular imports
Copy link
Member

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but isn't the main reason for lazy init is to allow platform plugins to be loaded before the built-in ones?

Copy link
Member

Choose a reason for hiding this comment

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

The circular import problem should already be solved by returning strings in the built-in plugins instead of direct import.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, you are right.

what this line of comment mean:

for third-party platform developers, they also need to import vllm.platforms and inherit the base Platform. therefore, we cannot resolve the current platform during import vllm.platforms

Copy link
Member

@DarkLight1337 DarkLight1337 Dec 30, 2024

Choose a reason for hiding this comment

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

I think we can avoid this import by moving the code into a registry.py, similar to that for models.

Copy link
Member Author

Choose a reason for hiding this comment

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

even if we have vllm.platforms.registry , when people import that module, vllm/platforms/__init__.py will be executed.

Copy link
Member

Choose a reason for hiding this comment

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

We can also move current_platform into vllm.platforms.registry so it's not imported automatically when accessing other platform modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

explained in 8f43a03 . let me know if it works.

Copy link
Member

Choose a reason for hiding this comment

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

This is much clearer now, thanks!

Signed-off-by: youkaichao <[email protected]>
Copy link
Member

@DarkLight1337 DarkLight1337 left a comment

Choose a reason for hiding this comment

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

I really don't like the necessity of the lazy import (similar to CUDA re-initialization problem) but we can iterate on this later if needed...

@youkaichao
Copy link
Member Author

I really don't like the necessity of the lazy import (similar to CUDA re-initialization problem)

I agree we should avoid lazy import if possible. But there are some historical code we need to take care of, many people unconsciously initialize cuda or trigger current_platform resolution :(

@youkaichao youkaichao enabled auto-merge (squash) December 30, 2024 08:35
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 30, 2024
@MengqingCao
Copy link
Contributor

MengqingCao commented Dec 30, 2024

Sorry for the delay, Overall lgtm.

Just one question. Maybe this pr lack of a func like is_oot_platform() in vllm/platforms/interface.py? I think we should expose a similar api for oot platforms to use. And an expanded PlatformEnum is also needed for this.

@youkaichao
Copy link
Member Author

Maybe this pr lack of a func like is_oot_platform() in vllm/platforms/interface.py? I think we should expose a similar api for oot platforms to use. And an expanded PlatformEnum is also needed for this.

We can add it later if it is necessary. Right now I don't see the necessity. For the oot platform related code, they always run on oot platforms; for vllm's main branch code, they don't need to consider oot platform code.

I think vllm mainly exposes integration hooks to oot platforms. for example, when vllm's code use current_platform.device_name , oot platform should directly benefit from it by providing its own device_name . I don't see when it is necessary to tell whether we are using a builtin platform or oot platform.

@MengqingCao
Copy link
Contributor

We can add it later if it is necessary. Right now I don't see the necessity. For the oot platform related code, they always run on oot platforms; for vllm's main branch code, they don't need to consider oot platform code.

I think vllm mainly exposes integration hooks to oot platforms. for example, when vllm's code use current_platform.device_name , oot platform should directly benefit from it by providing its own device_name . I don't see when it is necessary to tell whether we are using a builtin platform or oot platform.

OK, thx! We will verify NPU as an oot device recently. If necessary, I will add it through a new PR.

Signed-off-by: youkaichao <[email protected]>
@youkaichao
Copy link
Member Author

failed tests are not related, merging.

@youkaichao youkaichao disabled auto-merge December 30, 2024 12:24
@youkaichao youkaichao merged commit b12e87f into vllm-project:main Dec 30, 2024
75 of 77 checks passed
@youkaichao youkaichao deleted the platform_plugin branch December 30, 2024 12:24
BKitor pushed a commit to BKitor/vllm that referenced this pull request Dec 30, 2024
xcnick pushed a commit to xcnick/vllm that referenced this pull request Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build documentation Improvements or additions to documentation ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants