Wrong checkpoint got deleted when use_mtime=True
Author: xkszltl
Pull request: no

### System Info

transformers 4.25.1
Ubuntu 22.04 in docker

### Description

We have a job with checkpointing per 200 iters and keeps at most 3 checkpoints.
However, we found in a 1100-iter job, only 400/600/800 was saved at the end.
200 is rotated as expected, but 1000 is missing as well.

Looking into the log, we can see checkpoint-1000 got deleted immediately after saving.
See the last line:
```
 91%|█████████ | 1000/1100 [...][INFO|trainer.py:2693] 2023-10-20 12:24:47,419 >> Saving model checkpoint to output/checkpoint-1000
[INFO|configuration_utils.py:447] 2023-10-20 12:24:49,472 >> Configuration saved in output/checkpoint-1000/config.json
[INFO|modeling_utils.py:1637] 2023-10-20 12:24:55,486 >> Model weights saved in output/checkpoint-1000/pytorch_model.bin
[INFO|tokenization_utils_base.py:2157] 2023-10-20 12:24:55,799 >> tokenizer config file saved in output/checkpoint-1000/tokenizer_config.json
[INFO|tokenization_utils_base.py:2164] 2023-10-20 12:24:56,111 >> Special tokens file saved in output/checkpoint-1000/special_tokens_map.json
[INFO|trainer.py:2771] 2023-10-20 12:28:06,058 >> Deleting older checkpoint [output/checkpoint-1000] due to args.save_total_limit
```

And this line may be the root cause:
- https://github.com/huggingface/transformers/blob/31d452c68b34c2567b62924ee0df40a83cbc52d5/src/transformers/trainer.py#L2267

Our filesystem underneath, for the output dir, is a fuse fs to http blob storage.
So most likely it's not POSIX.
mtime is  probably wrong, or maybe it's simply 0.

Code was first introduced by @sgugger in:
- https://github.com/huggingface/transformers/pull/7431

Fix to this should be straightforward, i.e. set it to False to rely on the numeric part of dir name.
But we'd like to know why it is True currently.

Comments:
cc @pacman100  and @muellerzr 

FYI this kind of issue is what we're seeing.
May not be this specific one because I don't know what we're using underneath, just for the context.
- https://github.com/Azure/azure-storage-fuse/issues/95

Any update?

I got the same errors.
You can override the `_sorted_checkpoints` function and set `use_mtime = False`.

Ping again on this @ArthurZucker @pacman100 @muellerzr @sgugger 

> Fix to this should be straightforward, i.e. set it to False to rely on the numeric part of dir name.
But we'd like to know why it is True currently.

Hello @xkszltl, having gone through the issue, the fix suggested makes sense. A training argument flag for this should provide the required flexibility. We encourage you to raise a PR if you are interested, Thank you!

Sounds good, will send a PR later on.

> A training argument flag for this should provide the required flexibility.

Do we still supposed to keep mtime code path? If so what's the use case?

If adding a cmd-line flag, I would suggest to default to numerical code path, which is a breaking change if whoever actually wants mtime, while defaulting to mtime won't make sense because it introduces a hidden and expensive bug.

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.

Still active

#28364 might not fix it but related

Not stale

cc @muellerzr a gentle ping as I am not sure this was fixed

Just a simple fix, let me send a PR.