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

[plugin] move custom executor to plugins #7879

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

youkaichao
Copy link
Member

move the custom executor introduced in #6557 to plugin system, to reduce the code complexity of the main codebase.

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 consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

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

🚀

@WoosukKwon
Copy link
Collaborator

Thanks for the PR! However, I'd like to push back this PR and this line of effort:

  1. I think the executors are not the source of code complexity. While supporting different hardware backends does increase complexity, this is orthogonal to the executor abstraction. The complexity comes from the fact that the shared system components like scheduler should work with all different backends, which is still the case even if we make the executors pluggable.
  2. Different hardware backends touch more than the executor/worker/model runner. They often touch models, weight loading utilities, device communicators, etc. Therefore, separating out them from vllm main repo may not remove the complexity and instead add another complexity of versioning.
  3. I feel this is a bit too early. Can we focus on other refactoring/simplification first and get back to this topic? I think there are higher priority items that might affect the APIs and eventually this PR.

Copy link

mergify bot commented Nov 26, 2024

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @youkaichao.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants