[Configs] Fix layer type validation to include its mlp counterpart#46220
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Cyrilvallez
left a comment
There was a problem hiding this comment.
Nice thanks! Would be nice to separate mlp and attention in the future though probably!
| elif not all(layer_type in ALLOWED_LAYER_TYPES for layer_type in layers): | ||
| raise ValueError(f"The `(mlp)_layer_types` entries must be in {ALLOWED_LAYER_TYPES} but got {layers}") |
There was a problem hiding this comment.
Let's raise more precise error, does not cost much haha
| elif not all(layer_type in ALLOWED_LAYER_TYPES for layer_type in layers): | |
| raise ValueError(f"The `(mlp)_layer_types` entries must be in {ALLOWED_LAYER_TYPES} but got {layers}") | |
| elif not all(layer_type in ALLOWED_LAYER_TYPES for layer_type in layers): | |
| raise ValueError(f"The `{layer_types}` entries must be in {ALLOWED_LAYER_TYPES} but got {layers}") |
There was a problem hiding this comment.
Done, also fixed the message below
| f"`num_hidden_layers` ({self.num_hidden_layers}) must be equal to the number of layer types " | ||
| f"({len(self.layer_types)})" | ||
| ) | ||
| for layer_types in ["layer_types", "mlp_layer_types"]: |
There was a problem hiding this comment.
mlp layer types have different set of acceptable names, no? I'd prefer to add a separate self.validate_mlp_layer_types in this PR before merging since it's not a huge change
There was a problem hiding this comment.
It used to be different sets but apparently they got merged into one 🤷♂️
There was a problem hiding this comment.
transformers/src/transformers/configuration_utils.py
Lines 62 to 76 in f39b5c8
There was a problem hiding this comment.
Should I still merge or do we want to be breaking here 😓 it's kind of a weird situation
There was a problem hiding this comment.
fine with merging, I assumed there are already non-overlapping sets for two keys. We can create an issue and fix it later for the sake of "correctness"
|
Opened #46245 to keep track, this will be likely breaking |
…uggingface#46220) * fix to include mlp layer types as well * slightly adjust the err msg * specify the message a bit
…uggingface#46220) * fix to include mlp layer types as well * slightly adjust the err msg * specify the message a bit
Slight rewrite to make the validation of layer type to include mlp layer types (2 separate lists - 1 is attn (e.g. full vs swa) 1 is mlp (dense vs sparse)). Probably got lost in the refactor for strict validation