Skip to content

Add vllm colocation - #3

Draft
toslali-ibm wants to merge 33 commits into
mainfrom
coloc
Draft

Add vllm colocation#3
toslali-ibm wants to merge 33 commits into
mainfrom
coloc

Conversation

@toslali-ibm

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a GitHub issue? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

Comment thread trl/trainer/grpo_trainer.py Outdated
(self.accelerator.process_index + 1) * len(prompts),
)
completion_ids = completion_ids[process_slice]
# corresponding slice (if colocated, no need for broadcasting).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

best is to eventually move this control logic into the class

Comment thread trl/trainer/grpo_trainer.py Outdated
all_prompts_text = gather_object(prompts_text)
if self.accelerator.is_main_process:
# Generate completions using vLLM: gather all prompts and use them in a single call in the main process (if colocated, work on your own batch)
all_prompts_text = prompts_text if self.args.vllm_colocation else gather_object(prompts_text)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

control logic move into class

)
vllm_dtype: Optional[str] = field(
default=None,
default="auto",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we should change this..

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used to be auto - now it is not used in the upstream repo, but we need it.

Comment thread trl/extras/vllm_client.py Outdated


class VLLMClient:
class VLLMClient(BaseVLLMClient):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think to keep it simple, jsut put all clients in one file.

Comment thread trl/trainer/grpo_trainer.py Outdated
# synchronize all processes after vLLM has been fully initialized.
self.accelerator.wait_for_everyone()
# synchronize all processes after vLLM has been fully initialized (if colocated, no need to wait).
if not self.args.vllm_colocation:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move into class

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to move this.. This can be a non-main process of a vllm_coloc False setting where vllm_client is None but should wait. We can leave this if statement here, imo...

Comment thread trl/trainer/grpo_trainer.py Outdated
Comment thread trl/trainer/grpo_trainer.py Outdated
name = name.replace("modules_to_save.default.", "")

if self.accelerator.is_main_process:
if self.accelerator.is_main_process or self.args.vllm_colocation:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move into class

Comment thread trl/trainer/grpo_trainer.py Outdated
for name, param in self.model.named_parameters():
with gather_if_zero3([param]):
if self.accelerator.is_main_process:
if self.accelerator.is_main_process or self.args.vllm_colocation:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move into class

Comment thread trl/trainer/grpo_trainer.py Outdated
# Reset cache on main process
if self.accelerator.is_main_process:
# Reset cache on main process (if colocated, reset cache on all vllms)
if self.accelerator.is_main_process or self.args.vllm_colocation:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move into class

Comment thread trl/trainer/grpo_trainer.py Outdated
guided_decoding_regex=self.guided_decoding_regex,
)
# prompt individually (if colocated, work on your own batch).
ordered_set_of_prompts = all_prompts_text if self.args.vllm_colocation else all_prompts_text[:: self.num_generations]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move into calss

@fabianlim fabianlim left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alot of "move into class" comments, but try to be creative on how to reduce code verbage, rather than just doing it by copying and pasting

@toslali-ibm

Copy link
Copy Markdown
Owner Author

@fabianlim

I think moving if/else logics to the class (vllm clients) may not be really good. Because, if noncolocated and not main process, vllm client will be empty/None... So those checks have to be made in trainer as we cannot do it in vllmclient of none.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants