[modeling] keys to ignore revisited
Author: stas00
Pull request: no

If possible let's please revisit:

1.  `_keys_to_ignore_on_save `
2.   `_keys_to_ignore_on_load_unexpected` 
3.  `_keys_to_ignore_on_load_missing` 

I'm trying to debug a key that refuses to be ignored on load and I'm not sure if I'm not setting it correctly in all those `keys_to_ignore_*` patterns.

-----------

1. should the keys include the model prefix or not? e.g. here it's a mixed bunch:

https://github.com/huggingface/transformers/blob/69233cf03be5fbce0492f3997e139c4d05499e27/src/transformers/models/m2m_100/modeling_m2m_100.py#L1215-L1227

should they all have the `model.` prefix, or all not have it?

2. should we consistently escape the \. or not? Again see the example above for a mixed bunch

I know I was adding non-escaped keys, because there was no ambiguity in things like: `encoder.embed_positions.weights` - do we ever need to escape it? Whatever the decision I ask that we use a consistent way so that when things don't work it's easy to know how it should be written correctly.

3. I'm not very clear about the naming of the last 2 keys, At the point of the model itself it's hard to remember what they mean, and their explanation is really hard to understand. Could the following explanation be revised. I have a hard time parsing this text:

https://github.com/huggingface/transformers/blob/69233cf03be5fbce0492f3997e139c4d05499e27/src/transformers/modeling_utils.py#L726-L731

4. I think the logic of defining which keys not to load is either completely missing or incomplete.

I'm trying to tell m2m_100 not to load `encoder.embed_positions.weights` (and same for decoder), I added it to all 3 keys to ignore and it still loads it, which is invalid since the [model](https://huggingface.co/hf-internal-testing/tiny-random-m2m_100/blob/main/config.json) has these saved and I want to load a model with a different `max_position_embeddings` value and I can't.

```
stderr: RuntimeError: Error(s) in loading state_dict for M2M100ForConditionalGeneration:
stderr:         size mismatch for model.encoder.embed_positions.weights: copying a param with shape torch.Size([22, 16]) from checkpoint, the shape in current model is torch.Size([514, 16]).
stderr:         size mismatch for model.decoder.embed_positions.weights: copying a param with shape torch.Size([22, 16]) from checkpoint, the shape in current model is torch.Size([514, 16]).
```

Either the current logic needs to be further refined or we need a new key `_keys_to_ignore_on_load_always`?

The current logic is here:
https://github.com/huggingface/transformers/blob/69233cf03be5fbce0492f3997e139c4d05499e27/src/transformers/modeling_utils.py#L1921-L1964

It's easy to see how it fails if `set(expected_keys) == set(loaded_keys))` which is the case in this situation:

https://github.com/huggingface/transformers/blob/69233cf03be5fbce0492f3997e139c4d05499e27/src/transformers/modeling_utils.py#L1953-L1954

I think the "bug" is here:

```
        expected_keys = list(model_state_dict.keys())
```
This further needs to be processed to remove `_keys_to_ignore_on_save`, since they are not expected even if they are in the model.

I think the logic is missing and I propose to fix it with this additional chunk (first if):

1. 
```
        if cls._keys_to_ignore_on_save is not None:
            for pat in cls._keys_to_ignore_on_save:
                expected_keys = [k for k in expected_keys if re.search(pat, k) is None]

        missing_keys = list(set(expected_keys) - set(loaded_keys))
        unexpected_keys = list(set(loaded_keys) - set(expected_keys))
```

2. and it never removes the `unexpected_keys` from `state_dict` - so all these still get loaded in `_load_state_dict_into_model` which doesn't get the list of keys to load and loads everything from the `state_dict`

------------

If I piled up too many issues together please let me know and I will split it up, they are just all seem to be interconnected.

Thank you!

@LysandreJik, @sgugger, @patrickvonplaten, @patil-suraj 


Comments:
I'll address point 4 for now. It looks like you are just missing an `ignore_mismatched_sizes=True`. `_keys_to_ignore_on_save` does not impact the loading, as its name indicates, and there is no `_keys_to_ignore_on_load` as if a weight should be ignored on load, it shouldn't be in the checkpoint in the first place.

I'll comment more on points 1 to 3 when I have more time.

Thank you, Sylvain.

1. I can't pass `ignore_mismatched_sizes` since I'm using examples in the tests
2. Unless I'm missing something `ignore_mismatched_sizes` is an incorrect solution for positional encodings since if the size does match it'd still load and use the key and it shouldn't load any of the keys inside `_keys_to_ignore_on_save` - these need to be generated by the model and not overwritten.

I can of course just make a new tiny checkpoint that doesn't have this problem in the first place, but I think it's a good exercise at validating paths that are quite undefined behavior-wise.

I'll let @patil-suraj and @patrickvonplaten give their advice on how to solve this problem with M2M100, but personally very much against any mechanism that will ignore keys in a checkpoint as it's a multitude of bugs waiting to happen. If keys are not supposed to be in a checkpoint, they should just not be inside it.

wrt to point 4 there is no problem with M2M100 per se, it just happened to be one of the tests that is failing since the tiny checkpoint was created in a way that made it somewhat inflexible and a new checkpoint can be made instead.

Regarding your other questions:

1. It depends and that's some tricky logic of the `from_pretrained` method. The prefix will be removed/added in the model state_dict keys when the model you are using expects it or not, depending on whether the checkpoint has it or not. This is to deal with model with heads vs base models and make sure that you can load a checkpoint of a model with head in a base model and vice versa.

2. `_keys_to_ignore_on_load_missing` and `_keys_to_ignore_on_load_unexpected` use re, so the dot should be escaped. Absolutely no problem on my side to have the same for `_keys_to_ignore_on_save` which currently does not use `re`, so should not escape the .

3. `_keys_to_ignore_on_load_missing` -> those are keys that should be removed from the list of missing keys we find (keys inside the model but not in the checkpoint)
`_keys_to_ignore_on_load_unexpected` -> those are keys that should be removed from the list of unexpected keys we find (keys inside the checkpoint but not the model)
Comments should be clearer, I completely agree!


> Regarding your other questions:
> 
> 1. It depends and that's some tricky logic of the `from_pretrained` method. The prefix will be removed/added in the model state_dict keys when the model you are using expects it or not, depending on whether the checkpoint has it or not. This is to deal with model with heads vs base models and make sure that you can load a checkpoint of a model with head in a base model and vice versa.

Yes, and so how does one define the keys to ignore wrt prefix? It sounds like `base_model_prefix` should be excluded. Which means that this is incorrect then:

https://github.com/huggingface/transformers/blob/69233cf03be5fbce0492f3997e139c4d05499e27/src/transformers/models/m2m_100/modeling_m2m_100.py#L1215-L1227

(and the same issue afflicts many other model files with deterministic positional encodings)

> 2. `_keys_to_ignore_on_load_missing` and `_keys_to_ignore_on_load_unexpected` use re, so the dot should be escaped. Absolutely no problem on my side to have the same for `_keys_to_ignore_on_save` which currently does not use `re`, so should not escape the .

Yes, please! Let's make it consistent!

> 3. `_keys_to_ignore_on_load_missing` -> those are keys that should be removed from the list of missing keys we find (keys inside the model but not in the checkpoint)
>        `_keys_to_ignore_on_load_unexpected` -> those are keys that should be removed from the list of unexpected keys we find (keys inside the checkpoint but not the model)
>        Comments should be clearer, I completely agree!

Super. That's a way easier to understand. Thank you! Made a PR here: https://github.com/huggingface/transformers/pull/16741

do we want to resolve this or let it lapse?

On my side, it's just missing the point 2, as you solved point 3. Do you want to make a PR or should I do it?

For point 4, pinging again @patrickvonplaten and @patil-suraj 

> On my side, it's just missing the point 2, as you solved point 3. Do you want to make a PR or should I do it?

so we do we want to escape it or just have it unescaped everywhere? `r'.'` will just match any char, including the actual `.` , and the keys are usually quite unique to fail to match w/o escaping.

IMHO just having the unescaped `.` everywhere is more readable and easier to copy-n-paste/extend/etc.

Agreed!

Thank you! 

OK, I will make a PR then.


hmm, as I started working on it I see that it'd be tricky to make it consistent w/o backslashes, as some keys have regex bits in them as in:

```
src/transformers/models/gptj/modeling_gptj.py:    _keys_to_ignore_on_load_missing = 
[r"h\.\d+\.attn\.masked_bias", r"h\.\d+\.attn\.bias", r"lm_head\.weight"]
```

Not sure then. Thoughts?


Those who have clear regex patterns should be escaped and use \., for the ones that only use strings, I think it's okay to just leave the dot as is.

> Those who have clear regex patterns should be escaped and use ., 

Did you mean to say:

> Those who have clear regex patterns should be escaped and use `\.`,...

? 

or as an example:

```
_keys_to_ignore_on_load_missing = 
[r"h\.\d+\.attn\.masked_bias", r"h\.\d+\.attn\.bias", r"lm_head\.weight"]
```
should remain as is, right?

Yes, sorry about the confusion.

So https://github.com/huggingface/transformers/pull/17722 will resolve item (2).

So point (4) is remaining to be resolved.

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.