PreTrainedModel's tie_weights invocation needs to be configurable
Author: stas00
Pull request: no

`PreTrainedModel` defines `tie_weights` method and then in [one place suggests](https://github.com/huggingface/transformers/blob/master/src/transformers/modeling_utils.py#L512)

> Takes care of tying weights embeddings afterwards if the model class has a :obj:`tie_weights()` method.

But since the super-class has it defined, it's always there. 

So the only way for a sub-class to avoid this "tying" is to override it with:
```
     def tie_weights(self): pass
```

if nothing else happens that comment needs to be edited to suggest a noop override in the sub-class.

But it took some hunting to get there, so a better solution is needed.

Most likely, currently, most (all?) models in transformers with encoder/decoder share token embed weights, hence the issue didn't come up. I'm working on porting a fairseq transformer and there the enc/dec token embeds aren't shared. 

I propose a solution which adds a new param to `PretrainedConfig`, say: `is_enc_dec_sharing_embeds=True` and let the subclass override those, then add at the start of `tie_weights` in `modeling_utils.py`

```
     def tie_weights(self):
         if not self.config.is_enc_dec_sharing_embeds:
             return
```
that way it's easy to quickly become aware that an action needs to be taken and set the desired behavior from within the subclass.

Thoughts?

If the proposed solution is agreeable, please, let me know which config param name it should be `is_enc_dec_sharing_embeds` - or different and I will submit a PR.

Thank you.

**edit:**

OK, having had a closer look:
```
grep -r -A 2  'def tie_weights' src/transformers | grep pass | wc -l
```
we have 5 sub-classes that override it with a no-op so only some rely on the default. Bad superclass, no cookies for you.


Comments:
Hey @stas00 - I think I agree with you here! I think this is not limited to Encoder Decoder models only, so I think a better configuration parameter would be `tie_word_embeddings`. `tie_word_embeddings` could be set to `True` by default and then set to `False` for the respective classes (such as Reformer, ....). What do you think? 

One thing, I'm wondering: For an encoder-decoder model, I think this variable should only apply to the decoder part (and tie its input and output word embeddings) and the encoder embeddings should be set equal to the decoder input embeddings by design in the `modeling_<model_name>.py` file (as it's done in `modeling_t5.py` for example).

Overall, I agree these `def tie_weights(self): pass` are not great. Thanks for opening this issue. @sgugger, @LysandreJik, @sshleifer, @thomwolf - could you add your opinion here as well? 

I think it's okay to control the tying in the init with a new param. For encoder/decoder models, I don't have enough experience with those to know the best default.

> Hey @stas00 - I think I agree with you here! I think this is not limited to Encoder Decoder models only, so I think a better configuration parameter would be `tie_word_embeddings`. `tie_word_embeddings` could be set to `True` by default and then set to `False` for the respective classes (such as Reformer, ....). What do you think?

If you think this is a clear enough that works for me. And yes, `True` by default, since most current classes use it.

And while at it, perhaps, rename the method `tie_weights` to `tie_word_embeddings` to match the config option? the current `tie_weights` method name is not descriptive enough to tell which weights it's about tie, IMHO.

> One thing, I'm wondering: For an encoder-decoder model, I think this variable should only apply to the decoder part (and tie its input and output word embeddings) and the encoder embeddings should be set equal to the decoder input embeddings by design in the `modeling_<model_name>.py` file (as it's done in `modeling_t5.py` for example).

I haven't delved into t5 yet, but the fairseq transformer, unlike most (all?) translators we currently have, has different input and output vocabs, and they are of different sizes, so you can't share the two. If I look at t5 it has shared embeds:
https://github.com/huggingface/transformers/blob/master/src/transformers/modeling_t5.py#L659
I guess it's only the reformer that overrides `tie_weights` at the moment (all 5 matches are in `modeling_reformer`), but it does have a single vocab. 

So we have 2 different issues here:
1. tie in/out weights: yes/no
2. vocabs: shared/not shared

But here we are looking at just issue 1.

I haven't gotten yet to the juicy part yet, just trying to match the pretrained weights to the model and adjusting a copy of BART  to the weights, I will be able to give a more intelligent follow up once I step through the whole process, and have a better understanding of what ties where.


This sounds like a good idea. I would advocate for a `tie_word_embeddings` parameter in the configuration as @patrickvonplaten suggested, but I would keep `tie_weights` as the method that does the weight tying rather than renaming that method as well. Just a quick glance at the configuration tells you which weights it's about to tie, and it will able to handle other cases of weight tying that we might encounter in the future without the need of adding additional new methods.

Awesome, I will open a PR. I actually need this feature for the `EncoderDecoderModel` as well.

Fixed in https://github.com/huggingface/transformers/pull/6692