Skip to content

change GGML_MAX_NAME to 128 - #682

Merged
ggerganov merged 2 commits into
ggml-org:masterfrom
leejet:sd.cpp
Jan 10, 2024
Merged

change GGML_MAX_NAME to 128#682
ggerganov merged 2 commits into
ggml-org:masterfrom
leejet:sd.cpp

Conversation

@leejet

@leejet leejet commented Jan 5, 2024

Copy link
Copy Markdown
Contributor

Names of certain tensors, like those in stable-diffusion, exceed a length of 64 characters. I hope the upstream ggml can merge this pr, so I don't have to maintain a separate fork.

@Cyberhan123

Copy link
Copy Markdown
Contributor

@slaren @ggerganov Can we merge this as well?

@slaren
slaren requested a review from ggerganov January 6, 2024 03:21
@slaren

slaren commented Jan 6, 2024

Copy link
Copy Markdown
Member

I hope we can find a better solution in the future, ggml_tensor is becoming absurdly large.

@Cyberhan123

Cyberhan123 commented Jan 6, 2024

Copy link
Copy Markdown
Contributor

I think if I define it in cmake, will it work?
add_definitions(-DGGML_MAX_NAME=128)

@ggerganov ggerganov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe projects using long tensor names should implement their own name shorteners to fit into ggml limit. Can be done with basic string replacements:

// set 
ggml_set_name(t, name_pack("some.very.long.tensor.name"));

// get
name_unpack(ggml_get_name(t));

Or name maps as in llama.cpp:

https://github.com/ggerganov/llama.cpp/blob/90fd43c7eb3d6d69326c26c02fea5cd0c6859a6e/llama.cpp#L369-L425

@leejet

leejet commented Jan 8, 2024

Copy link
Copy Markdown
Contributor Author

I think the best approach might be to place the tensor name and some variable-length fields into dynamically allocated memory.

@ggerganov

Copy link
Copy Markdown
Member

I think the best approach might be to place the tensor name and some variable-length fields into dynamically allocated memory.

Hm, not obvious how to do that.

Are my suggestions above not applicable in sd.cpp?

@leejet

leejet commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

Are my suggestions above not applicable in sd.cpp?

Your suggestion is feasible, but I'm more inclined to increase the tensor name size. This way, I can maintain consistency between the tensor names in sd.cpp and the original stable diffusion code, aiding in code comprehension and debugging.

Or could the code be modified like this? This way, I can independently increase the tensor name size in the sd.cpp code without affecting other repositories using ggml."

#ifndef GGML_MAX_NAME
#define GGML_MAX_NAME           64
#endif

@ggerganov

Copy link
Copy Markdown
Member

Yes - let's do the -DGGML_MAX_NAME thing. Could you update the PR?

@leejet

leejet commented Jan 10, 2024

Copy link
Copy Markdown
Contributor Author

I've updated the PR.

@WhyNotHugo

Copy link
Copy Markdown

When the system libggml is built with -DGGML_MAX_NAME=128, the headers still declare GGML_MAX_NAME=64, so there's a mismatch when other programs link to libggml (e.g.: stable-diffusion.cpp using -DSD_USE_SYSTEM_GGML=ON).

@Cyberhan123

Copy link
Copy Markdown
Contributor

@WhyNotHugo Because this is a compile-time macro substitution, this code will be embedded into the ggml library, therefore you need to recompile ggml.

@WhyNotHugo

Copy link
Copy Markdown

Recompiling won't change anything; libggml is properly compiled with -DGGML_MAX_NAME=128, but its header libraries don't reflect this, and program which link to it have no way of ascertaining the value of GGML_MAX_NAME

@Green-Sky

Green-Sky commented Feb 8, 2026

Copy link
Copy Markdown
Contributor

Recompiling won't change anything; libggml is properly compiled with -DGGML_MAX_NAME=128, but its header libraries don't reflect this, and program which link to it have no way of ascertaining the value of GGML_MAX_NAME

-DGGML_MAX_NAME is not taken into account by the cmake.

@WhyNotHugo

Copy link
Copy Markdown

-DGGML_MAX_NAME is not taken into account by the cmake.

It isn't? Then how do we build libggml with GGML_MAX_NAME=128?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants