- Notifications
You must be signed in to change notification settings - Fork14.2k
gguf : general usability improvements#3409
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
cebtenzzre commentedSep 30, 2023
@ggerganov I updated the PR to remove MODEL_TENSOR_NAMES, since it generally makes more sense to use MODEL_TENSORS and TENSOR_NAMES separately. But this is a breaking change. What do you think? |
ggerganov left a comment
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.
Hm, why is it a breaking change? If it is just that python scripts have to switch to usinggguf.TENSOR_NAMES then I think it's fine
| fortensor,keysinself.mappings_cfg.items(): | ||
| tensor_name=tensor_names.get(tensor) | ||
| iftensor_nameisNone: | ||
| iftensornotinMODEL_TENSORS[arch]: |
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.
It's kind of late to say anything, but I'm curious why you'd make these changes. There's no usability improvement from the user's perspective except it'll be twice as slow now since it requires the key to get hashed and the dictionary searched twice compared to usingdict.get().
cebtenzzreOct 2, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I think the code makes more sense this way - the standard name of the tensor is never dependent on the model it is from, so we should represent that fact by using separate data structures. You shouldn't have to know what model architecture you are working with to find the name of a standard tensor, either. I don't think this part of the code is performance-critical, so I didn't bother optimizing it.
This has confused developers at least, which is what I mean by usability. See#3417 (comment)
…example* 'master' of github.com:ggerganov/llama.cpp: (24 commits) convert : fix Baichuan2 models by using vocab size in config.json (ggml-org#3299) readme : add project status link ggml : fix build afterggml-org#3329 llm : add Refact model (ggml-org#3329) sync : ggml (conv 1d + 2d updates, UB fixes) (ggml-org#3468) finetune : readme fix typo (ggml-org#3465) ggml : add RISC-V Vector Support for K-Quants and improved the existing intrinsics (ggml-org#3453) main : consistent prefix/suffix coloring (ggml-org#3425) llama : fix session saving/loading (ggml-org#3400) llama : expose model's rope_freq_scale in the API (ggml-org#3418) metal : alibi for arbitrary number of heads (ggml-org#3426) cmake : make LLAMA_NATIVE flag actually use the instructions supported by the processor (ggml-org#3273) Work on the BPE tokenizer (ggml-org#3252) convert : fix vocab size when not defined in hparams (ggml-org#3421) cmake : increase minimum version for add_link_options (ggml-org#3444) CLBlast: Add broadcast support for matrix multiplication (ggml-org#3402) gguf : add BERT, MPT, and GPT-J arch info (ggml-org#3408) gguf : general usability improvements (ggml-org#3409) cmake : make CUDA flags more similar to the Makefile (ggml-org#3420) finetune :fixggml-org#3404 (ggml-org#3437) ...
Resolves the discussion at#2842 (review)