Fix UMT5EncoderModel embedding weights not being tied after loading - #43880
Conversation
| def tie_weights(self, missing_keys=None, recompute_mapping=True): | ||
| # Always tie encoder.embed_tokens to shared, regardless of tie_word_embeddings config | ||
| # This is required because UMT5EncoderModel's architecture expects them to be the same | ||
| self.encoder.embed_tokens.weight = self.shared.weight | ||
|
|
||
| @auto_docstring |
There was a problem hiding this comment.
This needs to be fixed in config file. We have hardcoded tie_word_embeddings=True tho if the config has tie_word_embeddings=False saved then it'll be in kwargs and override our hardcoded value
We need to pop tie_word_embeddings from kwargs if any
There was a problem hiding this comment.
same happens in other T5 family models, can you check them as well?
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
|
Hi @zucchini-nlp . Do you mean I should change the model config.json file like: https://huggingface.co/Wan-AI/Wan2.1-T2V-1.3B-Diffusers/discussions/15 ? I am afraid I can't handle all t5 models cause there are so many models: https://huggingface.co/models?search=t5. Please let me know if I misunderstood it. Thanks! |
|
No, no, you don't need to change the hub config. I meant the file |
|
Hi @zucchini-nlp . If I remove the current change of modeling and change the config file by |
|
No, just pop without using like kwargs.pop("tie_word_embeddings", None)
self.tie_word_embeddings = True # hardcode here for BC |
|
Hi @zucchini-nlp . Thanks for your guide. I've changed it by your comment and verified it works. |
|
Other T5 models also tie weights by default, and we have hardcoded |
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
|
Hi @zucchini-nlp . I also updated other t5 models, please review it. |
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
zucchini-nlp
left a comment
There was a problem hiding this comment.
let's revert Vilt and merge
| # Force tie_word_embeddings to `True` for T5 family | ||
| kwargs.pop("tie_word_embeddings", None) | ||
| super().__init__(**kwargs) |
There was a problem hiding this comment.
vilt doesn't force True, we need to revert this file
|
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. |
ArthurZucker
left a comment
There was a problem hiding this comment.
I feel like we are missing a test no?
|
Yep, a test would be nice as well! |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: mt5, umt5 |
|
Hi @zucchini-nlp . I have reverted the vilt change, and also added tests to make sure the tie_word_embeddings are forced to be True |
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
After the "Refactor weight loading" commit (#41580), UMT5EncoderModel fails to properly tie encoder.embed_tokens.weight to shared.weight when loading checkpoints with tie_word_embeddings=False (e.g., Wan-AI video generation models).
This causes encoder.embed_tokens.weight to remain uninitialized (all zeros), producing garbage output in downstream applications like WanPipeline.
Root Cause
The new tie_weights() in modeling_utils.py skips weight binding when tie_word_embeddings=False. However, UMT5EncoderModel architecturally requires encoder.embed_tokens to share weights with shared, regardless of the config setting.
Solution
Override tie_weights() in UMT5EncoderModel to always bind the embedding weights
Verification
Before fix:
Before fix:
encoder.embed_tokens.weight.sum() = 0.0
shared.weight.sum() = -4456448.0
Same tensor: False
After fix:
Both weights sum to -4456448.0
Same tensor: True
WanPipeline video generation works correctly
The video script see huggingface/diffusers#13105