-
Notifications
You must be signed in to change notification settings - Fork 4.3k
.Net: Add RetainArgumentTypes
support to FunctionCallContentBuilder
#13142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
Before this patch, the `FunctionCallContentBuilder` did not support retaining the argument types. We need this to support retained argument types when not using auto invoke function choices.
76d98f9
to
9bf7fae
Compare
The Can reproduce by spamming the following until it races: dotnet test dotnet\src\Connectors\Connectors.AzureOpenAI.UnitTests --filter "(FullyQualifiedName=SemanticKernel.Connectors.AzureOpenAI.UnitTests.KernelCore.KernelTests.FunctionUsageMetricsAreCapturedByTelemetryAsExpected) | (FullyQualifiedName=SemanticKernel.Connectors.AzureOpenAI.UnitTests.Services.AzureOpenAIChatCompletionServiceTests.GetChatMessageContentsUsesPromptAndSettingsCorrectlyAsync)" --no-build
Can also reproduce from VS by running these two tests in parallel until failure. |
Head branch was pushed to by a user without write access
Find a fix in e08e4d1. This bit of code can be triggered concurrently (from two parallel tests). The semantic-kernel/dotnet/src/Connectors/Connectors.AzureOpenAI.UnitTests/KernelCore/KernelTests.cs Lines 83 to 90 in e08e4d1
Fix the problem by using a thread-safe collection, |
Motivation and Context
Before this patch, the
FunctionCallContentBuilder
did not support retaining the argument types. We need this to support retained argument types when not using auto invoke function choices, i.e: a custom agent loop.Figured this was small enough to not warrant an issue.
Description
Modified the experimental constructor to include a
bool retainArgumentTypes
parameter. I decided to update the existing experimental constructor to avoid polluting the class with extra constructors. Not sure if this is considered breaking.Contribution Checklist