Skip to content
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

make distributed true for multiple process #997

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

allanj
Copy link
Contributor

@allanj allanj commented Nov 15, 2023

This PR is to fix the issue in #960 (comment)

The issue will cause "gather stat" not to work for other distributed scenarios other than MULTI_GPU.

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I left one comment, what do you think?

distributed should have more than 1 process

Co-authored-by: Younes Belkada <[email protected]>
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@allanj
Copy link
Contributor Author

allanj commented Nov 15, 2023

It's kind of my first time to make PR here. So I guess one more review needed? @younesbelkada

@younesbelkada
Copy link
Contributor

No this is great! Merging!

@younesbelkada younesbelkada merged commit e140d22 into huggingface:main Nov 15, 2023
lapp0 pushed a commit to lapp0/trl that referenced this pull request May 10, 2024
* make distributed true for multiple process

* Update trl/trainer/ppo_trainer.py

distributed should have more than 1 process

Co-authored-by: Younes Belkada <[email protected]>

---------

Co-authored-by: Younes Belkada <[email protected]>
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.

3 participants