feat(server): Add tool call support to WebUI (LLama Server)

[samolego]

Make sure to read the contributing guidelines before submitting a PR

Hi there!
I added tool calling support for the frontend interface to llama-server. There are still some things to fix, but I'm opening this early to get some feedback.

Currently there's only 1 tool added (basic javascript interpreter with sandboxed iframe code eval), but the code structure supports expanding in the future.

[samolego]

I added toggles to settings now:

Also merged the assistant responses on rendering, it looks better imo:

[samolego]

After merging latest master:

[Loufe]

You may want to avoid enlarging the scope any further in this PR, it's already quite large. Further changes should be in a further PR.

[samolego]

You may want to avoid enlarging the scope any further in this PR, it's already quite large. Further changes should be in a further PR.

Sure! I'm done with the changes I wanted. I can perhaps move js repl tool into a separate PR, if needed.

[ngxson]

I done a quick review, haven't yet looked at the entire code. But I think some parts can be done easier, less abstract, so it will become more obvious for other people if someone wants to build on top of this.

A good example is the AVAILABLE_TOOLS array and some literal types. As we don't yet have many tools for now (and even if we have multiple tool, how many we will have? I think it will be less than 10), it's better to write them explicitly in the code.

[ngxson]

I think this can be kept simple for now, by simply listing the list of tools here (we don't have many, right?)

[samolego]

Well, sure can, but I think we will need to readd it in the future?
#13501 (comment)

[ngxson]

If you're saying that we will have user-provided tool, then I think this is not the correct approach.

This Array.from(AVAILABLE_TOOLS.values()) will be called when the app is loaded. It will become out of sync with AVAILABLE_TOOLS if something decide to add a new tool after that

[samolego]

Ok!

[ngxson]

I know we don't yet support stream with tool_calls, but we will have it soon and I think this whole if..else may become redundant in the future

What I'm thinking is to abstract out this, maybe into a func createChatCompletionStream, and from outside perspective the function will always return streamed response.

Inside, we if we see tool_calls, we disable streamwhile still converts the non-stream response to streamed (kinda a compat layer - or adapter if you are talking about design pattern). In the near future, when we have streamed tool_calls support, we can simply delete that code branch in createChatCompletionStream

[samolego]

This is right, though ... I'm not really confident I can do it well enough tbf :/, at least not in this PR as I feel like it will grow even more.

[ngxson]

I can help doing this if you can take care of the other concern about weak typing

Btw, even though we don't yet have mcp, I think having a placeholder class for it (just to demo how your code is designed to handle such case) can be a good idea

[ngxson]

I expected this to be a plain variable like AVAILABLE_TOOLS = 'calc' | 'draw' | ...

Abstracting the naming out will make it difficult for other developers to read the code

[samolego]

I'm not sure what you mean by that? It's a map, iterable type, not just "one" tool indicated by type 'calc' | 'draw' | ...

[ngxson]

I think it should be mapping from a literal type to AgentTool, something like: new Map<'calc' | 'draw' | ..., AgentTool>

I don't think we should allow arbitrary tool right now as it make the PR overly complicated, while we don't yet have any plan how to implement it correctly (both in term of UX/UI and code structure)

Having a second thought, see in comment below

[ngxson]

I don't quite like this AVAILABLE_TOOLS because it's quite weak typing. As pointed out in https://github.com/ggml-org/llama.cpp/pull/13501/files#r2098510552 , it also not allowing us to add tool in an asynchronous manner.

The better way here is to have a dedicated class for managing tool, something like:

interface ToolParam {
  type: ...; // this must be a literal type - for MCP, we have a dedicated "mcp" type for it
  enabled: boolean;
  builtIn: boolean;
  name: string;
  description: string;
  // etc
};

class AgentTool {
  constructor(params: ToolParam) {
    // etc
  }

  // to "plain object" representation, ready for JSON serialization
  toPOJO(): ToolParam {
    return {
      // etc
    };
  }
}

class Tools {
  AVAILABLE_TOOLS = new Map<string, AgentTool>();

  init(tools: ToolParam[]) {
    for (const tool of tools) {
      // ...
    }
  }

  // to "plain object" representation, ready for JSON serialization
  toPOJO(): ToolParam[] {
    return Array.from(this.AVAILABLE_TOOLS.values()).map(tool => tool.toPOJO());
  }
}

This will allow serialize the tool config, which then can be saved later in localStorage

export const CONFIG_DEFAULT = {
  ...,
  builtInTools: [] as ToolParam[], // use this to overwrite built-in tool configs
  customTools: [] as ToolParam[], // by default, we have no custom tools

[samolego]

Ah, I understand it now, sorry.

[ngxson]

AVAILABLE_TOOLS is used for executing the functions as well

Yes but you are also aiming to make AVAILABLE_TOOLS expandable (i.e. user can add things to it later on), and our current design is not suitable for that case.

If all tools inside AVAILABLE_TOOLS are known and fixed (no custom tool support), it should have had a strict type for the key, new Map<'calc' | 'draw' | ..., AgentTool>, to communicate that all the elements are known.

If you want to make user to customizing something then allowing it to be saved as config, the first thing you need think about is how to (de)serialization it

[samolego]

Hey,
I think I'll need more time to incorporate settings and config serialization. I think that I should leave it simple as you suggested at first (#13501 (comment)) (sorry!) for the scope of this PR. There's another reason - I haven't really deepdived into MCP yet (which is kinda the only reason why we're still here :P) and I don't want to make bad architectural decisions that we'll need to revert later on anyway. I believe that expansion (and MCP support) should be left to separate PR.

Ergo we retire AVAILABLE_TOOLS for now and have a simple toggle for js repl in settings and only use that for now.

Is that still OK with you?

[samolego]

@ngxson ?

[ngxson]

I currently busy with multimodal support, will go back to this later.

Btw, streaming tool call has just been merged recently, I think you should update this PR to support that.

[samolego]

Sure, great news 🎉 !

[samolego]

I done a quick review, haven't yet looked at the entire code. But I think some parts can be done easier, less abstract, so it will become more obvious for other people if someone wants to build on top of this.

A good example is the AVAILABLE_TOOLS array and some literal types. As we don't yet have many tools for now (and even if we have multiple tool, how many we will have? I think it will be less than 10), it's better to write them explicitly in the code.

Yeah, I agree, we'll probably have less than 10 "official" ones. But if we want to support mcp someday, that number can quickly grow I'd say. Rn you could simply register mcp tools into the available tools and it should just work 😄 (tm).

On the other hand, we can put another variable for mcp later on (when we implement it) and keep the official tools to still be registered "by hand" everywhere.

[samolego]

I have to do some more testing now since supporting streaming tool calls.