-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[Core] Some simplification of WorkerWrapper changes #4183
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good to me. Thanks!
vllm/executor/ray_gpu_executor.py
Outdated
init_worker_all_kwargs.append({ | ||
"model_config": self.model_config, | ||
"parallel_config": self.parallel_config, | ||
"scheduler_config": self.scheduler_config, | ||
"device_config": self.device_config, | ||
"cache_config": self.cache_config, | ||
"load_config": self.load_config, | ||
"local_rank": local_rank, | ||
"rank": rank, | ||
"distributed_init_method": distributed_init_method, | ||
"lora_config": self.lora_config, | ||
"vision_language_config": self.vision_language_config, | ||
"is_driver_worker": rank == 0, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this, but i think keep the calling-like code is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youkaichao could you explain why? it's more code and more indirection and not any more readable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps subjective but imo this is actually more readable due to the spacing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future I plan to use WorkerWrapper for all executors. calling-like code will be easier to modify because it is similar to existing code, but dict-like code would require many changes.
Kind of like javascript syntax suger: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer
Maybe we should move collect_arg_helper_func
to utils, so that in the future all executors can use this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I'll revert it. But IMHO it's not a good idea to complicate things based on ideas of future generalization, better to do that at the time. Won't it be essentially the same set of parameters used to initialize the other kinds of workers in other executors too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youkaichao ok reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@njhill there are some bugs in argument passing |
self, | ||
method: str, | ||
*args, | ||
driver_args: Optional[Tuple[Any]] = None, | ||
driver_args: Optional[Tuple[Any, ...]] = None, | ||
driver_kwargs: Optional[Dict[str, Any]] = None, | ||
all_args: Optional[List[List[Any]]] = None, | ||
all_args: Optional[List[Tuple[Any, ...]]] = None, | ||
all_kwargs: Optional[List[Dict[str, Any]]] = None, | ||
use_dummy_driver: bool = False, | ||
max_concurrent_workers: Optional[int] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, it's better to document in the doc string to note several usages:
- only args/kwargs , all workers share the same args
- only args/kwargs + driver_args/driver_kwargs , all workers except for the driver share the same args
- all_args/all_kwargs , specify each arg/kwargs for each worker
use_dummy_driver
is orthogonal to the above, indicating whether to use dummy driver worker.
As this function goes more and more complicated, I'm thinking of simplifying the usecases. Maybe only 1 and 3 are allowed.
92251b4
to
08ccf5f
Compare
I am still catching up with reviews and only now looking at #4024 which was merged recently, because it impacts #3466 and #3763 that I need to rebase.
These are some minor changes to fix typing, reduce verbosity, reduce list allocations/slicing when running workers, remove some redundant code, etc.