Feature/tool calling for local llm #117
Conversation
srebrek
force-pushed
the
feature/tool-calling-for-local-llm
branch
from
08f75d2 to
06f6693
Compare
|
After a test, I noticed that message about tool invocation is also printed by notification service - Its not what we want, LLMService should be able to ommit this message as its not "user friendly" message, Maybe some tweaks can be done in similar way as we process reasoning models, so we can apply tokenType to tool messages |
| await AIHub.Chat() | ||
| .WithModel("gpt-5-nano") | ||
| .WithMessage("What time is it right now?") | ||
| .WithMessage("What time is it right now? Use tool provided.") |
There was a problem hiding this comment.
This addition is not needed, It can use tool without mentioning it directly in message
|
|
||
| await AIHub.Chat() | ||
| .WithModel("gemma3:4b") | ||
| .WithMessage("What time is it right now? Use tool provided.") |
There was a problem hiding this comment.
Same here - "Use tool provided" can be removed
| // OpenAI standard { "tool_calls": [...] } | ||
| if (root.ValueKind == JsonValueKind.Object && root.TryGetProperty("tool_calls", out var toolCallsProp)) | ||
| { | ||
| var calls = toolCallsProp.Deserialize<List<ToolCall>>(new JsonSerializerOptions { PropertyNameCaseInsensitive = true }); |
There was a problem hiding this comment.
JsonSerializerOptions should always be reused
| iterations++; | ||
| } | ||
|
|
||
| if (iterations >= MaxToolIterations) |
There was a problem hiding this comment.
Might be nice to do something with it - log it / return something that inform about extended use
| return calls; | ||
| } | ||
|
|
||
| public class ToolCall |
There was a problem hiding this comment.
I think those classes should already exist, as they (or similar) are already used in other tool support configurations.
We should aim to reuse existing ones
| if (singleCall != null) return new List<ToolCall> { singleCall }; | ||
| } | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
empty catch - never do it :D
If its not critical - just log it,
If it is critical - throw
- Prevent tool definition duplication in the system prompt during subsequent loop iterations. - Refine system prompt to enforce format more effectively.
| if (hasTools && isNewConversation) | ||
| { | ||
| var toolsPrompt = FormatToolsForPrompt(chat.ToolsConfiguration!); | ||
| // Dodaj to jako wiadomoæ systemow¹ lub na pocz¹tku pierwszego promptu u¿ytkownika |
| sb.AppendLine($" Parameters: {JsonSerializer.Serialize(tool.Function.Parameters)}"); | ||
| } | ||
|
|
||
| sb.AppendLine("\n## RESPONSE FORMAT"); |
There was a problem hiding this comment.
Whats the reason to use multiple AppendLine? Can raw string literal be used ?
"""
something
"""
| catch (Exception) | ||
| { | ||
| // No tool calls found | ||
| // No tool calls found no need to throw nor log |
There was a problem hiding this comment.
It's a part of the flow. When response cant be parsed and JsonException error is thrown it means that model responded without using tools (expected as final answer) or response is in invalid format (we display that message also so the user sees what happend. I can check if message is misformated tool call and display it with "error"). I can narrow to catching only JsonException and ArgumentException.
| "If you want to call multiple functions, you have to combine them into one array." + | ||
| "Your response MUST contain only one tool call block:"); | ||
| sb.AppendLine("<tool_call>"); | ||
| sb.AppendLine("{\"tool_calls\": [{\"id\": \"call_1\", \"type\": \"function\", \"function\": {\"name\": \"tool_name\", \"arguments\": \"{\\\"param\\\":\\\"value\\\"}\"}},{\"id\": \"call_2\", \"type\": \"function\", \"function\": {\"name\": \"tool2_name\", \"arguments\": \"{\\\"param1\\\":\\\"value1\\\",\\\"param2\\\":\\\"value2\\\"}\"}}]}"); |
There was a problem hiding this comment.
Try using raw string literal text to structure prompts. It makes it a lot easier to modify and debug in future. Also, if you use them you will probably not need string builder - maybe for building tools part.
|
|
||
| foreach (var tool in toolsConfig.Tools) | ||
| { | ||
| // TODO: refactor to not allow null Function |
There was a problem hiding this comment.
Add an explanation why the refactor is needed in future - if there isn't any - refactor it now 😉 TODOs added in code tend to stay there longer than one would want...
|
|
||
| private List<ToolCall>? ParseToolCalls(string response) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(response)) return null; |
There was a problem hiding this comment.
in single line if statements, move return to next line. Will make code more consistent and easier to read.
| { | ||
| if (string.IsNullOrWhiteSpace(response)) return null; | ||
|
|
||
| string jsonContent = ExtractJsonContent(response); |
There was a problem hiding this comment.
| string jsonContent = ExtractJsonContent(response); | |
| var jsonContent = ExtractJsonContent(response); |
we use var almost everywhere else. Good to keep it consistent.
| return NormalizeToolCalls(calls); | ||
| } | ||
|
|
||
| // TODO: test if those formats are used by any model |
There was a problem hiding this comment.
Same here - add more explanation if there is more work here to be done, or implement it
|
|
||
| if (string.IsNullOrEmpty(name)) return null; | ||
|
|
||
| string? args = "{}"; |
There was a problem hiding this comment.
change to var - this string will most likely not be null
| { | ||
| string name = string.Empty; | ||
| if (root.TryGetProperty("tool_name", out var tn)) name = tn.GetString(); | ||
| else if (root.TryGetProperty("function", out var fn) && fn.ValueKind == JsonValueKind.String) name = fn.GetString(); |
There was a problem hiding this comment.
move the operation in this if/else if to next line. Handle the warnings
|
|
||
| return new ToolCall | ||
| { | ||
| Id = Guid.NewGuid().ToString().Substring(0, 8), |
| if (calls == null) return new List<ToolCall>(); | ||
| foreach (var call in calls) | ||
| { | ||
| if (string.IsNullOrEmpty(call.Id)) call.Id = Guid.NewGuid().ToString().Substring(0, 8); |
There was a problem hiding this comment.
use range indexer and format the if
|
|
||
| private List<ToolCall> NormalizeToolCalls(List<ToolCall>? calls) | ||
| { | ||
| if (calls == null) return new List<ToolCall>(); |
There was a problem hiding this comment.
move to next line and use collection expression
Working implementation of tool calling for local LLMs.
Left TODOs about testing with other LLMs. Currently working perfectly with gemma3:4b (example 7).
Closes #111