Config: unified logic to retrieve text config #33219
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. |
gante
force-pushed
the
unified_decoder_config
branch
from
9dd336d to
df20b61
Compare
gante
changed the title
gante
force-pushed
the
unified_decoder_config
branch
from
807c582 to
d456788
Compare
gante
force-pushed
the
unified_decoder_config
branch
from
005297c to
5531106
Compare
| if not self.test_resize_embeddings: | ||
| self.skipTest(reason="test_resize_embeddings is set to `False`") |
There was a problem hiding this comment.
(moved the skip here: no point in spending compute if we are going to skip the test)
| @@ -675,14 +675,12 @@ def validate_test_components(test_case, task, model, tokenizer, processor): | |||
| # Avoid `IndexError` in embedding layers | |||
| CONFIG_WITHOUT_VOCAB_SIZE = ["CanineConfig"] | |||
| if tokenizer is not None: | |||
| config_vocab_size = getattr(model.config, "vocab_size", None) | |||
| # Removing `decoder=True` in `get_text_config` can lead to conflicting values e.g. in MusicGen | |||
There was a problem hiding this comment.
Alternatively, we can add a flag to a) return the first valid match; OR b) return all matches when there is more than one match.
| Returns the config that is meant to be used with text IO. On most models, it is the original config instance | ||
| itself. On specific composite models, it is under a set of valid names. | ||
|
|
||
| If `decoder` is set to `True`, then only search for decoder config names. |
There was a problem hiding this comment.
for my own understanding, what does it mean to search in "decoder config names"? Is it somehow related to a model being an encoder-decoder or decoder-only?
From what I see, text_encoder is used in Musicgen only and we never used the decoder=False in transformers
There was a problem hiding this comment.
It is indeed mostly for musicgen at the moment. Needing/wanting to use the encoder text config is uncommon, but there is at least one pair of use cases:
resize_token_embeddings(hasdecoder=False)- The tests for
resize_token_embeddings
| f"Multiple valid text configs were found in the model config: {valid_text_config_names}. " | ||
| "Either don't use `get_text_config`, as it is ambiguous -- access the text configs directly." |
There was a problem hiding this comment.
I don't entirely understand what this comment hints at doing 😅
There was a problem hiding this comment.
Indeed, I've updated the code but left this exception halfway 😅
What does this PR do?
Expected in #32673, #33212
This PR unifies how we access the text config in composite models in a single function, instead of having to look for the right key. This avoids recurrent patterns of
foo if hasattr(...) else bar.[Previously discussed with @amyeroberts here]