AdaFactor: avoid updating group["lr"] attributes
Author: ceshine
Pull request: yes

This affects Adafactor with `relative_step=False` and `scale_parameter=True`.

Updating `group["lr"]` makes the result of ._get_lr() depends on the previous call, i.e., on the scale of other parameters. This isn't supposed to happen.

# What does this PR do?

I've observed weird behaviors when using Adafactor with `relative_step=False` and `scale_parameter=True` and an LR scheduler. I think the problem is that the code [updates the `lr` attribute of the current parameter group](https://github.com/huggingface/transformers/blob/490b39e6142ca8f2ccb84c5436402899ae54e44f/src/transformers/optimization.py#L549), and then uses the updated attribute to [calculate the next attribute](https://github.com/huggingface/transformers/blob/490b39e6142ca8f2ccb84c5436402899ae54e44f/src/transformers/optimization.py#L469). I don't think this is supposed to happen.  

A simple fix would be replacing the update operation with an assignment to a local variable. 

I'm not entirely sure if I understand the problem correctly, so I apologize in advance if this is a stupid PR. I'd appreciate it if someone could point out where I am wrong. Thanks!

## 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/master/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/master/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/master/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

@moscow25  @sshleifer



Comments:
Can you provide evidence that supports the following:

> Updating group["lr"] makes the result of ._get_lr() depends on the previous call, i.e., on the scale of other parameters. **This isn't supposed to happen.**

Thanks!




> Can you provide evidence that supports the following:
> 
> > Updating group["lr"] makes the result of ._get_lr() depends on the previous call, i.e., on the scale of other parameters. **This isn't supposed to happen.**
> 
> Thanks!

Hi, 

Thanks for the quick reply. 

This is taken from the AdaFactor paper:

![fig-1](https://user-images.githubusercontent.com/674501/105513561-6ecfc400-5d0d-11eb-95f0-635e0eb185ea.png)

![fig-2](https://user-images.githubusercontent.com/674501/105513634-860eb180-5d0d-11eb-8e3c-4bccc5fb85d3.png)

As you can see,  ρ only depends on the step number if we use relative steps. And if we switch to any other learning rate schedules (in my case, linear warmup + cosine decay), it doesn't make sense to make the ρ part depends on the scale of the other parameters, nor can I find any reference of this approach in the paper.

If we (loosely) factor the α<sub>t</sub> in the original implementation to α<sub>i,t</sub>, where `i` indicate the set of parameters corresponding to the `for p in group["params"]` loop. The original implementation essentially made α<sub>i,t</sub> depended on α<sub>i-1,t</sub> (i.e., making ρ<sub>i,t</sub> = α<sub>i-1,t</sub>). 


> I've observed weird behaviors when using Adafactor with relative_step=False and scale_parameter=True and an LR scheduler.

I should probably clarify what I meant by "weird behaviors." The model (T5 v1.1) never converged when trained Adafactor with `relative_step=False` and `scale_parameter=True`. After this patch, I managed to get convergence and even better results than the built-in LR schedule in the `relative_step=True` mode (with `warmup_init=True`).

cc @patrickvonplaten @patil-suraj 
This looks like a reasonable change to me!

Thank you all for your time and for accepting the patch! Glad to have made a tiny contribution to this great library.

> BTW, if you have some working code for how to train a `google/t5v1_1` model I think it would be super helpful to post it here, on the forum or as a community notebook! Many people have been asking for good t5v1_1 training scripts :-)

I don't have anything that is sufficiently readable yet. Nonetheless, I have these notebooks published on Kaggle that use the patched Adafactor: one for [T5 v1.1](https://www.kaggle.com/ceshine/preprocess-and-finetune-t5-1-1-full/) and one for [mT5](https://www.kaggle.com/ceshine/preprocess-and-finetune-mt5). They are based on this [Github repo](https://github.com/ceshine/finetuning-t5/tree/mt5-classifier-trim-lm-head/mnli), which is quite messy at this moment. The part that set up the optimizer is located [here](https://github.com/ceshine/finetuning-t5/blob/de34e0c735568d00f9244e0b6f019c3f5cb64576/mnli/train.py#L314).