🚨 Generalize get_decoder() for multimodal and delete redundant code 🔪
#42156
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. |
zucchini-nlp
changed the title
get_decoder() for multimodal and delete redundant code 🔪 get_decoder() for multimodal and delete redundant code 🔪
|
[For maintainers] Suggested jobs to run (before merge) run-slow: aria, autoformer, aya_vision, bart, bigbird_pegasus, blenderbot, blenderbot_small, blip_2, cohere2_vision, conditional_detr, d_fine, dab_detr, deformable_detr, detr, dia, emu3 |
| model = GPT2LMHeadModel(cfg) | ||
| dec = model.get_decoder() | ||
|
|
||
| assert dec is model, f"GPT2 get_decoder() should return self (fallback), got {type(dec)}" |
There was a problem hiding this comment.
prev helper didn't cover all edge cases! This should be the base model, if we compare with other LLMs (e.g. llama)
molbap
left a comment
molbap
left a comment
There was a problem hiding this comment.
Very nice unbloating 🔪
OK for me, just would be cool to add to the make style/ruff rules/quality check to reduce cognitive load
| Symmetric setter. Mirrors the lookup logic used in `get_encoder`. | ||
| """ | ||
|
|
||
| # NOTE: new models need to use existing names for layers if possible, so this list doesn't grow infinitely |
There was a problem hiding this comment.
To note, this should be enforced in make fixup in code consistency part to save ourselves the hassle
There was a problem hiding this comment.
hmm, isn't it going to be a huge limitation for contributors if we force it and auto-renam with fix-copies? Imo we need to communicate it when reviewing and explain why it's important. It's only a few ppl reviewing VLMs currently, so it might be easier
| ): | ||
| # 1. get audio encoder | ||
| encoder = self.get_audio_encoder() | ||
| encoder = self.get_encoder(modality="audio") |
There was a problem hiding this comment.
ah I see now, ok, it's a good thing it's decoupled from the kwargs actually. So, the logic is
- if I don't set the modality: defaults to text encoder
- If I set the modality, checks possible module names
- If I override get_encoder it's whatever I want
Not 100% sure about the modality arg but I see the motivation, understood
There was a problem hiding this comment.
Yep, the idea is to allow users to ask for modality encoder through single interface, instead of having each as self.get_vision_encoder/self.get_audio_encoder etc.
Just added tests to make sure it works
|
Merge conflicts after a big refactor 😢 |
3 participants
What does this PR do?
As per title, blocked by #41589 for VLMs! We should be able to use
get_decoder()to get the LM part of any model after this and have much less duplicate code. Same foes for theget_encoder()to get the encoder if the model has a separate encoding module. In comparison to decoder, we can have specific encoder per modality so the helper will acceptmodalityas argUniversal helper first reduces duplicate code, nudges us to use standardized names for major modules and can be used by 3rd party libraries. Right now we have 5 ways to name a vision encoder!
🚨 Breaking changes, ig we can break helpers for v5:
self.language_modeldirectly from task-model and users will need to callself.get_decoder()get_text_encoderandget_audio_encoderin some audio models because functionality is covered now by theget_encoder()