-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
] Fix DeepSpeed zero-3 issue
#182
Conversation
The documentation is not available anymore as the PR was closed or merged. |
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.
Thank you @younesbelkada for fixing TRL+DS integration. Left comment. Sentiment pipeline related changes have shared offline.
trl/trainer/ppo_trainer.py
Outdated
) = self.accelerator.prepare( | ||
self.model, self.ref_model, self.optimizer, self.data_collator, self.dataloader, self.lr_scheduler | ||
# Safety checkers for DS integration | ||
is_deepspeed_zero_3 = ( |
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.
This changes is irrespective of DS Stage, it should be applied for all DS Stages
Co-authored-by: Sourab Mangrulkar <13534540+pacman100@users.noreply.github.com>
if self.accelerator.state.deepspeed_plugin.zero_stage == 3: | ||
self.model.train() |
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.
Based on the offline discussion I had with @pacman100 , I confirm this hack is needed to make DS3 work
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.
One small comment, otherwise looks good!
What does this PR do?
This is an attempt to fix #171
For now the sentiment script hangs, so I need to investigate