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

🚀 Supporting deepspeed>=0.16.4's rename #2963

Merged
merged 4 commits into from
Mar 5, 2025

Conversation

jamesbraza
Copy link
Contributor

Fixes #2962

@jamesbraza jamesbraza changed the title Added else clause to avoid NameError on optimizer_offload Supporting deepspeed>=0.16.4's rename Feb 26, 2025
@yeruoforever
Copy link

TypeError: '>=' not supported between instances of 'list' and 'tuple'

@qgallouedec
Copy link
Member

Thanks a lot @jamesbraza

@qgallouedec
Copy link
Member

by the way, is this change needed as well? #2871

@jamesbraza
Copy link
Contributor Author

by the way, is this change needed as well? #2871

Yes, testing it now. Clearly I didn't test this PR previously, as @yeruoforever reported it had a TypeError haha, my bad.

@jamesbraza
Copy link
Contributor Author

Hi @qgallouedec I have completed my validations, this PR is ready for merge. I had to also pull in:

@qgallouedec
Copy link
Member

Thanks, but I can't see the above mentionned changes. Did you pushed them?

@jamesbraza
Copy link
Contributor Author

Thanks, but I can't see the above mentionned changes. Did you pushed them?

The other ones I mentioned weren't related to this PR, so they're not here. They are all here: https://github.com/Future-House/trl/tree/working-grpo-2025-02-27

@qgallouedec
Copy link
Member

Thanks! Just commit the suggestions and we are good to merge.
Usually allowing maintainer to edit the PR makes things easier for us.

@jamesbraza jamesbraza force-pushed the updating-deepspeed branch from f3e48db to a8766fb Compare March 4, 2025 17:50
@jamesbraza
Copy link
Contributor Author

Usually allowing maintainer to edit the PR makes things easier for us.

Yeah I always do, but for some reason that checkbox isn't present in this PR's right panel :/

screenshot of right panel

Regardless, PR is ready for review again

@qgallouedec
Copy link
Member

Thanks!

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@qgallouedec qgallouedec changed the title Supporting deepspeed>=0.16.4's rename 🚀 Supporting deepspeed>=0.16.4's rename Mar 5, 2025
@qgallouedec qgallouedec merged commit e3244d2 into huggingface:main Mar 5, 2025
13 checks passed
@jamesbraza jamesbraza deleted the updating-deepspeed branch March 5, 2025 18:07
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.

DeepSpeedZeRoOffload is incompatible with DeepSpeed>=0.16.4
4 participants