-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix _save_checkpoint
for online methods
#2288
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. |
_save_checkpoint
for online methods
.github/workflows/tests-main.yml
Outdated
@@ -30,7 +30,7 @@ jobs: | |||
python -m pip install --upgrade pip | |||
# install PEFT & transformers from source | |||
pip install -U git+https://github.com/huggingface/peft.git | |||
pip install -U git+https://github.com/huggingface/transformers.git | |||
pip install -U git+https://github.com/huggingface/transformers.git@f339042b0b8bdc0b57a70d37f67cafbea960a2ab |
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.
There is another recent breaking change. I set this commit to make the CI run anyway. I will fix the new breaking change in a followup PR
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.
Not for this PR. But do you think we should add a CI workflow that tests against our lowest supported version of transformers? This may have picked up the issue with the examples being broken on main when processing_class
was added?
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.
Good point. For ref, #2298 aims to make tests clearer/cleaner. I'll extend the work with another PR that implements your suggestion.
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.
pip doesn't have an option to prefer the min version yet pypa/pip#8085
Let's do it manually
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.
There is another recent breaking change. I set this commit to make the CI run anyway. I will fix the new breaking change in a followup PR
This other breaking change is solved here: #2302
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.
add a CI workflow that tests against our lowest supported version of transformers
Done in #2303
If at least one of you @muellerzr @SunMarc can take a look please 🙏 |
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.
Thanks! Sorry that's a bit breaking (but also I suppose somewhat understandable since it's _save_checkpoint
). Fix makes sense to me to deprecate on your own :)
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.
Thanks for the fix!
What does this PR do?
The change in huggingface/transformers#31817 is breaking for TRL since it removes
metrics
from_save_checkpoint
.We need to use the new style, using the new method
Trainer._determine_best_metric
. We need to copy this method fromTrainer
to ensure compatibly.Compatibility
Trainer._determine_best_metric
intoOnlineDPOTrainer
Note that once we've set the minimum transformers version to version > 4.46, we will be able to remove
OnlineDPOTrainer._determine_best_metric
.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines.
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.