Fix FunctionInvokingChatClient to respect ChatOptions.Tools modifications by function tools#7218
Conversation
…ions by function tools Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
@copilot, would it make more sense / be cleaner to just get rid of the map entirely? I believe we added it originally out of concern for the case where there are lots of tools and lots of function requests, but it's rare to have more than a few function requests in a single response, if we consider the number of tools to be a constant controlled by the client then we're still O(N) in the number of tool requests, and having to keep the map up to date adds even more overhead that the 90% case is having to pay. |
Per reviewer feedback, removed the Dictionary-based tool map entirely and replaced it with linear search through AdditionalTools and options.Tools. This simplifies the code and removes the overhead of maintaining/refreshing the map, which is acceptable since: - Most function invocations have only a few function calls - Tool lists are typically small - Linear search avoids dictionary allocation and synchronization overhead Added helper methods: - FindTool: Linear search for a tool by name - AnyToolsRequireApproval: Checks if any tool requires approval - HasAnyTools: Checks if there are any tools in the lists Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Good point. Removed the map entirely in fc8c227. Now using linear search through |
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.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
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
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
Show resolved
Hide resolved
|
@copilot try again |
… add approval tests - Remove unnecessary initial anyToolsRequireApproval assignment before the loop (lines 289 and 431) - Simplify FindTool to use forward iteration (earlier lists take precedence) - Add AIFunctionDeclaration check in FindTool to only match tools that can be described as functions - Simplify approval check in ReplaceFunctionCallsWithApprovalRequests using |= operator - Update all call sites to pass options.Tools first, AdditionalTools second - Add tests for approval-required functions being added and replaced during invocation Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Addressed all the review feedback in a835320:
All 70 FunctionInvokingChatClient tests pass. |
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
NeedsToolMapRefreshmethod to detect when tools have changedCreateToolsMapandNeedsToolMapRefreshmethodsFindToolhelper for linear search through tool listsAnyToolsRequireApprovalhelper to check if any tool requires approvalHasAnyToolshelper to check if there are any toolsanyToolsRequireApprovalassignment before loopsFindToolto use forward iteration (earlier lists take precedence)AIFunctionDeclarationcheck inFindToolto only match describable functions|=operatorFindToolreturn type fromAITool?toAIFunctionDeclaration?for more specific typingapprovalRequiredFunctionsdeclaration inside the loop to ensure it reflects tool modificationsOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Microsoft Reviewers: Open in CodeFlow