[Core] Centralize GPU Worker construction

[njhill]

Add _get_worker_kwargs and _create_worker methods to GPUExecutor which construct Worker instances from the executor configuration, and make use of these in places where the logic is currently duplicated.

This will also be used by the to-be-added MultiprocessingGPUExecutor.

[youkaichao]

Is this WorkerWrapperBase.init_worker ?

[njhill]

@youkaichao I don't think so, this is primarily centralizing how the GPU executor config is converted to the worker args.

I don' think WorkerWrapperBase.init_worker is necessarily needed in non-ray cases, the "wrapped" worker init can just be done as part of the remote worker init rather than having separate RPCs to set up the env vars and then init the worker.

[youkaichao]

I mean, they can share the same code to reduce code duplication. You can include WorkerWrapperBase.init_worker in your code without a rpc call. This way we have a unified entrypoint of worker construction, and we can inject some common logic like log handling, enable_trace_function_call_for_thread, etc.

My design of WorkerWrapperBase is definitely not just tailored to ray.

[njhill]

@youkaichao maybe better to move that common logic to a static function in base_worker.py? since for example in the single GPU case there isn't a wrapper, and it can be called from the existing WorkerWrapperBase.init_worker method (I can make that change if you agree). Or else could you suggest what changes would be made to this PR?

[youkaichao]

I think WorkerWrapper is required for every Worker, but I didn't find time to do it. The idea is that we need to inject some common logic before every worker starts to initialize, e.g. enable logging, debug tracing, setting environment variables. That's why I designed the WorkerWrapperBase to have arguments of module name and module class name: after we initialize WorkerWrapper, before initialize Worker, the whole Worker module is not imported, meaning that none of the code from Worker-related module will be executed.

[njhill]

@youkaichao I'm using WorkerWrapperBase to initialize the single GPU worker now, PTAL!

[youkaichao]

Looks good to me, thanks!

[youkaichao]

A side note @njhill :

Initially, I would like to make single gpu executor more similar to multi-gpu executor, e.g. move _run_workers function to ExecutorBase. Then all executors just need to run self._run_workers("init_worker"), self._run_workers("init_device"), self._run_workers("load_model") . This unifed interface looks better to me. Different executors just have different _run_workers implementation (and of course, different _init_executor implementation).

I'm not sure if it is good to go. It will have some small overhead for single-gpu execution though, but I expect the overhead to be minimal.

[njhill]

@youkaichao that does make sense from a unification pov, but the code was actually originally like this before @zhuohan123 refactored it to be as it is now in #3191.

Perhaps the reason was so that we can streamline/optimize the single-gpu case which is potentially most common

[youkaichao]

Thanks for pointing it out. Didn't notice it before.