Fix: Don't tie weights when checkpoint has different values - #42612
Fix: Don't tie weights when checkpoint has different values#42612diodiogod wants to merge 1 commit into
Conversation
Fixes a bug where tie_weights() incorrectly ties lm_head to embed_tokens even when the checkpoint contains both weights with different values. This bug was introduced in 4.54.0 and affects models where: - The checkpoint has both lm_head.weight and embed_tokens with different values - The model config has tie_word_embeddings=True - The model uses _tied_weights_keys The issue occurs because tie_weights() is called after loading with missing_keys=None, which skips the safety check that prevents tying when both weights exist in the checkpoint. This fix adds an else branch to check if both params exist with different values when missing_keys=None, and skips tying if they do. Example affected model: Wan2000/Step-Audio-EditX
vasqu
left a comment
There was a problem hiding this comment.
I'm confused why we would need this, doesn't
transformers/src/transformers/modeling_utils.py
Lines 2337 to 2349 in f6331bb
|
hey! we are moving to v5, and now we only tie if the users sets tie_word_embeddings = True. so this should not be needed! |
Fixes critical bug where transformers 4.54+ incorrectly ties lm_head weights to embed_tokens, causing Step Audio EditX to generate text tokens instead of audio tokens (silent/gibberish output). Root cause: - Transformers 4.54+ ties lm_head to embed_tokens even when checkpoint has different values - Step-Audio-EditX has lm_head (norm=227) and embed_tokens (norm=255) as separate weights - Tying overwrites correct lm_head, breaking audio generation Changes: - model_loader.py: Add workaround to restore correct lm_head weights after loading - Detects incorrect weight tying by comparing data pointers - Loads correct lm_head.weight from safetensors - Works with transformers 4.54-4.57+ - Clean up debug code and console spam: - Remove torch_complex warnings (funasr_detach files) - Remove sox import and unused audio functions - Silence FunASR registration tables - Filter torchaudio deprecation warnings Upstream fix: - Submitted PR to transformers: huggingface/transformers#42612 - Workaround will remain for backward compatibility with unfixed versions Tested with transformers 4.57.3 - generates audio tokens correctly.
|
@vasqu Good question! You're right that lines 2337-2349 should handle this case, but only when The issue is that
The check at lines 2337-2349 is inside the Our fix adds the same check for the Without this fix, models with wrong |
|
@ArthurZucker Thanks! Good to know v5 will be more explicit about tying. However, even in v5, if a model config has The issue is:
This fix adds a safety check: "If both weights exist with different values, don't tie them" regardless of config. This prevents silent corruption when model configs are wrong. Even in v5, wouldn't it be better to warn/skip rather than silently break a working model because of a wrong config? The fix is minimal (18 lines) and only affects the edge case where checkpoint explicitly has different values for supposedly-tied weights. |
vasqu
left a comment
There was a problem hiding this comment.
Thanks for the explanation, makes sense to me now. I overlooked the case with init_weights cc @Cyrilvallez because you initially introduced this
Would be cool if we could add a test here with any "corrupted/wrong" model.
| if source_param_check is not None and target_param_check is not None: | ||
| if source_param_check.data_ptr() != target_param_check.data_ptr(): | ||
| logger.warning( | ||
| f"The tied weights mapping for this model specifies to tie {source_param_name} to " | ||
| f"{target_param_name}, but both exist with different values. Skipping tying. " | ||
| "You should update the config with `tie_word_embeddings=False` to silence this warning" | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Would we receive 2 warnings in this case then? Once from the first call with missing keys and once from init weights iiuc - would be nice if we could avoid that
|
Hmm, looking at the CI, it seems this PR breaks quite a few models. Can you check? |
CI is broken so changes seem incompatible atm
|
Sorry but I don't understand. If we are not using Any other later manual (from user) call to |
|
After reflecting on the feedback from @ArthurZucker, @Cyrilvallez, and @vasqu, I've come to realize we were fighting the wrong battle here. The real issue: The Step-Audio-EditX model has a bug in its config - it sets The right fix: The model should update its config to Our solution: We've implemented a workaround in our project (TTS Audio Suite) that detects this specific case and restores the correct weights after loading. This is the appropriate place for such defensive code. Thank you all for your time and thoughtful feedback. I'll close this PR and instead report the bug to the Step-Audio-EditX model maintainers. Apologies for the noise! 🙏 |
What does this PR do?
Fixes a bug introduced in transformers 4.54.0 where
tie_weights()incorrectly tieslm_headtoembed_tokenseven when the checkpoint contains both weights with different values.Root Cause
The bug occurs because:
lm_head.weightandmodel.embed_tokens.weightas separate tensors with different valuestie_word_embeddings=Trueand model code has_tied_weights_keys = ["lm_head.weight"]from_pretrained(), both weights load correctly from checkpointtie_weights()is called withmissing_keys=None(line 4974 in modeling_utils.py)missing_keys is not Nonemissing_keys=None, the check is skipped and weights are tied unconditionallylm_head.weightwithembed_tokens.weightThe Fix
Adds an
elsebranch totie_weights()that checks if both parameters exist with different values whenmissing_keys=None. If they do, it skips tying and logs a warning.Impact
This fixes any model where:
lm_head.weightand embedding weights as separate tensors with different valuestie_word_embeddings=True_tied_weights_keysExample affected model:
Wan2000/Step-Audio-EditX- this model generates wrong output (text tokens instead of audio tokens) in transformers 4.54+ due to this bug.Testing
Tested with Step-Audio-EditX model:
Before fix (transformers 4.54-4.57.3):
After fix:
Backward Compatibility
This fix is fully backward compatible:
Fixes the regression introduced in #39339
cc @ArthurZucker @gante