Fix/best model checkpoint fix

[seanswyi]

What does this PR do?

Fixes #35609 (TL;DR best_model_checkpoint is being set when the checkpoint may not even exist)

• Add a new attribute to TrainerState called best_global_step that keeps track of the step where we performed evaluation and achieved a new best metric.
• Moved setting best_model_checkpoint from _determine_best_metric to _save_checkpoint.
• Inside _save_checkpoint, check if the best_model_checkpoint actually exists, and set it if so.

This was a bit trickier than I thought it would be, since the Trainer's saving and evaluation logic are not tightly coupled as would be in the save_strategy == "best" scenario.

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. Trainer sets state.best_model_checkpoint even when it doesn't save there; leads to training crash #35609
• 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
@tomaarsen

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

[SunMarc]

tests/trainer/test_trainer.py::TrainerIntegrationTest::test_best_model_checkpoint_behavior test you created is failing, can you check ?

[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.

[seanswyi]

tests/trainer/test_trainer.py::TrainerIntegrationTest::test_best_model_checkpoint_behavior test you created is failing, can you check ?

I think the error should be fixed now. Problem was I had hardcoded values and wasn't using the patch correctly. Thanks for pointing it out.

[SunMarc]

Make sure to run make style for the CI. There is also a failing test but that's unrelated to this PR

[seanswyi]

Make sure to run make style for the CI. There is also a failing test but that's unrelated to this PR

Done. I'm not sure why but there was a bit of discrepancy between my local setting and the CI pipeline so I had to make the changes through a Docker image since make style wasn't working.

[SunMarc]

thanks ! i will wait for @tomaarsen to test the PR before reviewing it

[tomaarsen]

I haven't reviewed the code in this PR, but I can confirm that my test passes with this branch again!
cc @SunMarc

• Tom Aarsen

[SunMarc]

Thanks for the PR and sorry for the wait ! Left a few comments !