-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
improved code readability #72
Conversation
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
👋 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:
🚀 |
vllm/worker/spyre_worker.py
Outdated
@@ -59,7 +59,7 @@ def __init__( | |||
self.is_driver_worker) | |||
self._env_initialized = False | |||
|
|||
def init_distributed_environment(self) -> None: | |||
def init_distributed_spyre_environment(self) -> 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.
all the workers have names init_distributed_environment
, imo it is better to keep it that way for consistency
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.
ref: openvino, cpu, neuron
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 agree. I thought it was a Spyre specific thing that we introduced. If its their convention to have the same name for the two functions I guess we just stick to it too.
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
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
this PR improves code readability by - [commit 1](0c9bec0): removing the batch size argument in the load_model function [here](https://github.com/IBM/vllm/blob/124f3a961d1a9ce2628c01fe56dfcc589a49c8dd/vllm/worker/spyre_worker.py#L135) since unused in `SpyreModelRunner` ([here](https://github.com/IBM/vllm/blob/124f3a961d1a9ce2628c01fe56dfcc589a49c8dd/vllm/worker/spyre_model_runner.py#L104)) as well as in `SpyreEmbeddingModelRunner` ([here](https://github.com/IBM/vllm/blob/124f3a961d1a9ce2628c01fe56dfcc589a49c8dd/vllm/worker/spyre_embedding_model_runner.py#L52)). - -- _[commit 2](beadbd5): function renaming to better distinguish between the imported function `init_distributed_environment` from `vllm.distributed` and our own function `self.init_distributed_environment`._ -- (**reverted since this seems to be a convention introduced in other worker classes**) --------- Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
this PR improves code readability by
SpyreModelRunner
(here) as well as inSpyreEmbeddingModelRunner
(here).init_distributed_environment
fromvllm.distributed
and our own functionself.init_distributed_environment
. -- (reverted since this seems to be a convention introduced in other worker classes)