-
Notifications
You must be signed in to change notification settings - Fork 27.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the incorrect permutation of gguf #31788
Conversation
Thanks for your PR @PenutChen, we really appreciate it! @SunMarc it'd be awesome if you can review this when you have a minute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the deep dive @PenutChen Really good job ! Can you add a few tests since it was not working with a bigger llama ! If possible, we can test this on a q4 model to not take too much memory !
Co-authored-by: Marc Sun <[email protected]>
Co-authored-by: Marc Sun <[email protected]>
@SunMarc Thanks for the advice! I noticed that there is already a Q4 Llama3 test. This update will change the expected text. Should I just update the expected text? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for iterating ! Could you are the perplexity of the model before and after for llama 3 ? If the llama 3 perpelxity was so high, it's a bit strange that it was able to pass the test / generate coherent text
The perplexity of Llama3 Q4 is 1713.8865 before the fix and 6.4626 after the fix. The model comes from NousResearch/Meta-Llama-3-8B-GGUF. They still produce reasonable output if the sequence length is short. This might be because only the q_proj and k_proj weights are abnormal, and their weights are relatively low. Other weights are robust to a certain extent, which could be the cause. This is just my guess. |
Thanks for the confirmation ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed review, thanks for the PR @PenutChen!
* Fix the incorrect permutation of gguf * rename num_kv_heads Co-authored-by: Marc Sun <[email protected]> * add typing to num_kv_heads Co-authored-by: Marc Sun <[email protected]> * rename variables * refactor permute function name * update the expected text of the llama3 q4 test --------- Co-authored-by: Marc Sun <[email protected]>
* Fix the incorrect permutation of gguf * rename num_kv_heads Co-authored-by: Marc Sun <[email protected]> * add typing to num_kv_heads Co-authored-by: Marc Sun <[email protected]> * rename variables * refactor permute function name * update the expected text of the llama3 q4 test --------- Co-authored-by: Marc Sun <[email protected]>
* Fix the incorrect permutation of gguf * rename num_kv_heads Co-authored-by: Marc Sun <[email protected]> * add typing to num_kv_heads Co-authored-by: Marc Sun <[email protected]> * rename variables * refactor permute function name * update the expected text of the llama3 q4 test --------- Co-authored-by: Marc Sun <[email protected]>
* Fix the incorrect permutation of gguf * rename num_kv_heads Co-authored-by: Marc Sun <[email protected]> * add typing to num_kv_heads Co-authored-by: Marc Sun <[email protected]> * rename variables * refactor permute function name * update the expected text of the llama3 q4 test --------- Co-authored-by: Marc Sun <[email protected]>
What does this PR do?
Fixes #31766. The permutation of
q_proj
andk_proj
needs to consider bothnum_attention_heads
andnum_key_value_heads
.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@SunMarc