Llamacpp backend

[Gnurro]

llama-cpp-python-based backend to allow running GGUF/GGML models.

[phisad]

the comment seems unnecessary

[Gnurro]

Why?

[phisad]

Because it is exactly what is written in code.

[phisad]

why loop if it's a dict. better self.context_length = metadata.get("context_length", 512)

[Gnurro]

I can change it to that. The way I wrote is due to the metadata not being thoroughly standardized and me checking with prints during development.

[Gnurro]

This does not work, as the key is not just context_length, but has a prefix for the model architecture. For example openchat_3.5-GGUF has the key llama.context_length. So it either also needs to get the model architecture from the metadata or remain in the loop with the if "context_length in key.
I need to test a lot more models to make sure that the second solution would work.

[phisad]

OK, I feared that. Then stick to the loop.

[Gnurro]

I'll rely on what is specified (https://github.com/ggerganov/ggml/blob/master/docs/gguf.md#required), which gets rid of the loop. Specific models need testing and setup if they are missing something (ie chat template) anyways, so if a model file does not follow the basic specification, I assume it'll be noticed then.

[phisad]

I suggest to extract a load_formatter method wich goes below load_model and a test_case for loading a formatter. And self.bos_string also seems not to be accessed anymore after this line, so why in self?

[phisad]

I suggest to raise this already in check_context_limit (see comment below). the brackets are also hard to read here.

[Gnurro]

It's raised here for easy access to the model name, so that it gets logged properly. If it's raised in the checking function, the model name would need to be passed to it as well. I can make that change, but it would mean to change this in HF backend as well.

I don't know what you mean with hard to read brackets here.

[phisad]

Return of multiple variables can be done, but is hard to read context_check[3]. Then better return a named tuple: check_results.max_tokens or something. That this line needs a comment is a bad sign: if not context_check[0]: # if context is exceeded, context_check[0] is False.

If you need access to the model name (for logging), maybe move this method to Model.

[phisad]

context_size is an input parameter and does not need to be returned. And instead of returned two extract arguments the token_used and tokens_left should be in the error message. Maybe rename method into assert_context_limit

[Gnurro]

Context size is returned here so that it can be used when the exception is caught in custom Player/GameMaster classes. The idea is to leave it to the game developers how to handle their messages if the context would be exceeded, and providing all details about the context this way allows for flexibility.

[phisad]

Nah, here we talk about the check utility. And it does not throw an exception (yet)? So if game devs are supposed to catch the exception (and e.g. try re-prompting) then these details should become part of the exception as class attributes (not the return params here).

[phisad]

see comments in code

[Gnurro]

I've found an issue with custom chat template application (after excising the chat formatter part), I'll rework the generation method to work as intended. We can close the PR until that is done.
FYI: llama-cpp-python defaults to the llama2 template silently, and it took me hours to find that part in its source code. It's thoroughly hardcoded, so create_chat_completion can't be used for any model that does not use the llama2, mistral or chatml templates. Formatting using the already implemented chat formatter and then passing the prompt to create_completion will solve this. Models that come with a chat template supposedly should use it, but the actual prompt used in create_chat_completion can't be checked without hacking major parts of llama-cpp-python, so I'll fall back to the basic create_completion for these as well, to be sure that formats are applied properly.
Open issue on the llama-cpp-python repository regarding this issue: abetlen/llama-cpp-python#717

[phisad]

We can close the PR until that is done

No need to close this (from my side). You can continue using this branch.

[Gnurro]

Tested and working with a full benchmark run using openchat_3.5-GGUF-Q5. Should be ready to merge from my view.