Fix Failed tests with mobile bert resize tokens embedding #33950
Conversation
abuelnasr0
changed the title
|
Ah could you update |
|
Ah and |
|
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 I have addressed mobilebert test, GitModeltest, and recurrent_gemma test. recurrent_gemma test failed because an outlier was sampled, so I multiplied covariance by 1e-9 instead of 1e-5. |
|
I feel bad about those tests' failures actually, I wanted to deliver good code but tests didn't help me. 😅 The outlier of recurrent_gemma got sampled only after merging the code haha. And I am not sure if other tests were actually skipped before merging. It's weird. |
|
Yep! They are not part of the important model, the test fetcher seems to badly behave! It should have found out the whole dependencies! No worries, we are the ones who set you up for failure in that case! cc @ydshieh if you can have a look at the reasons why this was not fetched when you have time! |
| config.vocab_size = 4 | ||
| config.pad_token_id = 3 |
There was a problem hiding this comment.
what was this failing?
There was a problem hiding this comment.
The pad_toke_id in the config is 98 for the GitModel. This results in an error in the embedding layer because it's higher than the vocab_size
This error appeared in the GitModel after fixing the error which was caused by overwriting the configuration.
Could one of you provide a link of the previous (PR) job run page where you believe there is something being missed by the test fetcher? |
|
Yes! https://app.circleci.com/pipelines/github/huggingface/transformers/106469/workflows/2ce53b8a-9a18-4e16-9b94-fd3b786c0916/jobs/1414187 is all green, but a few hours later: https://app.circleci.com/pipelines/github/huggingface/transformers/106724/workflows/fb68022e-2d64-44e9-91a5-f86c18f73cdd/jobs/1417162 or any other test torch on main would fail! |
|
In the mean time @abuelnasr0 could you commit with a message |
|
This way all models should be ran! |
A few failing tests still!
|
|
|
|
I hope you don't mind @abuelnasr0 -- this PR is blocking other PRs, so I'm taking care of the rest of the fixes 🤗 |
|
(@ArthurZucker -- all tests in |
|
@gante No problem. That is completely fine. |
|
@gante thanks for fixing it! |
…e#33950) * Fix Failed tests with mobile bert * Cast to the correct dtype * Code fixup * Fix padding_idx larger that embedding_size * Reduce covariance more. use 1e-7 instead of 1e-5 * Comment fix * Reduce covariance more. use 1e-9 instead of 1e-7 * Copy new config * all but MRA fixed * fix mra * very flaky * skip instead * make fixup --------- Co-authored-by: Joao Gante <[email protected]>
Fixes the failures introduced by #33325
The tests failed with mobilebert because of a missing transposing for the
old_lm_head. This PR fixes that. I have tried the two failed tests locally.It's weird that all tests passed before merging. EDIT: I see now, some tests were skipped
I have also changed the logic when the covariance matrix is not positive definite, just initialize the new embeddings with the mean if covariance is not positive definite.
c.c. @ArthurZucker