From 3b257f88674b005144f20e7bfd8291046018c4fa Mon Sep 17 00:00:00 2001 From: Francis Couture-Harpin Date: Fri, 1 Mar 2024 10:32:38 -0500 Subject: [PATCH 1/4] llama : fix segfault from unknown model arch name --- llama.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/llama.cpp b/llama.cpp index b1db5b1797dc5..042fa36d175b5 100644 --- a/llama.cpp +++ b/llama.cpp @@ -809,6 +809,16 @@ static std::map> LLM_TENSOR_NAMES = static llm_arch llm_arch_from_string(const std::string & name) { for (const auto & kv : LLM_ARCH_NAMES) { // NOLINT + if (kv.second == nullptr) { + // LLM_ARCH_UNKNOWN does not have a name, + // but is somehow still in the LLM_ARCH_NAMES map. + if (kv.first == LLM_ARCH_UNKNOWN) { + // skip string comparison + continue; + } + // known architectures should always have a name + GGML_ASSERT(false && "missing architecture in LLM_ARCH_NAMES"); + } if (kv.second == name) { return kv.first; } From 6cf481b3ac2c2b2bffad5214fcba65b0316ef279 Mon Sep 17 00:00:00 2001 From: Francis Couture-Harpin Date: Fri, 1 Mar 2024 14:26:20 -0500 Subject: [PATCH 2/4] llama : make all LLM maps const This also requires using `std::map::at` instead of its `operator[]` which does not exist for const maps. * llama : name LLM_ARCH_UNKNOWN to "(unknown)" This avoids errors from `std::map::at` when getting the general name of the model architecture. Using "(unknown)" instead of an empty string as per suggestion https://github.com/ggerganov/llama.cpp/pull/5820#issuecomment-1973735284 --- llama.cpp | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/llama.cpp b/llama.cpp index 042fa36d175b5..f39a6ba0ac42a 100644 --- a/llama.cpp +++ b/llama.cpp @@ -215,7 +215,7 @@ enum llm_arch { LLM_ARCH_UNKNOWN, }; -static std::map LLM_ARCH_NAMES = { +static const std::map LLM_ARCH_NAMES = { { LLM_ARCH_LLAMA, "llama" }, { LLM_ARCH_FALCON, "falcon" }, { LLM_ARCH_GPT2, "gpt2" }, @@ -240,6 +240,7 @@ static std::map LLM_ARCH_NAMES = { { LLM_ARCH_MINICPM, "minicpm" }, { LLM_ARCH_GEMMA, "gemma" }, { LLM_ARCH_STARCODER2, "starcoder2" }, + { LLM_ARCH_UNKNOWN, "(unknown)" }, }; enum llm_kv { @@ -300,7 +301,7 @@ enum llm_kv { LLM_KV_TOKENIZER_RWKV, }; -static std::map LLM_KV_NAMES = { +static const std::map LLM_KV_NAMES = { { LLM_KV_GENERAL_ARCHITECTURE, "general.architecture" }, { LLM_KV_GENERAL_QUANTIZATION_VERSION, "general.quantization_version" }, { LLM_KV_GENERAL_ALIGNMENT, "general.alignment" }, @@ -364,7 +365,7 @@ struct LLM_KV { llm_arch arch; std::string operator()(llm_kv kv) const { - return ::format(LLM_KV_NAMES[kv], LLM_ARCH_NAMES[arch]); + return ::format(LLM_KV_NAMES.at(kv), LLM_ARCH_NAMES.at(arch)); } }; @@ -399,7 +400,7 @@ enum llm_tensor { LLM_TENSOR_LAYER_OUT_NORM, }; -static std::map> LLM_TENSOR_NAMES = { +static const std::map> LLM_TENSOR_NAMES = { { LLM_ARCH_LLAMA, { @@ -810,13 +811,6 @@ static std::map> LLM_TENSOR_NAMES = static llm_arch llm_arch_from_string(const std::string & name) { for (const auto & kv : LLM_ARCH_NAMES) { // NOLINT if (kv.second == nullptr) { - // LLM_ARCH_UNKNOWN does not have a name, - // but is somehow still in the LLM_ARCH_NAMES map. - if (kv.first == LLM_ARCH_UNKNOWN) { - // skip string comparison - continue; - } - // known architectures should always have a name GGML_ASSERT(false && "missing architecture in LLM_ARCH_NAMES"); } if (kv.second == name) { @@ -842,38 +836,38 @@ struct LLM_TN { llm_arch arch; std::string operator()(llm_tensor tensor) const { - if (LLM_TENSOR_NAMES[arch].find(tensor) == LLM_TENSOR_NAMES[arch].end()) { + if (LLM_TENSOR_NAMES.at(arch).find(tensor) == LLM_TENSOR_NAMES.at(arch).end()) { return "__missing__"; } - return LLM_TENSOR_NAMES[arch].at(tensor); + return LLM_TENSOR_NAMES.at(arch).at(tensor); } std::string operator()(llm_tensor tensor, const std::string & suffix) const { - if (LLM_TENSOR_NAMES[arch].find(tensor) == LLM_TENSOR_NAMES[arch].end()) { + if (LLM_TENSOR_NAMES.at(arch).find(tensor) == LLM_TENSOR_NAMES.at(arch).end()) { return "__missing__"; } - return LLM_TENSOR_NAMES[arch].at(tensor) + "." + suffix; + return LLM_TENSOR_NAMES.at(arch).at(tensor) + "." + suffix; } std::string operator()(llm_tensor tensor, int bid) const { - if (LLM_TENSOR_NAMES[arch].find(tensor) == LLM_TENSOR_NAMES[arch].end()) { + if (LLM_TENSOR_NAMES.at(arch).find(tensor) == LLM_TENSOR_NAMES.at(arch).end()) { return "__missing__"; } - return ::format(LLM_TENSOR_NAMES[arch].at(tensor).c_str(), bid); + return ::format(LLM_TENSOR_NAMES.at(arch).at(tensor).c_str(), bid); } std::string operator()(llm_tensor tensor, const std::string & suffix, int bid) const { - if (LLM_TENSOR_NAMES[arch].find(tensor) == LLM_TENSOR_NAMES[arch].end()) { + if (LLM_TENSOR_NAMES.at(arch).find(tensor) == LLM_TENSOR_NAMES.at(arch).end()) { return "__missing__"; } - return ::format(LLM_TENSOR_NAMES[arch].at(tensor).c_str(), bid) + "." + suffix; + return ::format(LLM_TENSOR_NAMES.at(arch).at(tensor).c_str(), bid) + "." + suffix; } std::string operator()(llm_tensor tensor, const std::string & suffix, int bid, int xid) const { - if (LLM_TENSOR_NAMES[arch].find(tensor) == LLM_TENSOR_NAMES[arch].end()) { + if (LLM_TENSOR_NAMES.at(arch).find(tensor) == LLM_TENSOR_NAMES.at(arch).end()) { return "__missing__"; } - return ::format(LLM_TENSOR_NAMES[arch].at(tensor).c_str(), bid, xid) + "." + suffix; + return ::format(LLM_TENSOR_NAMES.at(arch).at(tensor).c_str(), bid, xid) + "." + suffix; } }; @@ -881,7 +875,7 @@ struct LLM_TN { // gguf helpers // -static std::map LLAMA_ROPE_SCALING_TYPES = { +static const std::map LLAMA_ROPE_SCALING_TYPES = { { LLAMA_ROPE_SCALING_TYPE_NONE, "none" }, { LLAMA_ROPE_SCALING_TYPE_LINEAR, "linear" }, { LLAMA_ROPE_SCALING_TYPE_YARN, "yarn" }, From 44e33d4f37ea0f178e8ebec6e63f3db3a5b96167 Mon Sep 17 00:00:00 2001 From: compilade <113953597+compilade@users.noreply.github.com> Date: Fri, 1 Mar 2024 16:21:15 -0500 Subject: [PATCH 3/4] llama : remove redundant inner const for LLM_TENSOR_NAMES The extra const won't do anything here as const maps return const references to values. Co-authored-by: Jared Van Bortel --- llama.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llama.cpp b/llama.cpp index f39a6ba0ac42a..28c035bb6b6f2 100644 --- a/llama.cpp +++ b/llama.cpp @@ -400,7 +400,7 @@ enum llm_tensor { LLM_TENSOR_LAYER_OUT_NORM, }; -static const std::map> LLM_TENSOR_NAMES = { +static const std::map> LLM_TENSOR_NAMES = { { LLM_ARCH_LLAMA, { From d8f7064cb1dcd822c36ca4f0d4fb840f3cff3778 Mon Sep 17 00:00:00 2001 From: Francis Couture-Harpin Date: Sat, 2 Mar 2024 00:31:51 -0500 Subject: [PATCH 4/4] llama : remove redundant nullptr check in llm_arch_from_string Since LLM_ARCH_NAMES is a const map, no spurious elements with a NULL name are inserted anymore, so this check is dead code. --- llama.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/llama.cpp b/llama.cpp index 28c035bb6b6f2..144d19bf79e22 100644 --- a/llama.cpp +++ b/llama.cpp @@ -810,9 +810,6 @@ static const std::map> LLM_TENSOR_NA static llm_arch llm_arch_from_string(const std::string & name) { for (const auto & kv : LLM_ARCH_NAMES) { // NOLINT - if (kv.second == nullptr) { - GGML_ASSERT(false && "missing architecture in LLM_ARCH_NAMES"); - } if (kv.second == name) { return kv.first; }