[WIP] Blenderbot
Author: mariamabarham
Pull request: yes

**UPDATES - 14 AUGUST 2020**

- Blenderbot-3B is working exactly in the same way as parlai and they have the same generation outputs

- Blenderbot-90M also generates the same output as parlai in most cases but it can sometime generate a sequence with a small difference.
For example:

Parlai: `i' m not sure . i just feel like i ' m going to throw up . `
hf: `i don ' t know . i just feel like i ' m going to throw up .`
Discrepancy could be from length penalty or some other beam search param.


**Update Sep 17**

@sshleifer  taking over

### TODO:
- check eos generation
- test distilled 2b model
- `AutoTokenizer`/`AutoModelForSeq2SeqLM` coverage
- debug failing 3b integration test
- implement backwards compatibility for the variant change by checking `config.model_type`
- document `layernorm_variant` nicely or pursue alternative solution.

Comments:
Awesome, looks like the model is soon complete :-) 

A couple of things I think that could be improved a bit:

1. More consistent naming with other models. For me personally, I try to write the code as similar as possible to the `bert_modeling.py` code. `input_tensor` => `hidden_states`, `incremental_state` => `past_key_value_state`, ...
2. More modularization. IMO, it's always good to have many independent classes in a model. `BlenderbotEmbeddings`, `BlenderbotEncoder`, `BlenderbotPoolingLayer`, ...Even if the forward function of these classes only has a couple of lines, it's more readable for the user and also gives you much more flexibility when you want to apply changes to the model later. I would also take a look at the BertModel for this and try to make it as similar as possible
3. Usually, the config is just passed down to the layers instead of writing out all the needed params. This has 2 advantages. 1) Less code 2) No need to set the params to default parameters in the funciton arguments that could confuse the user
4. Make the model as minimal as possible, especially as possible. What I mean by this is that the forward passes of the model should only do what there are supposed to do and we should try to avoid adding any functions that do things under the hood. For example cutting the hidden_states to its last state when using the cache (I have done the same thing in GPT2 and after discussing with @thomwolf it's quite clear now that this can lead to problems as shown in this issue: https://github.com/huggingface/transformers/issues/4368#issuecomment-630244541). This also concers any special function (-inf setting of certain tokens), which should be handled by `generate()` as it's done for Bart. 

Overall, I think the design as it is now fits well with the EncoderDecoder design! It looks to be very similar to Bart (pining @sshleifer here, maybe you can take a look as well). So I think you should just focus on one single forward() pass here and the incremental generation will be handled by the `generate()` method. 

Just a note from my side.
I'm 100% on board with inheriting from module classes, like `BartEncoder` and `BartDecoder` if they are 1-to-1 the same and only the naming has to be changed. 

On the other hand, I'm not 100% on board with inheriting from a class and then overwriting specific functionality that is different. I guess in the case of `RobertaTokenizer`, with this tokenizer inheriting from `GPT2Tokenizer`, it's alright because it follows already existing logic in the library, but I'm not really a fan of it. IMO, inheritance should only be done if the functionality is _1-to-1 the same_ and not if only parts of the functionality are the same. I very much like the "Composition over inheritance" principle: https://en.wikipedia.org/wiki/Composition_over_inheritance . Also, I don't really mind copy-pasting code to some degree if it gives a clear gain in flexibility, which for me is probably the most important factor to consider in fast-changing research code.

For this model, I think it's great if we can reuse `BartEncoder` and `BartDecoder`, but should not abstract at a too high level if the models are just not the same (as was  done in Longformer and which I want to change soon). 


@sshleifer I guess we have very different opinions on this case :D 

Also, pinging @thomwolf and @LysandreJik to hear their opinion on this 

@patrickvonplaten I think we are completely aligned on the specifics. We can talk about the principles at a bar someday, but your 100% rule would have us delete `PretrainedModel` :)

I just pushed a not particularly clean but working version for blenderbot-90M as shown by `test_samgen`. 

There are still a few known issues, most importantly:
- The model does not generate EOS Token at the end of generations. 
- I haven't tested the 3B model. Test is very slow (like 10 mins) on CPU. It should only run on GPU.
- The tokenizers don't work (at least on my machine)
- Our length_penalty implem is [different](https://github.com/facebookresearch/ParlAI/blob/22d75cbfdcf4c093b2e2c660656b65aba77bd802/parlai/core/torch_generator_agent.py#L1474)  than blenderbot. We need to do the math to figure out the right number. 
- Bart Change: If we decide to  use `config.variant` to decide the layernorm order, a practice that I took from [`parlai`](https://github.com/facebookresearch/ParlAI/blob/a20ea268f9b5ef930b97ba5c608b050f7ee63627/parlai/agents/transformer/modules.py#L445) I need to update configs and raise a DeprecationWarning for `config.normalize_before`. I can also check/fix the configs on the model zoo. My opinion are that both ways of supporting such a small difference between the variants are annoying, but this is the least annoying way to support the different order of layernorm operations for variant=='xlm' (which blenderbot-90B uses), and we already had to do it with `mbart/config.normalize_before`. I'd be happy to write a doc explaining the settings. We can also delete `aiayn` which we don't use. I also just copied the naming. It should probably be changed.

Can test 3B tomorrow. I'm fine with whatever other people want to do stylistically.

> I just pushed a not particularly clean but working version for blenderbot-90M as shown by `test_samgen`.
> 
> There are still a few known issues, most importantly:
> 
> * The model does not generate EOS Token at the end of generations.
> * I haven't tested the 3B model. Test is very slow (like 10 mins) on CPU. It should only run on GPU.
> * The tokenizers don't work (at least on my machine)
> * Our length_penalty implem is [different](https://github.com/facebookresearch/ParlAI/blob/22d75cbfdcf4c093b2e2c660656b65aba77bd802/parlai/core/torch_generator_agent.py#L1474)  than blenderbot. We need to do the math to figure out the right number.
> * Bart Change: If we decide to  use `config.variant` to decide the layernorm order, a practice that I took from [`parlai`](https://github.com/facebookresearch/ParlAI/blob/a20ea268f9b5ef930b97ba5c608b050f7ee63627/parlai/agents/transformer/modules.py#L445) I need to update configs and raise a DeprecationWarning for `config.normalize_before`. I can also check/fix the configs on the model zoo. My opinion are that both ways of supporting such a small difference between the variants are annoying, but this is the least annoying way to support the different order of layernorm operations for variant=='xlm' (which blenderbot-90B uses), and we already had to do it with `mbart/config.normalize_before`. I'd be happy to write a doc explaining the settings. We can also delete `aiayn` which we don't use. I also just copied the naming. It should probably be changed.
> 
> Can test 3B tomorrow. I'm fine with whatever other people want to do stylistically.

For the 90M tokenizer I pushed a working test here: https://github.com/huggingface/transformers/pull/4803/commits/724dc8798187801f382082bf32ba6025d15426de

Updates:

- both tokenizers (for 3B and 90M model)
-  `modeling_bart` when `variant==prelayernorm`
-  `special_tokens_map.json` file to replace `"sep_token": "
"`  by `"sep_token": "__end__"
-  `pytorch_model.bin` 

1. 3B model is working perfectly and generation output is the same as parlai
2. 90M model is working in some case but sometime it's generation output is a bit different to parlai for example:

- parlai output: `__start__ i ' m not sure . i just feel like i ' m going to throw up .`
- blenderbot output: `__start__ i don ' t know . i just feel like i ' m going to throw up .`

Not solved yet:

- Both models do not generate `eos_token`

### LayerNorm Variant Problem
The problem with `layernorm_variant` is that the two blenderbot checkpoints have layernorm in **different** places. `bbot-90m.config.layernorm_variant='xlm'`, whereas `bbot-3b.config.layernorm_variant=prelayernorm` so one way I can think to do it without an if statement is separate `Blenderbot90Model` and `Blenderbot3BModel` which is inconsistent with the rest of the repo, where individual checkpoints do not have separate model classes. We would then also need two configs and two model types and probably two of some other things.

### Possible Solutions
+ Write a doc and markdown table containing: what is each layernorm variant+which models use it, the link to that doc both in the config/code and also from model cards.
+ Don't port bbot-90m. If we don't port bbot-90m, we don't need to add config.layernorm_variant -- bbot3b layers are identical to mbart layers. The issue here is that bbot-3b **barely** runs inference on 1 GPU with bs=1.
+ separate `Blenderbot90Model` and `Blenderbot3BModel`.
+ There are also solutions where we parametrize out EncoderLayer/DecoderLayer, but these seem more confusing/harder to understand/less consistent to me. 

I am very open to suggestions, and if I don't get any I will keep working on trying to get the forward pass into one file, as @thomwolf wrote in slack today.



Moving here for cleaner history: https://github.com/huggingface/transformers/pull/7418