Skip to content

Adds model kwargs to SFT and DPO trainers#951

Merged
edbeeching merged 5 commits intomainfrom
model-kwargs-argument
Nov 6, 2023
Merged

Adds model kwargs to SFT and DPO trainers#951
edbeeching merged 5 commits intomainfrom
model-kwargs-argument

Conversation

@edbeeching
Copy link
Copy Markdown
Collaborator

  • Adds model as string support to the DPOTrainer.
  • Adds model_kwargs optional args to SFT and DPO trainers.
  • Refactors some of the SFT model instantiation logic as I thought the implementation was overly complicated.

This will greatly simplify the alignment handbook so it would be great to have it in the next trl release.

@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

HuggingFaceDocBuilderDev commented Nov 3, 2023

The documentation is not available anymore as the PR was closed or merged.

@edbeeching edbeeching marked this pull request as ready for review November 3, 2023 11:30
Copy link
Copy Markdown
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.

LGTM ! Can you just add a simple check for users that pass both a valid model and model_kwargs to make sure we throw an appropriate error telling users that passing model_kwargs with non-str model is not supported? 🙏 Thanks!

@edbeeching
Copy link
Copy Markdown
Collaborator Author

@younesbelkada , done. Let me know if you have any other comments

Copy link
Copy Markdown
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Looks generally good to me, just a small nit regarding naming.

Comment thread trl/trainer/dpo_trainer.py Outdated
Copy link
Copy Markdown
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.

Looks great thank you for this @edbeeching !

@edbeeching edbeeching merged commit c273b18 into main Nov 6, 2023
@edbeeching edbeeching deleted the model-kwargs-argument branch November 6, 2023 08:48
lapp0 pushed a commit to lapp0/trl that referenced this pull request May 10, 2024
* adds model kwargs to SFT and DPO trainers

* adds checks for model_kwarg passing when model is not str

* changed warning to ValueError

* renames model_kwargs to model_init_kwargs

* corrects argument names in
yxliu-TAMU pushed a commit to mincheolseong/ECEN743-GRPO-Project-Proposal that referenced this pull request Apr 20, 2025
* adds model kwargs to SFT and DPO trainers

* adds checks for model_kwarg passing when model is not str

* changed warning to ValueError

* renames model_kwargs to model_init_kwargs

* corrects argument names in
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.

4 participants