Trainer sets `state.best_model_checkpoint` even when it doesn't save there; leads to training crash
Author: tomaarsen
Pull request: no
Labels: name: bug

### System Info

- `transformers` version: 4.49.0.dev0
- Platform: Windows-10-10.0.22631-SP0
- Python version: 3.9.16
- Huggingface_hub version: 0.24.7
- Safetensors version: 0.4.5
- Accelerate version: 0.34.2
- Accelerate config:    not found
- PyTorch version (GPU?): 2.4.1+cu121 (True)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using distributed or parallel set-up in script?: No
- Using GPU in script?: Yes
- GPU type: NVIDIA GeForce RTX 3090

### Who can help?

@muellerz 
@SunMarc 
@seanswyi

### Information

- [ ] The official example scripts
- [X] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [X] My own task or dataset (give details below)

### Reproduction

`pytest tests/test_model_card.py::test_model_card` from `setfit` (link: https://github.com/huggingface/setfit/blob/main/tests/test_model_card.py#L15)

Apologies for not having a convenient ready-to-go `transformers`-only script. I'm afraid I don't have time for that right now.
In essence, the flow is as follows:
1. I start the trainer, with lots of evaluations (e.g. `eval_steps=1`, `eval_strategy="steps"`)
2. When evaluating, the new `_determine_best_metric` is called: https://github.com/huggingface/transformers/blob/f63829c87bd89a4a0cea45d81c1cd870996b30c4/src/transformers/trainer.py#L3070-L3075
3. With `args.metric_for_best_model` set, we only set the `best_metric` in the first evaluation: https://github.com/huggingface/transformers/blob/f63829c87bd89a4a0cea45d81c1cd870996b30c4/src/transformers/trainer.py#L3182
4. On the 2nd eval, we start comparing against the first. If the model is better, we now also set `best_model_checkpoint`: https://github.com/huggingface/transformers/blob/f63829c87bd89a4a0cea45d81c1cd870996b30c4/src/transformers/trainer.py#L3184-L3192 *but* we're not sure if we're even going to be saving at this step! If `args.save_strategy != SaveStrategy.BEST:`, then it's very possible that we're not saving.
5. The eventual crash occurs when "deleting old checkpoints", because there is no file at `best_model_checkpoint`: https://github.com/huggingface/transformers/blob/f63829c87bd89a4a0cea45d81c1cd870996b30c4/src/transformers/trainer.py#L2680-L2685


### Expected behavior

We should not be setting `best_model_checkpoint` unless we're confident that 1) `state.should_save` is True or 2) `args.save_strategy == "best"`. Then we'll avoid this crash.

- Tom Aarsen

Comments:
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.

This still needs to be fixed. I reckon #35885 is a right direction - it passed my tests.

- Tom Aarsen