8000 Support tie embedding for chatglm models by piDack · Pull Request #13328 · ggml-org/llama.cpp · GitHub
[go: up one dir, main page]
Skip to content

Support tie embedding for chatglm models #13328

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 7, 2025
Merged

Support tie embedding for chatglm models #13328

merged 2 commits into from
May 7, 2025

Conversation

piDack
Copy link
Contributor
@piDack piDack commented May 6, 2025

Make sure to read the contributing guidelines before submitting a PR

The GLM 1.5B model uses tied embeddings, but the chatglm architecture does not take this into account, resulting in a ‘missing tensor ‘output.weight’’ error.this pr is fixed this issue.

CISC
CISC requested changes May 6, 2025
View reviewed changes
@CISC
Copy link
Collaborator
CISC commented May 6, 2025

BTW, I checked both the safetensors and GGUFs, and even though it has tied word embeddings enabled the safetensors have both sets of (most likely identical) weights, and thus the GGUFs also.

Which GGUF file gave you this error?

To make such a GGUF you would have to do something similar to this (which could make sense to incorporate into this PR, but I'd like to know where and why such a GGUF already exists first):

llama.cpp/convert_hf_to_gguf.py

Lines 4309 to 4313 in 764b856

# assuming token_embd.weight is seen before output.weight
if self._tok_embd is not None and new_name == output_name:
if torch.equal(self._tok_embd, data_torch):
logger.debug(f"{output_name} is equivalent to {tok_embd_name}, omitting")
return []

@piDack
Copy link
Contributor Author
piDack commented May 7, 2025

BTW, I checked both the safetensors and GGUFs, and even though it has tied word embeddings enabled the safetensors have both sets of (most likely identical) weights, and thus the GGUFs also.

Which GGUF file gave you this error?

To make such a GGUF you would have to do something similar to this (which could make sense to incorporate into this PR, but I'd like to know where and why such a GGUF already exists first):

llama.cpp/convert_hf_to_gguf.py

Lines 4309 to 4313 in 764b856

# assuming token_embd.weight is seen before output.weight
if self._tok_embd is not None and new_name == output_name:
if torch.equal(self._tok_embd, data_torch):
logger.debug(f"{output_name} is equivalent to {tok_embd_name}, omitting")
return []

I’m sorry, this weight does not come from Hugging Face, but is for my own testing purposes. Indeed, there is no output.weight in the safetensor.
huggingface weight output

image

myself weight output
image

I think adding relevant checks is the right thing to do; at the very least, it can prevent some problems.right？

@piDack @CISC
Update src/llama-model.cpp
5aaf38c 
Format adjustment

Co-authored-by: Sigbjørn Skjæret <sigbjorn.skjaeret@scala.com>
@CISC CISC merged commit 6c7fd67 into ggml-org:master May 7, 2025
46 checks passed
@engineer1109
Copy link

Good Job

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CISC CISC CISC approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@piDack @CISC @engineer1109
0