-
Notifications
You must be signed in to change notification settings - Fork 851
Add InvocationRequired property to FunctionCallContent #7126
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
Conversation
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
|
@copilot, fix the CI failures |
Fixed in da8a275. The tests were reusing FunctionCallContent objects across multiple test iterations. When we set InvocationRequired=false during processing, subsequent iterations saw them as already processed. Added a ResetPlanFunctionCallStates helper method that resets InvocationRequired=true at the beginning of test helper methods. All 385 AI tests and 1115 abstraction tests now pass. |
|
@javiercn, @jozkee, @westey-m, thoughts on this approach? The goal here is to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds an InvocationRequired property to FunctionCallContent to prevent duplicate processing of function calls in the FunctionInvokingChatClient. The property defaults to true and is set to false after a function call has been processed, allowing the middleware to distinguish between new function calls and already-processed ones. This is particularly useful in scenarios where chat history containing function calls is reused across multiple requests.
Key changes:
- Added
InvocationRequiredboolean property toFunctionCallContentwith JSON serialization support (omitted when false) - Modified
FunctionInvokingChatClientto set the property to false after processing and filter out function calls withInvocationRequired = false - Added comprehensive test coverage including unit tests for serialization behavior and integration tests for filtering logic
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs | Adds InvocationRequired property with field initializer defaulting to true, marked as experimental, with JsonIgnoreCondition.WhenWritingDefault to omit when false |
| src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs | Updates CopyFunctionCalls to filter out function calls where InvocationRequired = false and sets the property to false in ProcessFunctionCallAsync and CreateFunctionResultContent |
| test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs | Adds 6 unit tests covering property defaults, getters/setters, and JSON serialization/deserialization behavior |
| test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs | Adds 4 integration tests for the filtering behavior and a ResetPlanFunctionCallStates helper method to reset the property when reusing test plans across multiple invocations |
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Assuming we will always have a The representation of a function that was already invoked already exists as the I suggest that we might just have a nullable Further downstream Often I found myself having to store the |
Could we instead look for the matching I think this goes in line with what @rogerbarreto has in mind. |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
src/Libraries/Microsoft.Extensions.AI.Abstractions/Microsoft.Extensions.AI.Abstractions.json
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Show resolved
Hide resolved
6f38322 to
30d0ee7
Compare
jozkee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot address feedback and resolve conflicts with main.
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
Outdated
Show resolved
Hide resolved
- Removed redundant tests (InvocationRequired_DefaultsToTrue, InvocationRequired_CanBeSetToFalse) - Consolidated serialization tests into single theory with true/false parameters - Consolidated deserialization tests into single theory with true/false parameters - Consolidated roundtrip tests into single theory with true/false parameters - Replaced ResetPlanFunctionCallStates with CloneContents approach to avoid sharing mutable state - All 400 AI tests + 1118 abstraction tests passing Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Addressed all test refactoring feedback in 8fa1a6b:
Note: I haven't resolved merge conflicts with main yet as there are conflicts in test files. I'll need assistance resolving those since I cannot use git commit/push directly. |
…lot/add-invocation-required-property # Conflicts: # test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs
Outdated
Show resolved
Hide resolved
- Changed InvocationRequired from "Experimental" to "Stable" in JSON file - Added Assert.True(c.InvocationRequired) to Constructor_PropsDefault test - Removed redundant InvocationRequired_InnerClientWithInvocationRequiredFalsePassesThroughUnprocessed test (covered by other test) - Converted InvocationRequired tests to theories with streaming parameter where applicable - Fixed static lambda issue in GenerateRejectedFunctionResults - All 428 AI tests + 1183 abstraction tests passing Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Show resolved
Hide resolved
- Removed redundant InvocationRequired = false assignment in CreateResponseMessages (already set in ProcessFunctionCallAsync) - Refactored InvocationRequired tests to use ternary operator with ToChatResponseAsync() to reduce code duplication - Added test in FunctionInvokingChatClientApprovalsTests to verify FunctionCallContent with InvocationRequired=false are not replaced with approval requests - Updated approval logic to skip FunctionCallContent where InvocationRequired=false - All 430 AI tests + 1183 abstraction tests passing Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
…nctionCallContent.cs
Implementation Summary
Successfully implemented the
InvocationRequiredproperty forFunctionCallContentwith all code review feedback addressed:Add
InvocationRequiredproperty toFunctionCallContentclasstrue(requires invocation)Update
FunctionInvokingChatClientto setInvocationRequired = falsewhen handling functionProcessFunctionCallAsyncbefore any processing (only location)Update
FunctionInvokingChatClientto filter out already-processed function callsCopyFunctionCallsto skipFunctionCallContentwhereInvocationRequired == falseFunctionCallContentwhereInvocationRequired == falseComprehensive test coverage (1613 tests passing)
FunctionCallContentTests.cs(refactored as theories)FunctionInvokingChatClientTests.csfor integration behaviorFunctionInvokingChatClientApprovalsTests.csfor approval flowTest refactoring per code review
All code review feedback addressed
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Microsoft Reviewers: Open in CodeFlow