Replace error by warning when loading an architecture in another
Author: sgugger
Pull request: yes

# What does this PR do?

#10586 introduced a breaking change by mistake by removing the possibility to do something like:
```
from transformers import BertGenerationEncoder
model = BertGenerationEncoder.from_pretrained("bert-large-uncased", bos_token_id=101, eos_token_id=102)
```
which is perfectly acceptable and [documented](https://huggingface.co/transformers/model_doc/bertgeneration.html?highlight=bertgeneration)

This PR reverts the hard error and replaces it with a warning.

Fixes #11184

Comments:
Would you consider this solution instead?

```
--- a/src/transformers/models/bert_generation/configuration_bert_generation.py
+++ b/src/transformers/models/bert_generation/configuration_bert_generation.py
@@ -78,7 +78,7 @@ class BertGenerationConfig(PretrainedConfig):
         >>> # Accessing the model configuration
         >>> configuration = model.config
     """
-    model_type = "bert-generation"
+    model_type = "bert"

     def __init__(
         self,
```

So it's adjusted to say that it's actually a `bert`-type of model that it works with. And if some model can work with 5 types or any, it could list those 5 types or have `any`.

perhaps we need to split this field - one of them should say the type of model it works with and keep this one that is just a unique identifier of the sub-type of the model.

So e.g. `model.config.works_with = "(type1|...|typeN|any)"`


Then the all the calls to `from_pretrained` with models that have a `model_type` of `bert-generation` will fail (and there are several checkpoints on the hub with that model type). I also don't think this is the only instance where it's possible to load some model's weights in another model, it's just the first one that is reported.

see my edits - perhaps we need a specific config entry that lists all the model types the model can work with?

So that field doesn't control too many functions?

But why would we actively prevent a user to load some weights in another model if that doesn't cause any error? Of course they would not work as is, but perhaps it could be a strategy of transfer learning.

Load the model weights with the model class it was pre-trained with, then do whatever you want with those weights - copy them into the new model, etc. Nothing stops the user here from using those weights. i.e. unless I'm missing something here we aren't preventing anything. 

I just fail to see how loading weights in a model class that is totally different can be of any direct use, even for transfer learning. If you can see such ways do you have an example?

That's said if you strongly feel that the enforcement of the match is not logical, then I'm totally fine with the proposed change.

I don't have strong opinions but the change made was breaking for existing use-cases. I have no way to know which other use cases have been broken by it too, so leaving the warning makes the most sense to me to avoid having to do a new patch release in ten days if a user comes with another case of `XxxModel.from_pretrained(yyy_model)` not working anymore.

Let's see what @LysandreJik thinks!



I agree wrt breaking changes. How far are we from v5.0? We could postpone the enforcement until then and use your proposed change until then.

But functionality-wise do you agree that the model type match enforcement would be useful and that it doesn't prevent the user from using the weights from a mismatched model?

The issue with raising an error is that I'm nearly 100% sure we're forgetting some use-cases. Bert Generation is one example, but I really wouldn't be surprised that there exist other niche use-cases and that we're involuntarily blocking those. Printing a warning instead seems safer, and while it's not as visible as an error, a user that doesn't obtain whatever performance they're looking for will look at the warnings and should still understand where the issue is coming from.

> How far are we from v5.0? 

It isn't on the horizon yet. The breaking changes we've wanted to make until now are mostly cosmetic, so there's nothing pushing us to release a breaking release as of now.

LGTM, thanks for taking care of this @sgugger.

That's all said - we ideally should start stirring users towards loading models with the exact classes that created them, and once loaded do whatever is wanted (copy weights, etc.). What is happening now in the edge cases is a misuse of not having a strict verification - it kind of works, so "why not" seems to be the way. If this is done, e.g. by changing the documentation, this issue will just disappear and we can reinstate the assert-check.

I was just thinking about this whole issue of warnings and how they don't quite work. A warning sign on the road is not surrounded by 20 other signs - it stands alone and acts as a warning - loud and clear. A warning in the logs is like a vendor in the bazaar shouting how good his wares are - nobody can hear unless you're right in front of that vendor.

Just 2 days ago my [PR](https://github.com/huggingface/transformers/pull/11168) trying to help with invalid warning, ended up introducing  a bug which I didn't see because it got covered up by yet another warning. The first warning was from incomplete design. And the second warning was covering a real bug.

Warnings should be a last resort and usually indicate that some aspect of the software design isn't fully thought out. IMHO, of course.