Potentially redundant learning rate scheduling
Author: nikitakit
Pull request: no

In the following two code snippets below:

https://github.com/huggingface/pytorch-pretrained-BERT/blob/647c98353090ee411e1ef9016b2a458becfe36f9/examples/run_lm_finetuning.py#L570-L573

https://github.com/huggingface/pytorch-pretrained-BERT/blob/647c98353090ee411e1ef9016b2a458becfe36f9/examples/run_lm_finetuning.py#L611-L613

it appears that learning rate warmup is being done *twice*: once in the example file, and once inside the BertAdam class. Am I reading this wrong? Because I'm pretty sure the BertAdam class performs its own warm-up when initialized with those arguments.

Here is an excerpt from the BertAdam class, where warm-up is also applied:
https://github.com/huggingface/pytorch-pretrained-BERT/blob/647c98353090ee411e1ef9016b2a458becfe36f9/pytorch_pretrained_bert/optimization.py#L146-L150

This also applies to other examples, e.g. 
https://github.com/huggingface/pytorch-pretrained-BERT/blob/647c98353090ee411e1ef9016b2a458becfe36f9/examples/run_squad.py#L848-L851
https://github.com/huggingface/pytorch-pretrained-BERT/blob/647c98353090ee411e1ef9016b2a458becfe36f9/examples/run_squad.py#L909-L911

Comments:
Humm could be the case indeed. What do think about this @tholor?

As far as I can tell this was introduced in c8ea286048517d9072397d77f4de21b8483a4531 as a byproduct of adding float16 support, and was then copied to other example files as well.

I agree, there seems to be double LR scheduling. The applied LR is therefore lower than intended. Quick plot of the LR being set in the outer scope (i.e. in run_squad or run_lm_finetuning) vs. the inner one (in BERTAdam) shows this: 

![lr_schedule_debug](https://user-images.githubusercontent.com/1563902/51321145-49df5100-1a62-11e9-8908-516aaf9362d4.png)

In addition, I have noticed two further parts for potential clean up: 
1. I don't see a reason why the function `warmup_linear()` is implemented in two places: In `optimization.py` and in each example script. 
2. Is the method `optimizer.get_lr()` ever being called? There's actually another LR scheduling.
https://github.com/huggingface/pytorch-pretrained-BERT/blob/f040a43cb3954e14dc47a815de012ac3f87a85d0/pytorch_pretrained_bert/optimization.py#L79-L92

There is als an additional problem that causes the learning rate to not be set correctly in run_classifier.py. I created a pull request for that (and the double warmup problem): #218 

Is there are something done for this double warmup bug?

Yes, @matej-svejda worked on this in https://github.com/huggingface/pytorch-pretrained-BERT/pull/218

I see that, but it isn't merge now?

No, not yet. As you can see in the PR it's still WIP and he committed only 4 hours ago. If you need the fix urgently, you can apply the changes easily locally. It's quite a small fix.

Sorry,I forget to see the time :)

By the way, how can I draw a picture about the LR schedule about BERT like yours. I see if use `print(optimizer.param_groups['lr'] `, the learning rate is always like I init it.

I have plotted `optimizer.param_groups[0]["lr"]` from here: 
https://github.com/huggingface/pytorch-pretrained-BERT/blob/f040a43cb3954e14dc47a815de012ac3f87a85d0/examples/run_lm_finetuning.py#L610-L616

and `lr_scheduled` from here: 
https://github.com/huggingface/pytorch-pretrained-BERT/blob/f040a43cb3954e14dc47a815de012ac3f87a85d0/pytorch_pretrained_bert/optimization.py#L145-L152

Your above could should actually throw an exception because `optimizer.param_groups` is a list. Try  `optimizer.param_groups[0]["lr"]` or `lr_this_step`.

Ok this should be fixed in master now!