Fix issue with ratio evaluation steps and auto find batch size
Author: muellerzr
Pull request: yes

# What does this PR do?

Modifies step ratios in the case of when `auto_find_batch_size` is used, otherwise it will still maintain the old ratio step (so if we went from 10% starting at 100 steps, at 1000 steps it would still try and evaluate at step 10 instead of step 100)

Fixes # (issue)

Solves #24248 


## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

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.

@sgugger @amyeroberts 

Conversation of original PR (https://github.com/huggingface/transformers/pull/25390) as had to re-open due to rebase shenanigans. 

Sylvain: 
> Thanks for the fix! There is normally a test that checks the training arguments have not been changed. I'm guessing it didn't kick in with a float value for those ;-)
> Might be worth using a logic that does not change the training arguments and use that test to avoid future regression. In general the training arguments are not supposed to be modified outside of the post init, to allow users to be able to re-use them. So here we should store (in the Trainer state if needed but I think this is all contained to one method?) the logging_steps/eval_steps etc. once converted.

Note: the silent fail on the test will now show up via https://github.com/huggingface/transformers/pull/25435

Amy:

> Thanks for fixing this!
> Just to make sure I've understood correctly before approving - is this right:
>     Previously when using auto_find_batch_size, the evaluation step number would be wrong if one of the step ratios < 1
>     This was because in the first _inner_training_loop call, the args were overridden so that they where absolute rather than relative
>     This meant that in the next call the _inner_training_loop the logic checking for relative values was skipped.
>     The solution is to store the absolute values in the TrainerState rather than modify the trainer arguments
> The part I think I'm missing is why this is triggered in the auto_find_batch_size case

Sylvain:

> I think more code should be migrated to look at the state. Are those the only places we look at logging_steps and co? The state should always be filled, not jsut when the variables are floats.
>  This is triggered by auto_find_batch_size since this decorator calls the training loop several times with different batch sizes. Except that at the second run, the values of  logging_steps and others are wrong since they were modified in the first run, and the number of steps per epoch has changed with the batch size change.

Myself:

> correct, those are the only places. (There are references in the tensorflow class, however I'm unsure if they need the migration or not).
> What other aspects of the trainer should we look for when determining if it should go into the state?

Sylvain:

> Thanks for iterating! We're almost there.
> In terms of design for the Trainer: training arguments should be frozen after post init (exactly for this kind of bug, and there were others in hyperparameter search as well) whereas state contains the thing that can change depending on the training run. Does that make sense?



Comments:
_The documentation is not available anymore as the PR was closed or merged._