state.best_metric does not update in EarlyStoppingCallback
Author: Richar-Du
Pull request: no

https://github.com/huggingface/transformers/blob/b18dfd95e1f60ae65a959a7b255fc06522170d1b/src/transformers/trainer_callback.py#L534

I print the `state.best_metric`, but it is always None. I wonder whether the `state.best_metric` should be updated if the condition is satisfied, like the following:
```
if state.best_metric is None or (
            operator(metric_value, state.best_metric)
            and abs(metric_value - state.best_metric) > self.early_stopping_threshold
        ):
            self.early_stopping_patience_counter = 0
            state.best_metric = metric_value
```

Comments:
No, the best metric is updated by the `Trainer` [here](https://github.com/huggingface/transformers/blob/51fa7191b10a13f655d7ab19c7ea10e10078d668/src/transformers/trainer.py#L1758), it's not this callback responsibility to use it.

As mentioned in the [docstring](https://github.com/huggingface/transformers/blob/b18dfd95e1f60ae65a959a7b255fc06522170d1b/src/transformers/trainer_callback.py#L521), are you sure you set `load_best_model_at_end` to `True`?

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.

I have set `load_best_model_at_end` to `True`, but it doesn't work. The following is my code:
```
args_list=[...]
parser = HfArgumentParser((ModelArguments, DataTrainingArguments, Seq2SeqTrainingArguments))
model_args, data_args, training_args = parser.parse_args_into_dataclasses(args_list)
training_args.load_best_model_at_end = True
training_args.metric_for_best_model = 'eval_rouge-l'
training_args.greater_is_better = True
```

However, when I add `control.should_save` in  https://github.com/huggingface/transformers/blob/b18dfd95e1f60ae65a959a7b255fc06522170d1b/src/transformers/trainer_callback.py#L531
it works. Just like this:

```
def check_metric_value(self, args, state, control, metric_value):
      # best_metric is set by code for load_best_model
      operator = np.greater if args.greater_is_better else np.less
      if state.best_metric is None or (
          operator(metric_value, state.best_metric)
          and abs(metric_value - state.best_metric) > self.early_stopping_threshold
      ):
          self.early_stopping_patience_counter = 0
          control.should_save = True
      else:
          self.early_stopping_patience_counter += 1
          control.should_save = False
```

I am facing the same issue (`transformers==4.20.0`), again despite setting `load_best_model_at_end` to `True`. What did the trick for me is also setting the `save_steps` argument in `TrainingArguments` (to a multiplicative of `eval_steps`). This makes sense since [this line in trainer.py](https://github.com/huggingface/transformers/blob/51fa7191b10a13f655d7ab19c7ea10e10078d668/src/transformers/trainer.py#L1635) only gets executed when reaching a step on which the model should be saved.

I wonder though if this is the desired behaviour. Although I agree with @sgugger that the `best_metric` value should be updated in trainer and not in the callback, in the current behaviour it only starts monitoring the early stopping values **after** saving the model for the first time. In my case, it sort of forces me to save model checkpoints just to get the early stopping going.

One solution would be to set an initial `best_metric` value of +/-INF (currently `None`). Wonder what you think/if you agree.



I'm happy to look at a PR :-)

Cool, I'll be working on one!

me too

how to set?

> I have set `load_best_model_at_end` to `True`, but it doesn't work. The following is my code:
> 
> ```
> args_list=[...]
> parser = HfArgumentParser((ModelArguments, DataTrainingArguments, Seq2SeqTrainingArguments))
> model_args, data_args, training_args = parser.parse_args_into_dataclasses(args_list)
> training_args.load_best_model_at_end = True
> training_args.metric_for_best_model = 'eval_rouge-l'
> training_args.greater_is_better = True
> ```
> 
> However, when I add `control.should_save` in
> 
> https://github.com/huggingface/transformers/blob/b18dfd95e1f60ae65a959a7b255fc06522170d1b/src/transformers/trainer_callback.py#L531
> 
> 
> it works. Just like this:
> ```
> def check_metric_value(self, args, state, control, metric_value):
>       # best_metric is set by code for load_best_model
>       operator = np.greater if args.greater_is_better else np.less
>       if state.best_metric is None or (
>           operator(metric_value, state.best_metric)
>           and abs(metric_value - state.best_metric) > self.early_stopping_threshold
>       ):
>           self.early_stopping_patience_counter = 0
>           control.should_save = True
>       else:
>           self.early_stopping_patience_counter += 1
>           control.should_save = False
> ```

not work for me

Has there been any updates on this? I've still found it to be a problem.

I agree with @baskrahmer's analysis - I don't think we should be forced to save model checkpoints just to record the best metric at regular evaluation intervals. Perhaps `Trainer.evaluate()` should be responsible for updatng `self.state.best_metric`?

I am using `transformers==4.44.2`