Trainer not keeping best model checkpoint with save_total_limit=1
Author: erickrf
Pull request: no

## Environment info
<!-- You can run the command `transformers-cli env` and copy-and-paste its output below.
     Don't forget to fill out the missing fields in that output! -->

- `transformers` version: 4.13.0
- Platform: Linux-5.4.109+-x86_64-with-glibc2.10
- Python version: 3.8.8
- PyTorch version (GPU?): 1.10.0+cu113 (True)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using GPU in script?: yes
- Using distributed or parallel set-up in script?: no

### Who can help
<!-- Your issue 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:

- ALBERT, BERT, XLM, DeBERTa, DeBERTa-v2, ELECTRA, MobileBert, SqueezeBert: @LysandreJik
- T5, BART, Marian, Pegasus, EncoderDecoder: @patrickvonplaten
- Blenderbot, MBART: @patil-suraj
- Longformer, Reformer, TransfoXL, XLNet, FNet, BigBird: @patrickvonplaten
- FSMT: @stas00
- Funnel: @sgugger
- GPT-2, GPT: @patrickvonplaten, @LysandreJik
- RAG, DPR: @patrickvonplaten, @lhoestq
- TensorFlow: @Rocketknight1
- JAX/Flax: @patil-suraj
- TAPAS, LayoutLM, LayoutLMv2, LUKE, ViT, BEiT, DEiT, DETR, CANINE: @NielsRogge
- GPT-Neo, GPT-J, CLIP: @patil-suraj
- Wav2Vec2, HuBERT, SpeechEncoderDecoder, UniSpeech, UniSpeechSAT, SEW, SEW-D, Speech2Text: @patrickvonplaten, @anton-l

If the model isn't in the list, ping @LysandreJik who will redirect you to the correct contributor.

Library:

- Benchmarks: @patrickvonplaten
- Deepspeed: @stas00
- Ray/raytune: @richardliaw, @amogkam
- Text generation: @patrickvonplaten @narsil
- Tokenizers: @SaulLu
- Trainer: @sgugger
- Pipelines: @Narsil
- Speech: @patrickvonplaten, @anton-l
- Vision: @NielsRogge, @sgugger

Documentation: @sgugger

Model hub:

- for issues with a model, report at https://discuss.huggingface.co/ and tag the model's creator.

HF projects:

- datasets: [different repo](https://github.com/huggingface/datasets)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Examples:

- maintained examples (not research project or legacy): @sgugger, @patil-suraj

For research projetcs, please ping the contributor directly. For example, on the following projects:

- research_projects/bert-loses-patience: @JetRunner
- research_projects/distillation: @VictorSanh

 -->
@sgugger

## Information

Model I am using (Bert, XLNet ...): BERT

The problem arises when using:
* [ ] the official example scripts: (give details below)
* [x] my own modified scripts: (give details below)

The tasks I am working on is:
* [ ] an official GLUE/SQUaD task: (give the name)
* [x] my own task or dataset: (give details below)

## To reproduce

Steps to reproduce the behavior:

1. Create a trainer with `save_total_limit=2` and `load_best_model_at_end=True`
2. Train the model

After each evaluation, the trainer will save the most recent checkpoint and delete the previous one to keep the save total limit, even if the previous one was better.

That is not what I expected, considering [this comment](https://discuss.huggingface.co/t/save-only-best-model-in-trainer/8442/5?u=erickrf).

<!-- If you have code snippets, error messages, stack traces please provide them here as well.
     Important! Use code tags to correctly format your code. See https://help.github.com/en/github/writing-on-github/creating-and-highlighting-code-blocks#syntax-highlighting
     Do not use screenshots, as they are hard to read and (more importantly) don't allow others to copy-and-paste your code.-->

## Expected behavior

<!-- A clear and concise description of what you would expect to happen. -->
I'd expect the best model to be always kept on disk.

Comments:
I am unsure what behavior you are seeing, but `load_best_model_at_end=True` makes sure the best model checkpoint is always kept. That means the absolute best model checkpoint, so if at step 500, you get a model worse then at step 450, and the best model checkpoint was at step 350, the Trainer will delete the checkpoint at step 450 indeed, and only keep the checkpoint at step 350 for the best model.

I see it now, it was actually my fault. I forgot to provide the `metric_for_best_model` and the trainer was only considering the loss. Sorry for the misunderstanding!

No problem!

I'm still confused by this. I'm not able to use your example of `save_total_limit=2` and `load_best_model_at_end=True`, because it fails with: 
```
ValueError: --load_best_model_at_end requires the save and eval strategy to match, but found
- Evaluation strategy: IntervalStrategy.NO
- Save strategy: IntervalStrategy.STEPS
```
Ideally, I'd like to save the N best checkpoints, but I can't find a way to do that.

I'm on transformers 4.18.0.

UPDATE: Also tried on 4.19.0.dev0

What don't you understand in the error message? As stipulated, you need to have the same evaluation and save strategy when activating `load_best_model_at_end`.

Of course, the error message is clear. What I was confused about is how to find a combination of settings that would do what I was wanting to do. Ideally, I wanted to save the N best checkpoints at a given frequency of steps (e.g., 100 steps). It's helpful, particularly as my model gets closer to converging, to be able to `scp` checkpoints out to my laptop to try in the context of my actual application. And I often do that while the model's still training on my server, so it's nice to have frequent saves where I can quickly find a recent minimal loss.

Anyway, I'm now using the settings suggested here: https://discuss.huggingface.co/t/save-only-best-model-in-trainer/8442/8?u=jbmaxwell, which is fine:
```
save_total_limit = 2
save_strategy = “no”
load_best_model_at_end=False
```
Though honestly it's still quite counterintuitive, as from the settings alone it looks like this will save only the 2 most recent checkpoints, while apparently it will save the most recent and the best.

@sgugger Is it possible update the documentation of the `save_total_limit` parameter of `Trainer`?

What I think would make the documentation better is to document the following behaviour:
- What happens when `load_best_model_at_end` is True, but `save_total_limit=1`?
  - In the documentation this is confusing as `save_total_limit` is mentioning that it keeps the models based recency. Instead perhaps the documentation there should also be updated to reflect that it's behaviour changes depending on other parameters.
- What happens if you select `save_total_limit>=2` and `load_best_model_at_end` is set to True.

Maybe this will help others understand the behaviour straight from the docs. Side note: the docs are generally awesome!

@gitjoop Feel free to open a PR, it would indeed be awesome to have all of this in the doc :-)

I tried running run_mlm.py in examples, with MLFlow Callback but apparently no checkpoint is logged in mlflow.
I used the following config.
```
save_total_limit = 2
save_strategy = “no”
load_best_model_at_end=False
```
and set the HF_MLFLOW_LOG_ARTIFACTS to 1. I can confirm it does log checkpoint but does not log the with the above configuration.