gguf: better type usage

[ngxson]

Follow up #640

Ref comments:

• gguf: Add types for LLM architectures #640 (review) by @julien-c suggests using a check metadata["general.architecture"] === ... to select the correct type
• gguf: Add types for LLM architectures #640 (comment) by @coyotte508 suggests using less generic but more verbose code

The type system introduce in this PR allows type-checking at both compile time & runtime:

const model: GGUFMetadata<GGUFType.STRICT> = null as any;

if (model["general.architecture"] === "whisper") {
	model["encoder.whisper.block_count"] = 0;
	// @ts-expect-error because it must be a number
	model["encoder.whisper.block_count"] = "abc";
}

if (model["tokenizer.ggml.model"] === undefined) {
	// @ts-expect-error because it's undefined
	model["tokenizer.ggml.eos_token_id"] = 1;
}
if (model["tokenizer.ggml.model"] === "gpt2") {
	// @ts-expect-error because it must be a number
	model["tokenizer.ggml.eos_token_id"] = undefined;
	model["tokenizer.ggml.eos_token_id"] = 1;
}

if (model["general.architecture"] === "mamba") {
	model["mamba.ssm.conv_kernel"] = 0;
	// @ts-expect-error because it must be a number
	model["mamba.ssm.conv_kernel"] = "abc";
}
if (model["general.architecture"] === "llama") {
	// @ts-expect-error llama does not have ssm.* keys
	model["mamba.ssm.conv_kernel"] = 0;
}

Type checks can be disable with GGUFMetadata<GGUFType.NON_STRICT>

[ngxson]

Also in the process, I noticed some details not being mentioned in gguf specs:

• For generic fields, only general.architecture is required. Other fields are all optional.
• LLM_KV_VOCAB_SIZE is not assured to present (if it's not present, we take the length of vocab list), ref:
  https://github.com/ggerganov/llama.cpp/blob/842500144ee02c8b0a46d2cc43880f8d80998fa5/llama.cpp#L3779
  Not a big problem for now, we can fix that later.
• MOE is optional (as long as many other Rope and Attention related fields), ref:
  https://github.com/ggerganov/llama.cpp/blob/842500144ee02c8b0a46d2cc43880f8d80998fa5/llama.cpp#L3785
  (explain: third param of get_key is set to false, indicating that it is not required)
  ==> Resolved with 9a9e771
• whisper.cpp indeed does not use gguf, ref code:
  https://github.com/ggerganov/whisper.cpp/blob/58210d6a7634ea1e42e0a2dab611f4a0518731dc/whisper.cpp#L1258
  So unfortunately we don't have any way to test whisper gguf. Regarding fields being mentioned in the documentation, I found a related discussion here: GGUF file format specification ggerganov/ggml#302 (comment)

[mishig25]

Very nice PR! And much needed improvement to the typing of gguf, especially type inference.
Left some nits. Otherwise, we are close to merge 🚀

[ngxson]

FYI, the unknown here allow casting between GGUFParseOutput<GGUFType.strict> and GGUFParseOutput<GGUFType.nonStrict>. Funny though, I have no idea why adding unknown make it work. Maybe this is not the best way to do, so feel free to tell if you have other suggestions.

[julien-c]

looks in good shape! do you want to take a quick look too @coyotte508?

[mishig25]

Question regarding GGUFType.NON_STRICT. Why do we need GGUFType.NON_STRICT? Shouldn't we want GGUFType.STRICT to be always the case?

[ngxson]

I believe that GGUFType.NON_STRICT will be useful in case user want to use their own gguf structure. In fact, gguf can also be used for storing control vectors, lora weights, and maybe other use cases outside of llama.cpp

[mishig25]

LGTM !

[mishig25]

@coyotte508 could you give a quick look and I will merge 🚀

[coyotte508]

Looks good!

I'm wondering about flat paths vs nested metadata (maybe it could be another option in the future 🤔) but LGTM with the 1/2 changes requested

[coyotte508]

Let's use // @ts-expect-error instead of // @ts-ignore in general

(no need for eslint-disable this way)

Here I think you can change const metadata: GGUFMetadata to const metadata: GGUFMetadata<GGUFType.NON_STRICT> to remove the error (not sure if it's the best fix)

[ngxson]

Fixed in 31bac8b

[coyotte508]

We should probably switch to something like https://github.com/sindresorhus/type-fest/blob/main/source/except.d.ts

interface GGUFMetadataOptions {
	/**
   * ...
   * 
	 * @default true
   */
  strict: boolean;
}


export GGUFMetadata<Options extends GGUFMetadataOptions = { strict: true}> {
  ...

[ngxson]

Good idea 👍 I've never thought about this before. Implemented in 31bac8b

[ngxson]

I'm wondering about flat paths vs nested metadata (maybe it could be another option in the future 🤔)

@coyotte508 I thought about using nested object, but that seems to be quite complicated atm, since part of the code/naming here is copied directly from cpp file (so it save us some headaches)

But I'm also agree that having nested object is a reasonable feature, since gguf keys are "namespaced" anyway. Maybe we will re-visit this idea later on.

[julien-c]

alright let's merge!

re. nested properties, that's what they did in https://github.com/ahoylabs/gguf.js (credited as one of the inspirations for this package) but I'm not sure it's worth it / I find it cleaner to just expose the raw data from the GGUF file.