[platforms] absorb worker cls difference into platforms folder

[youkaichao]

part of #9268

every platforms should specify the worker class inside their own code.

in addition, the default case is auto , and this allows users to specify custom classes for extensibility (which i'm working on as part of an RLHF support).

[github-actions]

👋 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.

🚀

[comaniac]

LGTM

[njhill]

Thanks @youkaichao! I am also reviewing this now...

[njhill]

@youkaichao I think this change is probably ok if we're going to continue iterating on the architecture, but it doesn't seem like the right end design to me.

I've already been thinking that we need to overhaul the executor hierarchy/abstractions a bit (hoping we can do this as part of v1), and that may be part of why this doesn't sit right.

In particular we have one or more executor class(es) per platform so this is in some way abstracting over the platforms. But then there's a parallel Platform abstraction. I think we should get rid of the platform-specific executors. I.e. no ray_*pu_executor.pys. Possibly the platform-specific aspects could be a mix-in.

It also feels a bit wrong to me to update the config objects in-place since these might be created/ "owned" by the user.

Also wdyt about changing this field to be custom_worker_cls: Optional[str] = None? Since it's a very specialized option I would consider it more overriding vLLMs native behaviour so it's not so much an "auto" thing.

[youkaichao]

I think we should get rid of the platform-specific executors. I.e. no ray_*pu_executor.pys. Possibly the platform-specific aspects could be a mix-in.

I strongly agree. We should only have {single worker executor, ray executor, mp executor} , and they should be able to initialize various workers.

I think this PR is a tiny step towards that direction.

[youkaichao]

It also feels a bit wrong to me to update the config objects in-place since these might be created/ "owned" by the user.

what's the concern here? the problem is we don't have a scratch space for the model to store some information, and right now we use vllm_config a lot, to store per-model basis global variables.

[njhill]

It also feels a bit wrong to me to update the config objects in-place since these might be created/ "owned" by the user.

what's the concern here? the problem is we don't have a scratch space for the model to store some information, and right now we use vllm_config a lot, to store per-model basis global variables.

Yeah it's kind of a more general point than this PR ... like you say two things are being conflated a bit. Ideally the config should be treated as read-only I think (could have been passed in by the user) and model global mutable state should be separate.

Perhaps something like:

@dataclass
class ModelState:
    config: VllmConfig
    
    # ...

[youkaichao]

class ModelState:
config: VllmConfig

# ...

@njhill this makes sense, but you need to figure out where to store it. All the classes, including engine, executor, worker, model runner, model needs to access it. And you cannot use module-level global state because people can create multiple LLM object in the same process.

[youkaichao]

wdyt about changing this field to be custom_worker_cls: Optional[str] = None?

I use a single worker_cls because it will be printed out during init.

Printing:

worker_cls = "vllm.worker.worker.Worker"
custom_worker_cls = "whatever.user.provide"

looks less clear than:

worker_cls = "auto"

and

worker_cls = "whatever.user.provide"

[njhill]

We can address the other suggestions as follow-on refactoring