[core] clean up executor class hierarchy between v1 and v0 #12171
Conversation
Signed-off-by: youkaichao <[email protected]>
youkaichao
requested review from
WoosukKwon,
njhill,
ywang96,
comaniac and
zhuohan123
as code owners
|
👋 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:
🚀 |
|
this PR will fix the error on main: https://buildkite.com/vllm/ci/builds/12093#0194744e-359d-40d2-ad87-a74e1fb40f38 |
There was a problem hiding this comment.
Great simplification to the executor code! But I remembered that one coding convention of v1 is that we should make the dependency between v0 code and v1 code simple and can achieve it via some code duplication. Correct me if it is wrong. @WoosukKwon
vllm/v1/executor/abstract.py
Outdated
| class Executor(ABC): | ||
| """Abstract class for executors.""" | ||
| class Executor(ExecutorBase): | ||
| """Abstract class for v1 executors, mainly define some new methods.""" |
There was a problem hiding this comment.
A more clear comment: "Abstract class for v1 executors, mainly define some methods for v1. Define methods shared by v0 and v1 in ExecutorBase" And how to add an interface to only v0?
There was a problem hiding this comment.
how to add an interface to only v0
we don't introduce new features that only work for v0.
vllm/v1/executor/abstract.py
Outdated
| self.collective_rpc("profile", args=(is_start, )) | ||
|
|
||
|
|
||
| class UniprocExecutorV1(UniProcExecutor, Executor): |
There was a problem hiding this comment.
Seems that we don't have xxxV1 class in v1 code now. Will this be better? from vllm.executor.uniproc_executor import UniProcExecutor as UniProcExecutorV0
There was a problem hiding this comment.
sounds good, added in 21a4b0e
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
This is a followup work of #11256 . After that PR, the executor hierarchy becomes quite messy:
This makes adding new functions quite confusing. For example, #11960 adds some new functions like
get_kv_cache_spec, but forgot to add it in the ray executor (which lives in v0 folder).I'd like to clean up the hierarchy in this PR. The basic functionality of executor lives in v0 folder. v1 executors mainly inherit and extend new functions (interfaces). They can just delegate the function to
collective_rpc.After this PR, if we want to add a new interface in v1, we just need to touch
vllm.v1.executor.abstract.Executor. If we want to add a new interface in both v0 and v1, we just need to touchvllm.executor.executor_base.ExecutorBase.It is also possible to design the functions so that we don't even need
UniprocExecutorV1etc. But right now, the signature ofexecute_modelconflicts. So I leave it as-is now.