Fix tie_word_embeddings not lifted from text_config for some VLM configs (BC regression) - #45857
Open
qgallouedec wants to merge 1 commit into
Open
Fix tie_word_embeddings not lifted from text_config for some VLM configs (BC regression)#45857qgallouedec wants to merge 1 commit into
tie_word_embeddings not lifted from text_config for some VLM configs (BC regression)#45857qgallouedec wants to merge 1 commit into
Conversation
…configs (BC regression)
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: glm4v, glm4v_moe, qwen2_5_vl, qwen2_vl |
|
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. |
Comment on lines
+176
to
183
| # BC: pre-v5 saves placed `tie_word_embeddings` inside text_config. Forward it to the | ||
| # outer config (where v5's tying logic looks) when the root value is the default. | ||
| if not self.tie_word_embeddings and self.text_config.get("tie_word_embeddings"): | ||
| self.tie_word_embeddings = self.text_config["tie_word_embeddings"] | ||
| self.text_config = self.sub_configs["text_config"](**self.text_config) | ||
| elif self.text_config is None: | ||
| self.text_config = self.sub_configs["text_config"](**kwargs) | ||
|
|
Member
There was a problem hiding this comment.
wont work if text config is already a PreTrainedConfig
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For
Qwen2_5_VLConfig,Qwen2VLConfig,Glm4vConfig, andGlm4vMoeConfig, pre-v5 saves placedtie_word_embeddingsinsidetext_config(and dropped it from the root) because the inner text class declared the field. v5 moved the field to the outer config (#42420) but__post_init__doesn't lift the value fromtext_config. The outer attribute therefore falls through to its dataclass default (False),get_expanded_tied_weights_keysreturns{}, no tying happens,lm_head.weightis reported MISSING and randomly initialized — the model loads silently broken.Regression introduced in #41541, which removed the forwarding line
kwargs["tie_word_embeddings"] = self.text_config.tie_word_embeddingsoriginally added in #42420 with aFIXME: tying has to be used from the text configcomment. The vestigialtie_word_embeddings: bool = Falsefield was later removed from the inner text classes in #44976.This patch mirrors the existing fix in
LlavaConfig.__post_init__: whentext_configarrives as a dict (the deserialization path), forwardtie_word_embeddingsto the outer when the root is the default and the dict has a truthy value.Reproducer
Before this PR (loud LOAD REPORT, broken model):
After this PR:
(no LOAD REPORT printed)
The Hub artifact above was saved by transformers 4.57.5; under 4.x its
__post_init__still propagatedtie_word_embeddingsfrom text → root, so the bug is v5-only and only triggers on configs saved by 4.x (the vast majority of pre-existing Hub repos for these architectures).Affected configs
Empirically determined by saving each composite VLM config under transformers 4.56.2 with
tie_word_embeddings=Trueand inspecting the resultingconfig.json:qwen2_5_vl<absent>Trueqwen2_vl<absent>Trueglm4v<absent>Trueglm4v_moe<absent>Truegot_ocr2<absent>TrueTruealreadyllava{,_next,_next_video,_onevision}True<absent>idefics3,smolvlm,llama4,pix2struct,fuyuTrueFiles touched:
qwen2_5_vl,qwen2_vl,glm4v(configuration + modular),glm4v_moe.