New option called `"best"` for `args.save_strategy`.
Author: seanswyi
Pull request: yes
Labels: name: trainer

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->

Addresses https://github.com/huggingface/transformers/issues/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).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#create-a-pull-request),
      Pull Request section?
- [x] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [x] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

@muellerzr @SunMarc 

<!-- Your PR will be replied to more quickly if you can figure out the right person to tag with @

 If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of **who to tag**.
 Please tag fewer than 3 people.

Models:

- text models: @ArthurZucker
- vision models: @amyeroberts
- speech models: @sanchit-gandhi
- graph models: @clefourrier

Library:

- flax: @sanchit-gandhi
- generate: @zucchini-nlp (visual-language models) or @gante (all others)
- pipelines: @Narsil
- tensorflow: @gante and @Rocketknight1
- tokenizers: @ArthurZucker
- trainer: @muellerzr and @SunMarc

Integrations:

- deepspeed: HF Trainer/Accelerate: @muellerzr
- ray/raytune: @richardliaw, @amogkam
- Big Model Inference: @SunMarc
- quantization (bitsandbytes, autogpt): @SunMarc 

Documentation: @stevhliu and @MKhalusova

HF projects:

- accelerate: [different repo](https://github.com/huggingface/accelerate)
- datasets: [different repo](https://github.com/huggingface/datasets)
- diffusers: [different repo](https://github.com/huggingface/diffusers)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Maintained examples (not research project or legacy):

- Flax: @sanchit-gandhi
- PyTorch: See Models above and tag the person corresponding to the modality of the example.
- TensorFlow: @Rocketknight1

 -->


Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_31817). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Gentle ping @muellerzr @SunMarc 

> While this may seem hacky to you, I think this perfectly expands our given system. Any chance you could add a few tests to this? (Over in `test_trainer.py`). Very nice work!

Thanks! And sure, I think I'll have some time to work on it over the weekend.

@muellerzr (cc. @SunMarc)

Hello. I've implemented a new test case inside of `tests/trainer/test_trainer.py` as suggested. Patches were used to control the output of the `_evaluate` method.

The first test case is when a value for `metric_for_best_model` is explicitly provided, and the second is when it's not provided and therefore defaults to the loss. Each case should have 2 and 3 checkpoints saved, respectively, as per the `side_effect`s.

I've changed the DefaultFlowCallback object so that if `save_strategy` is either `"no"` or `"best"` then a checkpoint will not be saved at the end of training, as is the default behavior; it didn't really make sense to me that we'd want to keep only the best checkpoint but the last checkpoint is also being saved.

Please let me know if there are any problems or additional changes that I should make!

---

After inspecting the tests it seems like I wasn't supposed to manually alter the training arguments. The most recent two commits undid those.

@muellerzr @SunMarc No rush, but just a gentle nudge/reminder! Thanks.

@muellerzr Hi. Just wondering if this PR is still relevant or not. Thanks.

@ArthurZucker @SunMarc @muellerzr Sorry to keep pinging you guys on this, but I'm noticing that the tests seem to be possibly stuck after the merge. Could anybody check this when they have the chance? Thanks.

https://github.com/huggingface/transformers/runs/32165247051

Indeed, thanks for noticing @seanswyi but don't worry, I check the most recent [CI](https://github.com/huggingface/transformers/actions/runs/11657144637/job/32454510816) and the test is passing. Can you cancel the run @ArthurZucker ? Or it will just cancel by itself after a few days ? 

> Indeed, thanks for noticing @seanswyi but don't worry, I check the most recent [CI](https://github.com/huggingface/transformers/actions/runs/11657144637/job/32454510816) and the test is passing. Can you cancel the run @ArthurZucker ? Or it will just cancel by itself after a few days ?

Ah, got it. Thanks for following up!

If `load_best_model_at_end = True`, the strategy cannot be set to `best`.

This solves https://github.com/huggingface/transformers/issues/35070, cheers! 🥳