feat: Allow overriding GGUF metadata when loading model

[KerfuffleV2]

For example you can do:

--override-kv tokenizer.ggml.bos_token_id=int:1 
--override-kv llama.attention.layer_norm_rms_epsilon=float:0.001
--override-kv tokenizer.ggml.add_bos_token=bool:false

Only supports int-type, float-type and boolean overrides currently. I think this is something that could be pretty useful for development, it's also something that would allow end users to correct metadata that has issues without having to download a whole new model.

This also makes the loader metadata KV handling nicer. Instead of:

GGUF_GET_KEY(ctx, hparams.n_ctx_train, gguf_get_val_u32, GGUF_TYPE_UINT32, true, kv(LLM_KV_CONTEXT_LENGTH));

You can now write:

ml.get_key(LLM_KV_CONTEXT_LENGTH, hparams.n_ctx_train, true);

Since hparams.n_ctx_train is a uint32_t it can figure out to call gguf_get_val_u32 and that the GGUF type is GGUF_TYPE_UINT32 without having to explicitly write it.

get_key() also returns a boolean value so it's possible to know whether the target got changed or not, which makes the logic for stuff like the special token handling less convoluted.

This could use a bit more refining but I don't want to put too much work into that until I know it has a chance of being accepted.

[KerfuffleV2]

@cebtenzzre

Is this in any way, shape or form heading in the right direction? (I mean the general approach, the code is terrible, also it doesn't actually do any overriding yet.)

Expand

namespace Merp {

    template<typename T> using gguf_getter = T (*)(const gguf_context *, const int);
    template<typename T> class Derp {
        public:
        static struct GetValue{} getValue;
        static struct GetArrayLen{} getArrayLen;

        static const gguf_type gt;
        static constexpr bool can_override = true;
        static const gguf_getter<T> getter;

        static void get_kv(const gguf_context * ctx, const int k, T & target) {
            const enum gguf_type kt = gguf_get_kv_type(ctx, k);

            if (kt != gt) {
                throw std::runtime_error(format("key %s has wrong type %s but expected type %s",
                    gguf_get_key(ctx, k), gguf_type_name(kt), gguf_type_name(gt)));
            }
            target = getter(ctx, k);
        }

        static bool set(const gguf_context * ctx, const int k, T & target, struct llama_model_kv_override *override = nullptr) {
            get_kv(ctx, k, target);
            return true;
        }
    };

    template<> const gguf_type Derp<bool>::gt = GGUF_TYPE_BOOL;
    template<> const gguf_getter<bool> Derp<bool>::getter = gguf_get_val_bool;

    template<> const gguf_type Derp<uint8_t>::gt = GGUF_TYPE_UINT8;
    template<> const gguf_getter<uint8_t> Derp<uint8_t>::getter = gguf_get_val_u8;

    template<> const gguf_type Derp<uint16_t>::gt = GGUF_TYPE_UINT16;
    template<> const gguf_getter<uint16_t> Derp<uint16_t>::getter = gguf_get_val_u16;

    template<> const gguf_type Derp<uint32_t>::gt = GGUF_TYPE_UINT32;
    template<> const gguf_getter<uint32_t> Derp<uint32_t>::getter = gguf_get_val_u32;

    template<> const gguf_type Derp<uint64_t>::gt = GGUF_TYPE_UINT64;
    template<> const gguf_getter<uint64_t> Derp<uint64_t>::getter = gguf_get_val_u64;

    template<> const gguf_type Derp<int8_t>::gt = GGUF_TYPE_UINT8;
    template<> const gguf_getter<int8_t> Derp<int8_t>::getter = gguf_get_val_i8;

    template<> const gguf_type Derp<int16_t>::gt = GGUF_TYPE_UINT16;
    template<> const gguf_getter<int16_t> Derp<int16_t>::getter = gguf_get_val_i16;

    template<> const gguf_type Derp<int32_t>::gt = GGUF_TYPE_UINT32;
    template<> const gguf_getter<int32_t> Derp<int32_t>::getter = gguf_get_val_i32;

    template<> const gguf_type Derp<int64_t>::gt = GGUF_TYPE_UINT64;
    template<> const gguf_getter<int64_t> Derp<int64_t>::getter = gguf_get_val_i64;

    template<> const gguf_type Derp<float>::gt = GGUF_TYPE_FLOAT32;
    template<> const gguf_getter<float> Derp<float>::getter = gguf_get_val_f32;

    template<> const gguf_type Derp<double>::gt = GGUF_TYPE_FLOAT64;
    template<> const gguf_getter<double> Derp<double>::getter = gguf_get_val_f64;

    template<> const gguf_type Derp<const char *>::gt = GGUF_TYPE_STRING;
    template<> const gguf_getter<const char *> Derp<const char *>::getter = gguf_get_val_str;

    template<> const gguf_type Derp<std::string>::gt = GGUF_TYPE_STRING;
    template<> const gguf_getter<std::string> Derp<std::string>::getter = nullptr;
    template<> bool Derp<std::string>::set(const gguf_context * ctx, const int k, std::string & target, struct llama_model_kv_override *override) {
        const char * temp;
        Derp<const char *>::get_kv(ctx, k, temp);
        target = std::string(temp);
        return true;
    }
}

This actually seems to work for handling the normal keys. I still have to figure out how to fit overrides and array lengths with this approach.

[cebtenzzre]

That seems like a better general approach, but the way you are setting the variables is again unidiomatic - you should really be using class template specializations, which are the main building block of the C++ type system. Consider this example:

template <typename T>
class Derp_Base; // no implementation

template <>
class Derp_Base<int> {
public:
    static constexpr int type_id = 1;
};

template <typename T>
class Derp: public Derp_Base<T> {
    Derp() = delete; // cannot be instantiated

public:
    static int foo() {
        return Derp::type_id;
    }
};

void foo() {
    Derp<int>::foo(); // returns 1
    // Derp<float>::foo(); will cause "use of incomplete type" error
}

Derp_Base in this example could also contain a protected static member function that calls the appropriate gguf_get_val_*.

[KerfuffleV2]

That seems like a better general approach, but the way you are setting the variables is again unidiomatic

Unfortunately, I'm pretty sure I'm a long ways off of being able to write complicated idiomatic template stuff in C++.

If you're not tired of teaching me C++ yet...

Expand

#define MK_GKVBASE(cty, gtid, gfun) \
    template<> class GKV_Base<cty> { \
        public: \
        static constexpr gguf_type gt = gtid; \
        static constexpr cty (*getter)(const gguf_context *, const int) = gfun; \
    }

    template <typename T> class GKV_Base;
    MK_GKVBASE(bool,         GGUF_TYPE_BOOL,    gguf_get_val_bool);
    MK_GKVBASE(uint8_t,      GGUF_TYPE_UINT8,   gguf_get_val_u8  );
    MK_GKVBASE(uint16_t,     GGUF_TYPE_UINT16,  gguf_get_val_u16 );
    MK_GKVBASE(uint32_t,     GGUF_TYPE_UINT32,  gguf_get_val_u32 );
    MK_GKVBASE(uint64_t,     GGUF_TYPE_UINT64,  gguf_get_val_u64 );
    MK_GKVBASE(int8_t,       GGUF_TYPE_INT8,    gguf_get_val_i8  );
    MK_GKVBASE(int16_t,      GGUF_TYPE_INT16,   gguf_get_val_i16 );
    MK_GKVBASE(int32_t,      GGUF_TYPE_INT32,   gguf_get_val_i32 );
    MK_GKVBASE(int64_t,      GGUF_TYPE_INT64,   gguf_get_val_i64 );
    MK_GKVBASE(float,        GGUF_TYPE_FLOAT32, gguf_get_val_f32 );
    MK_GKVBASE(double,       GGUF_TYPE_FLOAT64, gguf_get_val_f64 );
    MK_GKVBASE(const char *, GGUF_TYPE_STRING,  gguf_get_val_str );

    struct GetArrayLen{int value;};
    template<> class GKV_Base<GetArrayLen> {
        public:
        static constexpr gguf_type gt = GGUF_TYPE_ARRAY;
        static GetArrayLen getter(const gguf_context *ctx, const int k) {
            return GetArrayLen{gguf_get_arr_n(ctx, k)};
        }
    };

    template<typename T>
    class GKV: public GKV_Base<T> {
        GKV() = delete;

        public:
        static T get_kv(const gguf_context * ctx, const int k) {
            const enum gguf_type kt = gguf_get_kv_type(ctx, k);

            if (kt != GKV::gt) {
                throw std::runtime_error(format("key %s has wrong type %s but expected type %s",
                    gguf_get_key(ctx, k), gguf_type_name(kt), gguf_type_name(GKV::gt)));
            }
            return GKV::getter(ctx, k);
        }

        static bool set(const gguf_context * ctx, const int k, T & target, struct llama_model_kv_override *override = nullptr) {
            target = get_kv(ctx, k);
            return true;
        }
    };

    template<>
    class GKV<std::string>: public GKV_Base<const char *> {
        public:
        static bool set(const gguf_context * ctx, const int k, std::string & target, struct llama_model_kv_override *override = nullptr) {
            target = std::string(GKV<const char *>::get_kv(ctx, k));
            return true;
        }
    };

Is something like that kind of what you were talking about? It's dawned on me that templating the array length thing to work with any int types is actually really stupid and pointless. So I'm almost certainly going to remove the GetArrayLen thing, I just wanted to figure it out (it seems to work).

If this general approach is fairly reasonable, I think there's a pretty clean way to make the override part fit into it. I'll just have something like a bool try_override(...) method with a default implementation that returns false. Then the types that support overriding can try the override and return true if they applied the override, otherwise it can fall back to fetching from the GGUF metadata like the code I pasted here.

[cebtenzzre]

Yeah, that seems a lot better. I think it would be prettier if 'getter' were always a regular method and not a function pointer - I would argue that const function pointers are an anti-pattern. So like:

static cty getter(const gguf_context * ctx, const int k) { return gfun(ctx, k); }

Also, I'm not a fan of the macro - if you want it to be less repetitive, you can add another class to the hierarchy (like the standard library does with std::true_type and std::false_type) - you can add whatever you'd like to Derp_Base_Type and its template parameters:

template <int type_id_>
struct Derp_Base_Type {
    static constexpr int type_id = type_id_;
};

template <typename T>
struct Derp_Base; // no implementation

template <> struct Derp_Base<int>:   Derp_Base_Type<1> {};
template <> struct Derp_Base<float>: Derp_Base_Type<2> {};

[KerfuffleV2]

Also, I'm not a fan of the macro - if you want it to be less repetitive, you can add another class to the hierarchy

How much do you hate it? :) I do want to be less repetitive, but it's not really the typeid part I'm trying to avoid with the macro but all the class boilerplate.

I'd really prefer to write:

MK_GKVBASE(uint8_t,      GGUF_TYPE_UINT8,   gguf_get_val_u8  );
MK_GKVBASE(uint16_t,     GGUF_TYPE_UINT16,  gguf_get_val_u16 );
MK_GKVBASE(uint32_t,     GGUF_TYPE_UINT32,  gguf_get_val_u32 );
MK_GKVBASE(uint64_t,     GGUF_TYPE_UINT64,  gguf_get_val_u64 );

instead of

template<> class GKV_Base<u8> {
    public:
    static constexpr gguf_type gt = GGUF_TYPE_UINT8;
    static u8 get(const gguf_context *ctx, const int k) { return gguf_get_val_u8(ctx, k); }
};
template<> class GKV_Base<u8> {
    public:
    static constexpr gguf_type gt = GGUF_TYPE_UINT8;
    static u8 get(const gguf_context *ctx, const int k) { return gguf_get_val_u8(ctx, k); }
};
template<> class GKV_Base<u8> {
    public:
    static constexpr gguf_type gt = GGUF_TYPE_UINT8;
    static u8 get(const gguf_context *ctx, const int k) { return gguf_get_val_u8(ctx, k); }
};
template<> class GKV_Base<u8> {
    public:
    static constexpr gguf_type gt = GGUF_TYPE_UINT8;
    static u8 get(const gguf_context *ctx, const int k) { return gguf_get_val_u8(ctx, k); }
};

[cebtenzzre]

Here is a more specific self-contained example of what I mean:

int   gguf_get_int  (const struct gguf_context * ctx, const int k) { return -1;    }
float gguf_get_float(const struct gguf_context * ctx, const int k) { return -1.0f; }

template <typename T>
using gguf_getter = T(const struct gguf_context * ctx, int k);

template <typename T, int gt_, gguf_getter<T> gfun>
struct GKV_Base_Type {
    static constexpr int gt = gt_;
    static T get(const struct gguf_context *ctx, const int k) { return gfun(ctx, k); }
};

template <typename T>
struct GKV_Base; // no implementation

template <> struct GKV_Base<int>:   GKV_Base_Type<int,   1, gguf_get_int>   {};
template <> struct GKV_Base<float>: GKV_Base_Type<float, 2, gguf_get_float> {};

You can do this with any information that can be stored in a template parameter (which is any constant or type).

[KerfuffleV2]

Here is a more specific self-contained example of what I mean

That helped a lot. I rewrote most of this stuff.

The try_override part is sort of weird, there's probably a better way to do that. The reason I'm jumping through hoops here is because I can't specialize just based on the return type.

[cebtenzzre]

The thing about this type of template overload with std::enable_if is that every one of them needs a std::enable_if that represents its use case - C++11 at least doesn't have a simple way that I know of to prioritize one template overload over another. If you do want to have a general fallback template, it needs to represent the inverse of the types that are allowed for any of the others - e.g. !is_integral and such.

[KerfuffleV2]

Having to write forwarding methods is also sort of annoying as well, so there's probably a better way.

[cebtenzzre]

I pushed a simplification that I believe addresses your comments. The templates look good to me.

[KerfuffleV2]

I pushed a simplification that I believe addresses your comments.

Living dangerously but you guessed correctly that I wasn't around. Thanks, I appreciate it! (I also should have thought of that.)

The templates look good to me.

Amazing, though now that the magical C++ stuff is bigger and more magical I have a weird feeling this is less likely to get merged than the original version. :)

[KerfuffleV2]

Okay, I think this is finally ready for review.

1. Lots of cleanups thanks to cebtenzzre's help.
2. Runs successfully with address sanitizing turned on.
3. Since the gguf_type is now based on the target type, I double checked that every case of llama_model_loader.get_key has a target of the expected type.

This currently doesn't support overriding string or array types, but that could be added (though for arrays you'd probably want to load the override from a file or something). Being able to do something like override the vocab for a model could be useful.

The interface (llama_model_loader.get_key) doesn't currently support automatically fetching into an array target like ml.get_key(target, LLM_KV_MY_ARRAY) where target is a uint8_t * or std::vector<uint8_t> but that actually could be added pretty easily. You can fetch into an array info type that has the array item type, length and void * to the data.

The only potential issue I'm aware of in the current state is that overrides don't currently apply to slaren's funky fetch KV metadata as a string thing. I think the way it prints out the metadata values currently should show the original value in the actual model. If necessary I can make it so overrides apply to the stored string version (personally I'd rather store the original value and convert to a string if needed, storing it as a string seems weird to me).

[cebtenzzre]

@ggerganov Have you taken a look at this PR? Kerfuffle said on Discord that he closed this due to lack of interest.

[KerfuffleV2]

@cebtenzzre

Kerfuffle said on Discord that he closed this due to lack of interest.

Maybe it's weird but I still haven't changed my opinion. Perhaps I just need to completely rethink my concept of "interest". You'd better be careful, I think associating with me might be causing you to turn invisible too. Run, save yourself while you still can!

Joking aside, let's just close all these pulls again. If you or someone else wants to take any of them and make a pull request and do whatever with it, you have my blessing. Use the code in whole or part. However, I'd prefer not to have these pull requests open and owned by me because it's going to be confusing and people are going to think I'm the one to go to for questions, etc.

[ggerganov]

Let's merge after resolving the CI

p.s. sorry for the delay

[KerfuffleV2]

I rebased on master and fixed the one obvious issue (a call to GET_KEY that was added since the last rebase). I also added a note to the KV dump output that it doesn't currently take overrides into account.

Just going to cut and paste my summary of the current state from a different message:

And finally the override stuff: I think that one is in a pretty good state and was mergeworthy, at least back when I requested the second review around 2 weeks ago. I kept it up to date and it merged cleanly at least when I lasted looked at it but people may have added stuff that uses the metadata since then, and that would be using the old format not the new way to access the metadata. So it may be necessary to review the changes and see if anything needs to be adjusted. Also, that pull removed the normal accessor define (GET_KEY or whatever it was called) so stuff that tries to use it will merge but won't actually compile because that define no longer exists. The only other potential issue with that is how it works with the "access metadata as string stuff", right now the two things don't interact at all so slaren's thing will use/print out the metadata without taking any overrides into account. I think printing it out like that is fine, overrides probably should affect accessing the actual data though.

---

I compiled it, tested, seems to work. If merging is actually desired, it may be good to do a bit more testing just because I don't think anyone other than me ever really tested it (and a different person might think to test something I missed). Also it's been sitting there for a while and maybe there are interactions with other changes that occurred.

I'll let someone else do the actual merge if the feature/current implementation is actually wanted. I'm not trying to just force it through without consensus or anything like that.

[ggerganov]

The old GGUF_GET_KEY macro wasn't great so this refactoring is useful just for that.

I did a few tests and the overrides seem to work as expected. No need to add array and string override support for now - let's see if we encounter some specific use cases for these in the future