Repository navigation
fix(models): Add tie_word_embeddings parameter to AyaVisionConfig for proper weight tying - #43455
Conversation
|
[For maintainers] Suggested jobs to run (before merge) run-slow: aya_vision |
|
Just a slight comment on this fix -- I ran this in the venv listed in the issue (NVIDIA L4). And it does produce perfectly comprehensible outputs after the fix, but there are negligible differences between the output shown in my environment and the expected output; I suspect this is due to minor hardware precision issues, so I haven’t added those changes to the PR yet. If those test failures show up when run in this environment as well, I’ll add them, but they no longer produce gibberish so yeah :) Output from my environment (env specifications listed in the issue): |
ArthurZucker
left a comment
There was a problem hiding this comment.
This LGTM, errors are expected from different hardwares
|
@ArthurZucker Thanks for the review! I was wondering if you might also be able to take a look at this PR whenever you have time. It’s a long standing feature request, and it’s been quite some time since I raised a PR for it since September (with a few follow-up pings); I’d really appreciate it, thank you 🙏 |
What does the PR do?
Fixes weight tying between
lm_headandembed_tokensby exposingtie_word_embeddingsat the top-level config, following the canonical repo pattern. Upon inspecting both presets from the AyaVision collection, both presets havetext_config.tie_word_embeddingsset toTrue, but this was not being surfaced on the top-level config.Fixes #43454.
Output:
Accordingly, I’ve added
tie_word_embeddingstoAyaVisionConfig, with its default value set toTrue. As a consequence, the CI failures are also fixed. Run the following command to check the previously failing tests, which should now pass after the weight tying.Before submitting
Pull Request section?
to it if that’s the case.
documentation guidelines, and
here are tips on formatting docstrings.