-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
ggml : move rope type enum to ggml.h #8949
Conversation
This commit moves the `llama_rope_type` enum from `llama.h` to `ggml.h` and changes its name to `ggml_rope_type`. The motivation for this change is to address the TODO in `llama.h` and use the enum in ggml. Note: This commit does not change the `mode` parameter to be of type `enum ggml_rope_type`. The name `mode` and its usage suggest that it might be more generic and possibly used as a bit field for multiple flags. Further investigation/discussion may be needed to determine if `mode` should be restricted to RoPE types.
If it is a bit field, then my preference would be to declare the different values as constants rather than as an enum. However, I don't think it is a bit field at the moment, even if it used to be in the past. Also, support for GLM rope was removed a while ago, so from what I can tell, the only two valid options are either normal or neox. |
Yes make sense. Probably we need |
This commit removes GGML_ROPE_TYPE_NONE and GGML_ROPE_TYPE_GLM from ggml.h, and back the llama_rope_type enum. I've kept the assert for GGML_ROPE_TYPE_GLM as I'm not sure if it is safe to remove it yet.
@slaren @ggerganov Thanks for the reviews/feedback! I've updated with a commit containing your suggestions as I understood them. Would it makes sense to follow up this PR with one that updates the |
@slaren Sorry, I overlooked this yesterday. What's the advantage of having the values as constants compared to enum? And to become a bit field, do you mean that the @danbev It would be nice to make the change in a single PR. But let's first clarify how to express the rope types before we proceed. |
I just think it is clearer to define the flags as int constants, because an enum is only meant to be used with one value at a time, not as a combination of flags. The type created by the enum would also not be usable (you can still use it as an int in C due to implicit casts, but in C++ that would require explicit casts).
What I meant is that in practice only one value can be used at the same time, so there is no reason to make this a bit field, and using an enum is fine. |
Ok, got it. So how about we define NeoX type like this and we don't even need the "norm" type within diff --git a/ggml/include/ggml.h b/ggml/include/ggml.h
index 22f5dfed..a7ee17d2 100644
--- a/ggml/include/ggml.h
+++ b/ggml/include/ggml.h
@@ -244,6 +244,8 @@
#define GGML_EXIT_SUCCESS 0
#define GGML_EXIT_ABORTED 1
+#define GGML_ROPE_TYPE_NEOX 2
+
#define GGUF_MAGIC "GGUF"
#define GGUF_VERSION 3
@@ -437,12 +439,6 @@ extern "C" {
GGML_FTYPE_MOSTLY_Q4_0_8_8 = 27, // except 1d tensors
};
- // Rotary Positional Embedding (RoPE) types
- enum ggml_rope_type {
- GGML_ROPE_TYPE_NORM = 0,
- GGML_ROPE_TYPE_NEOX = 2,
- };
-
// available tensor operations:
enum ggml_op {
GGML_OP_NONE = 0, And we keep the @danbev We would need to update |
This commit removes the enum ggml_rope_type from ggml.h and replaces it with a define (GGML_ROPE_TYPE_NEOX). This define is used in the code to check if the mode is set to GPT-NeoX. Also the enum llama_rope_type has been updated to reflect this change.
This commit contains a suggestion enable the GGML_ROPE_TYPE_NEOX macro/define to be passed to the shader compiler.
This commit fixes the editorconfig-checker warnings.
Update comment for ggml_rope function.
This reverts commit 6261222.
Add GGML_ROPE_TYPE_NEOX to rope_common.comp.
* ggml : move rope type enum to ggml.h This commit moves the `llama_rope_type` enum from `llama.h` to `ggml.h` and changes its name to `ggml_rope_type`. The motivation for this change is to address the TODO in `llama.h` and use the enum in ggml. Note: This commit does not change the `mode` parameter to be of type `enum ggml_rope_type`. The name `mode` and its usage suggest that it might be more generic and possibly used as a bit field for multiple flags. Further investigation/discussion may be needed to determine if `mode` should be restricted to RoPE types. * squash! ggml : move rope type enum to ggml.h This commit removes GGML_ROPE_TYPE_NONE and GGML_ROPE_TYPE_GLM from ggml.h, and back the llama_rope_type enum. I've kept the assert for GGML_ROPE_TYPE_GLM as I'm not sure if it is safe to remove it yet. * squash! ggml : move rope type enum to ggml.h This commit removes the enum ggml_rope_type from ggml.h and replaces it with a define (GGML_ROPE_TYPE_NEOX). This define is used in the code to check if the mode is set to GPT-NeoX. Also the enum llama_rope_type has been updated to reflect this change. * squash! ggml : move rope type enum to ggml.h This commit contains a suggestion enable the GGML_ROPE_TYPE_NEOX macro/define to be passed to the shader compiler. * squash! ggml : move rope type enum to ggml.h This commit fixes the editorconfig-checker warnings. * squash! ggml : move rope type enum to ggml.h Update comment for ggml_rope function. * Revert "squash! ggml : move rope type enum to ggml.h" This reverts commit 6261222. * squash! ggml : move rope type enum to ggml.h Add GGML_ROPE_TYPE_NEOX to rope_common.comp. * remove extra line --------- Co-authored-by: slaren <[email protected]>
* ggml : move rope type enum to ggml.h This commit moves the `llama_rope_type` enum from `llama.h` to `ggml.h` and changes its name to `ggml_rope_type`. The motivation for this change is to address the TODO in `llama.h` and use the enum in ggml. Note: This commit does not change the `mode` parameter to be of type `enum ggml_rope_type`. The name `mode` and its usage suggest that it might be more generic and possibly used as a bit field for multiple flags. Further investigation/discussion may be needed to determine if `mode` should be restricted to RoPE types. * squash! ggml : move rope type enum to ggml.h This commit removes GGML_ROPE_TYPE_NONE and GGML_ROPE_TYPE_GLM from ggml.h, and back the llama_rope_type enum. I've kept the assert for GGML_ROPE_TYPE_GLM as I'm not sure if it is safe to remove it yet. * squash! ggml : move rope type enum to ggml.h This commit removes the enum ggml_rope_type from ggml.h and replaces it with a define (GGML_ROPE_TYPE_NEOX). This define is used in the code to check if the mode is set to GPT-NeoX. Also the enum llama_rope_type has been updated to reflect this change. * squash! ggml : move rope type enum to ggml.h This commit contains a suggestion enable the GGML_ROPE_TYPE_NEOX macro/define to be passed to the shader compiler. * squash! ggml : move rope type enum to ggml.h This commit fixes the editorconfig-checker warnings. * squash! ggml : move rope type enum to ggml.h Update comment for ggml_rope function. * Revert "squash! ggml : move rope type enum to ggml.h" This reverts commit 6261222. * squash! ggml : move rope type enum to ggml.h Add GGML_ROPE_TYPE_NEOX to rope_common.comp. * remove extra line --------- Co-authored-by: slaren <[email protected]>
This commit moves the
llama_rope_type
enum fromllama.h
toggml.h
and changes its name toggml_rope_type
.The motivation for this change is to address the TODO in
llama.h
and use the enum in ggml.Note: This commit does not change the
mode
parameter to be of typeenum ggml_rope_type
. The namemode
and its usage suggest that it might be more generic and possibly used as a bit field for multiple flags. Further investigation/discussion may be needed to determine ifmode
should be restricted to RoPE types.