-
Notifications
You must be signed in to change notification settings - Fork 417
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
update API to use latest TRL #182
Conversation
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. |
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.
LGTM thanks for the update @kashif!
Left some minor comments, but IMO we should keep some dataclass fields as those override existing defaults in either trl
or transformers
, or just ignore those and don't override the existing defaults. Additionally, I saw that the hub_model_revision
is only available for DPOConfig
so I'd either remove that for DPOConfig
or add it to SFTConfig
and ORPOConfig
too.
Thanks again 🤗
Co-authored-by: Alvaro Bartolome <[email protected]>
Co-authored-by: Alvaro Bartolome <[email protected]>
Co-authored-by: Alvaro Bartolome <[email protected]>
Co-authored-by: Alvaro Bartolome <[email protected]>
@alvarobartt do you know where the |
Co-authored-by: Alvaro Bartolome <[email protected]>
So AFAIK we used it in the past to e.g. push different versions of a fine-tune under diff branches, but probably not widely used, so I'd let @lewtun or anyone else chime in! But I believe we can maybe just go ahead with the current changes and leave that for a separate PR in case we want to keep that, WDYT? 🤗 Thanks again for the PR! |
fixes #180 and updates the API to use updated transformers and TRL versions #178