Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
.Net: Tool filters #4922
.Net: Tool filters #4922
Changes from 20 commits
610e7e7
39b125a
95156bc
e9b61fd
4f9a1cd
f263823
699b11d
efbfe78
29fdedc
865f3e5
a7e6e45
6172aa5
1f5752f
3dee7d6
f7cd9f7
037a996
a709123
363a9aa
bddcf05
cbdf008
09d7c86
46e8af1
cb3467e
85aeeff
087b307
a1012bc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is being called every time a filter is called right? Is that to handle the case of the filter handler changes the history in some way and we need to sync it? Do we do any updates to the history to clean up if the tool call is canceled?
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.
Yes and yes. If the filter changes the chat history, we would want that updated chat history to be included in the next request to the model. This helper function updates the chat history stored in
chatOptions
(which is what is sent to the model) to match what is in thechatHistory
object.If the tool call is cancelled before invocation, instead of a response message, we add an error message to the chat history. If the tool call is cancelled after invocation, I just have it keeping the regular tool response message in the chat history, but bailing out of future tool calls. (I am not necessarily opposed to a different behavior here, but this made the most sense to me.) In both these cases,
AddResponseMessage
handles updating both thechatHistory
andchatOptions
objects.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.
There's a lot that happens between here and the actual function invocation, including actually getting the corresponding KernelFunction object and creating the KernelArguments to pass to it. I see the value of a callback that's really early in the process, so that a callback can see the raw request from the model, but in that case, should it actually be even earlier and be passed just the raw string name and string arguments? And then have a separate callback that's invoked with the Kernel, KernelFunction, KernelArguments, etc. just before function.InvokeAsync is actually called?
I think it'd be helpful to enumerate all the extensibility scenarios we're trying to enable here, i.e. the different things we expect folks will want to do with this, and then write out the example code for each, showing both that it's possible and what the code would look like. Those can all become samples, too.
For example:
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.
This feels a bit icky to me. Does this mean that we're saying the way you early-exit non-exceptionally is to throw an OperationCanceledException?
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.
Are we going to expose this configuration?
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.
I would argue that should be in a separate PR, since it's unrelated to the tool filters or the planner updates that this PR covers. (I don't mind creating that PR though, should be fairly quick.) Also, if we expose this field, do we want to expose
ToolCallBehavior.MaximumUseAttempts
as well?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.
I don't think we want this here, at least not as a public mutable list. It means any code can just do
ToolCallBehavior.AutoInvokeKernelFunctions.Filters.Add(myFilter)
, and it'll be added to the singleton that will apply to everyone, which also means you need to remember to remove filters after you're done with them, even in the case of exception.I think instead we should add overloads to the existing factories below, e.g.
or something along those lines. That's just a sketch; names and overall shape are debatable.