Skip to content
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: Fixes wrong input type for raw_dtype in ggml to gguf scripts #8928

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

farbodbj
Copy link
Contributor

@farbodbj farbodbj commented Aug 8, 2024

A wrong data type has been passed from the add_tensor and add_tensor_info to this function causing another exception while raising another exception. So I changed the input types based on the name raw_dtype and later converted it to an GGMLQuantizationType object which can also handle invalid arguments itself.
related issue #8929

@github-actions github-actions bot added the python python script changes label Aug 8, 2024
@mofosyne mofosyne added bugfix fixes an issue or bug Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix labels Aug 9, 2024
@compilade
Copy link
Collaborator

compilade commented Aug 10, 2024

Thanks for finding this and fixing it. There has been many refactors lately where the old convert_llama_ggml_to_gguf.py was not tested at all. (mostly because I don't have old GGML models around to test)

But I think the type of the raw_dtype parameter of GGUFWriter.add_tensor and GGUFWriter.add_tensor_info should stay GGMLQuantizationType (because it's easier to figure how to use that parameter if its type is more specific than int), and that convert_llama_ggml_to_gguf.py should be fixed instead.

Would this work?

diff --git a/convert_llama_ggml_to_gguf.py b/convert_llama_ggml_to_gguf.py
index 7b00b439..701df869 100755
--- a/convert_llama_ggml_to_gguf.py
+++ b/convert_llama_ggml_to_gguf.py
@@ -116,7 +116,7 @@ class Tensor:
         assert quant is not None, 'Unknown tensor type'
         (blksize, tysize) = quant
         offset += 12
-        self.dtype= dtype
+        self.dtype= gguf.GGMLQuantizationType(dtype)
         self.dims = struct.unpack(f'<{n_dims}I', data[offset:offset + (4 * n_dims)])
         offset += 4 * n_dims
         self.name = bytes(data[offset:offset + name_len])

@farbodbj farbodbj force-pushed the fix-ggml-to-gguf-script branch from 451e52f to 66a4225 Compare August 10, 2024 18:16
@farbodbj
Copy link
Contributor Author

farbodbj commented Aug 10, 2024

@compilade
You're right using GGMLQuantizationType is kind of more readable and idempotent. I tested your suggested fix (created a patch and applied it), and it fixes this issue. I used your fix and committed the changes again.
There was another problem with this script too, I'll soon create the issue and I'm trying to fix it but I need to get to know the ggml and gguf format more.
Thanks for your review.

@farbodbj
Copy link
Contributor Author

Is the failed CI check required for merging this PR, do I need to do anything about it? as it does not seem to be related to this PR.

@compilade
Copy link
Collaborator

Is the failed CI check required for merging this PR, do I need to do anything about it? as it does not seem to be related to this PR.

You don't need to do anything about it; a fix is pending in #8982, and the source of the problem was identified in #7599 (comment).

@compilade compilade added the merge ready indicates that this may be ready to merge soon and is just holding out in case of objections label Aug 12, 2024
@farbodbj
Copy link
Contributor Author

farbodbj commented Aug 16, 2024

@compilade @ggerganov
Since This PR has been approved and labeled as merge-ready but has had no activity in the past 5 days, it came to me to prevent it from becoming stale I could mention you in the comments to take some action on it.

@ggerganov ggerganov merged commit ee2984b into ggerganov:master Aug 16, 2024
8 of 9 checks passed
@ggerganov
Copy link
Owner

Thanks for the reminder!

arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 15, 2024
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix fixes an issue or bug merge ready indicates that this may be ready to merge soon and is just holding out in case of objections python python script changes Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants