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

New option called "best" for args.save_strategy. #31817

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

seanswyi
Copy link
Contributor

@seanswyi seanswyi commented Jul 6, 2024

What does this PR do?

Addresses #31626.

Adds a new option called "best" for TrainingArguments.save_strategy which saves the model checkpoint each time a new best performance is achieved.

Details

  1. The previous _save_checkpoint method was in charge of not only saving the model checkpoint but also determining the best metric and best checkpoint. The logic for determining a new best metric was separated out into the _determine_best_metric method.
  2. _determine_best_metric is called after every evaluation inside of _maybe_log_save_evaluate. The return value new_best_metric is used to determine whether or not a new best metric has been achieved, and if the save strategy is "best" then the TrainerControl's should_save flag is switched on.
    • Contrary to what I initially thought, best_metric does not seem to be tracked by default. Rather, it's only tracked when args.metric_for_best_model is provided. I believe that a best metric of some sort should always be tracked, and therefore if a value is not provided then the validation loss is used to determine a new best.
  3. A new object called SaveStrategy was created in trainer_utils that adds a new attribute called BEST to the previous IntervalStrategy.

I'm not sure if I like the rather "hack-y" way that I implemented this by manually switching the TrainerControl's should_save flag rather than delegating it to the callback handler like the other flags are dealt with. The problem is that the flags are normally updated before calling _maybe_log_save_evaluate inside of the inner training loop, which means there's no way for us to determine whether or not a new best metric has been achieved with the current logic. I'm not sure if I'm making sense, but I'm open to any other suggestions.

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 or the forum? 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, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@muellerzr @SunMarc

`save_strategy` previously followed `IntervalStrategy` but now follows
`SaveStrategy`.

Changes were made accordingly to the code and the docstring.
1. Logic to determine the best logic was separated out from
`_save_checkpoint`.
2. In `_maybe_log_save_evaluate`, whether or not a new best metric was
achieved is determined after each evaluation, and if the save strategy
is "best' then the TrainerControl is updated accordingly.
Same as IntervalStrategy, but with a new attribute called BEST.
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants