Skip to content

typing: add rule 14 - checks for tie_word_embeddings presence#44988

Open
tarekziade wants to merge 6 commits intomainfrom
tarekziade-rule-tie-word
Open

typing: add rule 14 - checks for tie_word_embeddings presence#44988
tarekziade wants to merge 6 commits intomainfrom
tarekziade-rule-tie-word

Conversation

@tarekziade
Copy link
Collaborator

What does this PR do?

Adds Rule 14

if _tied_weights_keys is present and non-empty in modeling -> Config MUST contain the tie_word_embeddings field

@tarekziade
Copy link
Collaborator Author

run-slow: ibert

@github-actions
Copy link
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs:

models: ["models/ibert"]
quantizations: []

@HuggingFaceDocBuilderDev

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.

@github-actions
Copy link
Contributor

CI Results

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 2a68caa7 workflow commit (merge commit)
PR bf1e9854 branch commit (from PR)
main 28af8184 base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

@tarekziade tarekziade force-pushed the tarekziade-rule-tie-word branch from bf1e985 to e3c3d20 Compare March 25, 2026 12:34
@tarekziade tarekziade changed the title typing: add rule 14 typing: add rule 14 - checks for tie_word_embeddings presence Mar 25, 2026
Copy link
Member

@Cyrilvallez Cyrilvallez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried it in real world and does not work correctly all the time!
E.g. if _tied_weights_keys are in the main model but tie_word_embeddings in the text config it passes, even though it should not!
Also, would be nice to have which config type should be updated when it correctly finds the missing pattern, instead of saying a geenral: TRF014: AlignVisionModel defines _tied_weights_keys but configuration_align.py does not declare tie_word_embeddings. Add 'tie_word_embeddings: bool = True' to the config class.

Would be super nice to have the reverse as well: if you have tie_word_embeddings in Config -> model using THIS AND ONLY THIS config type should have a _tied_weights_keys, otherwise let's remove tie_word_embeddings

@tarekziade
Copy link
Collaborator Author

Just tried it in real world and does not work correctly all the time! E.g. if _tied_weights_keys are in the main model but tie_word_embeddings in the text config it passes, even though it should not!

ah! that's the sub_configs["text_config"] early exit - will add test cases and fix it, thanks!

Also, would be nice to have which config type should be updated when it correctly finds the missing pattern, instead of saying a geenral: TRF014: AlignVisionModel defines _tied_weights_keys but configuration_align.py does not declare tie_word_embeddings. Add 'tie_word_embeddings: bool = True' to the config class.

That would be a very nice addition indeed I will see how we could templatize error messages.

Would be super nice to have the reverse as well: if you have tie_word_embeddings in Config -> model using THIS AND ONLY THIS config type should have a _tied_weights_keys, otherwise let's remove tie_word_embeddings

👍

@tarekziade tarekziade force-pushed the tarekziade-rule-tie-word branch from 3721886 to 9aa263e Compare March 26, 2026 14:04
@github-actions
Copy link
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: blip, ibert, janus, kosmos2, kosmos2_5, pe_audio_video, perception_lm, qwen3_omni_moe

@tarekziade
Copy link
Collaborator Author

Would be super nice to have the reverse as well: if you have tie_word_embeddings in Config -> model using
THIS AND ONLY THIS config type should have a _tied_weights_keys, otherwise let's remove tie_word_embeddings

let's do this in a separate twin rule, it would be too complex to add in this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants