Repository navigation
.Net: Function calling: Add function author role and name property - #3302
Gina Triolo (gitri-ms) wants to merge 6 commits into
Conversation
| /// <summary> | ||
| /// If role is function, the name of the function that produced the content | ||
| /// </summary> | ||
| public string? Name { get; set; } = null; |
There was a problem hiding this comment.
Surfacing Stephen Toub (@stephentoub) 's suggestion from the issue (#3017):
If we're not comfortable adding a name property to a chat message in support of this, we could also just add a dictionary to the message so that additional context can be provided and interpreted by the chat completion service however it likes:
main...stephentoub:semantic-kernel:fixfunctioncalling
There was a problem hiding this comment.
Gina Triolo (@gitri-ms) I would not change Abstractions regarding FunctionCalling support, instead I would think of having a Specialized ChatHistory class for OpenAI where you can add that function.
On the examples where we know you are dealing with a OpenAIModel, you can check if the ChatHistory is an OpenAIChatHistory and play with the detailed function data.
SKChatMessage of a OpenAIChatHistory will have the name.
There was a problem hiding this comment.
I understand the concern here, but if we don't have a way to support additional fields like this in the base object, then devs will have to downcast the ChatHistory whenever they want to interact with OpenAI features, which makes for a really clunky experience. Talking with Ben Thomas (@alliscode), we thought that Stephen Toub (@stephentoub)'s approach would be most flexible and future-proof, so I updated this PR to add an optional dictionary to ChatMessageBase.
|
I found that your version has an issue. I have a CheckStock function and my prompt is "How many tables and chairs are there in stock?". Note that LLM should issue 2 function calling messages (I have added a do-while loop in Example59). However, with your version, LLM only called CheckStock once. In my original working version using native OpenAI, the Messages in the ChatCompletionsOptions keeps both the function calling assistance message and the function return user message. In your solution, only function return user Below is my Example59 source code. I asked for 2 stock items but LLM only call CheckStock once. The reason is because the function calling message was missing in the chat history. |
| /// <summary> | ||
| /// If role is function, the name of the function that produced the content | ||
| /// </summary> | ||
| public string? Name { get; set; } = null; |
There was a problem hiding this comment.
Gina Triolo (@gitri-ms) I would not change Abstractions regarding FunctionCalling support, instead I would think of having a Specialized ChatHistory class for OpenAI where you can add that function.
On the examples where we know you are dealing with a OpenAIModel, you can check if the ChatHistory is an OpenAIChatHistory and play with the detailed function data.
SKChatMessage of a OpenAIChatHistory will have the name.
| if (kernel.Functions.TryGetFunctionAndContext(functionResponse, out ISKFunction? func, out ContextVariables? context)) | ||
| { | ||
| var kernelResult = await kernel.RunAsync(func, context); | ||
| var result = (await kernel.RunAsync(func, context)).GetValue<object>(); |
There was a problem hiding this comment.
Good catch, I think i just overlooked that. However, now that you've pointed it out, it's not as simple as it seems to add the OpenAI message back into chat history. We can access the message via IChatResult.ModelResult.GetResult<ChatModelResult>().Choice.Message, which gives us back an Azure.AI.OpenAI.ChatMessage object. However, I dont believe we have the capability to add that back into chat history without losing some information in the translation (would convert to our ChatMessage to add to ChatHistory, and then back to Azure.AI.OpenAI.ChatMessage in ClientBase.cs)
There was a problem hiding this comment.
Actually.... I think we are able to handle this with the dictionary approach. When adding this message back into chat history, would need to add key/value for "function_call" into the dictionary and make sure that gets copied back into the OAI message. I will update the PR with this.
| validRole != ChatRole.Function) | ||
| { | ||
| throw new ArgumentException($"Invalid chat message author role: {role}"); | ||
| } |
There was a problem hiding this comment.
I think we should consider removing this role validation altogether. If the role is invalid according to OpenAI, it'll eventually tell us that. The problem with this validation is if OpenAI adds more valid roles, this validation will proactively block their use and will require changes to SK and picking up a new package in order to be able to use them, even if they'd otherwise work.
It's similar to how .NET used to proactively try to validate paths being passed to File/Path APIs, but now that validation has been removed and we just rely on the OS to say whether there was something wrong with the path.
| /// If there is no plugin name, this is the same as the function name. | ||
| /// </summary> | ||
| public string FullyQualifiedName => | ||
| this.PluginName.IsNullOrEmpty() ? this.FunctionName : string.Join(OpenAIFunction.NameSeparator, this.PluginName, this.FunctionName); |
There was a problem hiding this comment.
string.Join(..., params string[]) is unnecessary expense here. This can instead be:
| this.PluginName.IsNullOrEmpty() ? this.FunctionName : string.Join(OpenAIFunction.NameSeparator, this.PluginName, this.FunctionName); | |
| this.PluginName.IsNullOrEmpty() ? this.FunctionName : $"{this.PluginName}{OpenAIFunction.NameSeparator}{this.FunctionName}"; |
or
| this.PluginName.IsNullOrEmpty() ? this.FunctionName : string.Join(OpenAIFunction.NameSeparator, this.PluginName, this.FunctionName); | |
| this.PluginName.IsNullOrEmpty() ? this.FunctionName : this.PluginName + OpenAIFunction.NameSeparator + this.FunctionName; |
or
| this.PluginName.IsNullOrEmpty() ? this.FunctionName : string.Join(OpenAIFunction.NameSeparator, this.PluginName, this.FunctionName); | |
| this.PluginName.IsNullOrEmpty() ? this.FunctionName : string.Concat(this.PluginName, OpenAIFunction.NameSeparator, this.FunctionName); |
all of which will compile down identically.
| /// <summary> | ||
| /// The role that provides information about a function call result. | ||
| /// </summary> | ||
| public static readonly AuthorRole Function = new("function"); |
There was a problem hiding this comment.
All of these System/Assistant/User/Tool/Function fields should be changed to properties, e.g.
public static AuthorRole Function { get; } = new("function");| /// <summary> | ||
| /// Dictionary for any additional message properties | ||
| /// </summary> | ||
| public IDictionary<string, string>? AdditionalContext { get; set; } |
There was a problem hiding this comment.
I realize I came up with the AdditionalContext name, but we might want to consider alternatives, like AdditionalProperties.
There was a problem hiding this comment.
I like AdditionalProperties -- avoids confusion about the term "context" with SKContext / context variables
Stephen Toub (stephentoub)
left a comment
There was a problem hiding this comment.
Thank you for fixing this.
|
Gina Triolo (@gitri-ms) As mentioned above, we need to add LLM function call request to the chat history before adding the function execution result. Please consider my suggestions below:
|
|
Michael Leung (@leungkimming) - Thanks for the suggestions, I am incorporating them and will have updated changes soon! |
|
Gina Triolo (@gitri-ms) I am sorry that there is a minor bug in my suggested coding. In the original OpenAI function calling assistance message, the Name field should be null but in my coding, the Name field contains the function name. Only the function execution result message should have a value in the Name field. Below is the correction in the ClientBase.cs file. In addition, to demonstrate multiple function calling, I would like to suggest below changes to the Example59_OpenAIFunctionCalling.cs file.
|
| { | ||
| var chatMessage = chatResult.ModelResult.GetOpenAIChatResult().Choice.Message; | ||
|
|
||
| if (!chatMessage.Content.IsNullOrEmpty()) |
There was a problem hiding this comment.
Can a chat message have both text and a function call? The code suggests that it's possible.
There was a problem hiding this comment.
SergeyMenshykh As you have quoted from OpenAI documentation,
messages.append(response_message) # extend conversation with assistant's reply
I believe that we should simply extend the response_message from LLM, regardless of its content to the chat history. The response_message may contain message or function call or both because OpenAI's API reference does not explicitly mention that they cannot co-exist. You are correct that if they both exist, the code will generate two messages instead of one. Therefore, I suggest below changes:
public static void AddAssistantMessage(this ChatHistory chatHistory, IChatResult chatResult)
{
var chatMessage = chatResult.ModelResult.GetOpenAIChatResult().Choice.Message;
var functionCall = chatMessage.FunctionCall;
if (functionCall is not null)
{
chatHistory.AddMessage(AuthorRole.Assistant, chatMessage.Content, new Dictionary<string, string>
{
{ "Name", functionCall.Name },
{ "Arguments", functionCall.Arguments }
});
} else
{
chatHistory.AddAssistantMessage(chatMessage.Content);
}
}
|
Created a new PR here: #3421 |
|
Michael Leung (@leungkimming) I have created a new PR here: #3421 to continue this work. Could you try out the latest changes (just minor updates) and let me know if it works for your scenarios. Thanks |
@ markwallace-microsoft Thankyou. It works for my scenario. However, I have a minor suggestion. Using chatResult.GetOpenAIFunctionResponse to check if not null to execute functions may have problem if OpenAI set the CompletionsFinishReason other than "stop" or "function_call". Shall we add a "GetOpenAIFinishReason" in ChatResultExtensions.cs and let us properly check below values to determine the next action to take? I am using chatResult.ModelResult.GetOpenAIChatResult().Choice.FinishReason but not quite elegant! |
…3421) ### Motivation and Context Replaces #3302 Resolves #3017 and enables the full function calling scenario as described in the [OpenAI function calling documentation](https://platform.openai.com/docs/guides/gpt/function-calling). ### Description <!-- Describe your changes, the overall approach, the underlying design. These notes will help understanding how your code works. Thanks! --> Adds the "function" author role to allow adding function messages to chat history. Function messages require a "name" parameter, so this change also adds that to chat message model. ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 --------- Co-authored-by: Gina Triolo <[email protected]>

Motivation and Context
Resolves #3017 and enables the full function calling scenario as described in the OpenAI function calling documentation.
Description
Adds the "function" author role to allow adding function messages to chat history. Function messages require a "name" parameter, so this change also adds that to chat message model.
Contribution Checklist