Skip to content

Add optimizer_cls_and_kwargs attribute to PPO and RLOO#2302

Merged
qgallouedec merged 3 commits intomainfrom
fix-new-optimizer-arg
Oct 31, 2024
Merged

Add optimizer_cls_and_kwargs attribute to PPO and RLOO#2302
qgallouedec merged 3 commits intomainfrom
fix-new-optimizer-arg

Conversation

@qgallouedec
Copy link
Member

What does this PR do?

After huggingface/transformers#34358, a trainer has a self.optimizer_cls_and_kwargs. However, PPO and RLOO doesn't call super().__init__() in their __init__ method. Consequently, self.optimizer_cls_and_kwargs is not defined, resulting to

$ python examples/scripts/ppo/ppo.py --dataset_name trl-internal-testing/descriptiveness-sentiment-trl-style --dataset_train_split descriptiveness --learning_rate 3e-6 --output_dir models/minimal/ppo --per_device_train_batch_size 4     --gradient_accumulation_steps 1 --total_episodes 10 --model_name_or_path EleutherAI/pythia-14m --missing_eos_penalty 1.0 --save_strategy no --stop_token eos
Traceback (most recent call last):
  File "/fsx/qgallouedec/trl/examples/scripts/ppo/ppo.py", line 125, in <module>
    trainer = PPOTrainer(
              ^^^^^^^^^^^
  File "/fsx/qgallouedec/trl/trl/trainer/ppo_trainer.py", line 180, in __init__
    self.create_optimizer_and_scheduler(
  File "/fsx/qgallouedec/transformers/src/transformers/trainer.py", line 1138, in create_optimizer_and_scheduler
    self.create_optimizer()
  File "/fsx/qgallouedec/transformers/src/transformers/trainer.py", line 1183, in create_optimizer
    if self.optimizer_cls_and_kwargs is not None:
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'PPOTrainer' object has no attribute 'optimizer_cls_and_kwargs'. Did you mean: 'get_optimizer_cls_and_kwargs'?

This PR fixes our CI "Tests with dev dependencies".

Mentioned here #2288 (comment)

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.

@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.

Copy link
Member

@lewtun lewtun left a comment

Choose a reason for hiding this comment

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

LGTM!

@qgallouedec qgallouedec merged commit d57a181 into main Oct 31, 2024
@qgallouedec qgallouedec deleted the fix-new-optimizer-arg branch October 31, 2024 22:10
yxliu-TAMU pushed a commit to mincheolseong/ECEN743-GRPO-Project-Proposal that referenced this pull request Apr 20, 2025
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