-
Notifications
You must be signed in to change notification settings - Fork 3k
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: Fix a bunch of function calling issues #4258
Merged
stephentoub
merged 5 commits into
microsoft:main
from
stephentoub:functioncallingtweaks
Dec 15, 2023
Merged
.Net: Fix a bunch of function calling issues #4258
stephentoub
merged 5 commits into
microsoft:main
from
stephentoub:functioncallingtweaks
Dec 15, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
stephentoub
commented
Dec 14, 2023
•
edited
Loading
edited
- A recent change caused the execution settings passed into the chat completion service to be null, causing automatic function calling to stop working. This fixes that.
- We were providing the original execution settings to child invocations. This isn't appropriate, as the settings were known to be relevant to the parent invocation but not necessarily to the child invocation. It's also possibly dangerous, since the settings are known to enable function calling, and that could lead to recursive function invocation ad nauseum. For now, better to just explicitly not pass down the execution settings.
- We weren't validating that a function the model requested was actually one we told it about. We should be doing so; otherwise, it could end up hallucinating and invoking functions that do exist in the kernel but that we didn't ask it to use.
- We weren't validating with EnableFunctions + auto-invoke that the supplied functions were actually in the kernel; if they're not, we're setting ourselves up for failure, as we won't be able to invoke such a function when the model requests it.
- We weren't sending back error messages to the model. Every tool request needs a response.
- We weren't protecting against runaway cases where the invocation of a prompt function triggered auto-invocation of a prompt function ad nauseum. With (2), it's now hard to get into such a situation, but if we do, this adds a backstop that limits the length of that chain and disables auto-invocation if it's hit.
- We were logging the function requests after validating them rather than before, but from a logging perspective, it's better to get that information promptly.
- The "required" function calling support (where you can force the model to request a particular function's invocation) isn't behaving as expected: it's sending back a tool request but with a "stop" finish reason and it then balks at attempts to send back a function response. Until we have a better understanding of why this is happening and the right recourse, I've marked the RequireFunction entrypoint to it as experimental.
1. A recent change caused the execution settings passed into the chat completion service to be null, causing automatic function calling to stop working. This fixes that. 2. We were providing the original execution settings to child invocations. This isn't appropriate, as the settings were known to be relevant to the parent invocation but not necessarily to the child invocation. It's also possibly dangerous, since the settings are known to enable function calling, and that could lead to recursive function invocation ad nauseum. For now, better to just explicitly not pass down the execution settings. 3. We weren't validating that a function the model requested was actually one we told it about. We should be doing so; otherwise, it could end up hallucinating and invoking functions that do exist in the kernel but that we didn't ask it to use. 4. We weren't protecting against runaway cases where the invocation of a prompt function triggered auto-invocation of a prompt function ad nauseum. With (2), it's now hard to get into such a situation, but if we do, this adds a backstop that limits the length of that chain and disables auto-invocation if it's hit. 5. We were logging the function requests after validating them rather than before, but from a logging perspective, it's better to get that information promptly. 6. The "required" function calling support (where you can force the model to request a particular function's invocation) isn't behaving as expected: it's sending back a tool request but with a "stop" finish reason and it then balks at attempts to send back a function response. Until we have a better understanding of why this is happening and the right recourse, I've marked the RequireFunction entrypoint to it as experimental.
5990185
to
cc9eeb3
Compare
markwallace-microsoft
approved these changes
Dec 14, 2023
RogerBarreto
approved these changes
Dec 14, 2023
There's no additional work being done at the call site, so we don't need the same guard the subsequent LogXx check will do.
21ba160
to
fbc16d0
Compare
RogerBarreto
approved these changes
Dec 15, 2023
- Ensure that EnableFunctions functions are available in the kernel if auto-invocation was requested. - Ensure we always send back a response to a tool call, even if it's an error. Otherwise, the chat history is invalid. This also provides built-in error recovery, e.g. if the model hallucinates and requests an invalid tool, we'll now tell it that.
This was referenced Dec 15, 2023
markwallace-microsoft
approved these changes
Dec 15, 2023
Kevdome3000
pushed a commit
to Kevdome3000/semantic-kernel
that referenced
this pull request
Dec 15, 2023
1. A recent change caused the execution settings passed into the chat completion service to be null, causing automatic function calling to stop working. This fixes that. 2. We were providing the original execution settings to child invocations. This isn't appropriate, as the settings were known to be relevant to the parent invocation but not necessarily to the child invocation. It's also possibly dangerous, since the settings are known to enable function calling, and that could lead to recursive function invocation ad nauseum. For now, better to just explicitly not pass down the execution settings. 3. We weren't validating that a function the model requested was actually one we told it about. We should be doing so; otherwise, it could end up hallucinating and invoking functions that do exist in the kernel but that we didn't ask it to use. 4. We weren't validating with EnableFunctions + auto-invoke that the supplied functions were actually in the kernel; if they're not, we're setting ourselves up for failure, as we won't be able to invoke such a function when the model requests it. 5. We weren't sending back error messages to the model. Every tool request needs a response. 6. We weren't protecting against runaway cases where the invocation of a prompt function triggered auto-invocation of a prompt function ad nauseum. With (2), it's now hard to get into such a situation, but if we do, this adds a backstop that limits the length of that chain and disables auto-invocation if it's hit. 7. We were logging the function requests after validating them rather than before, but from a logging perspective, it's better to get that information promptly. 8. The "required" function calling support (where you can force the model to request a particular function's invocation) isn't behaving as expected: it's sending back a tool request but with a "stop" finish reason and it then balks at attempts to send back a function response. Until we have a better understanding of why this is happening and the right recourse, I've marked the RequireFunction entrypoint to it as experimental. (cherry picked from commit b619e84)
zengin
pushed a commit
to microsoftgraph/semantic-kernel
that referenced
this pull request
Jan 5, 2024
1. A recent change caused the execution settings passed into the chat completion service to be null, causing automatic function calling to stop working. This fixes that. 2. We were providing the original execution settings to child invocations. This isn't appropriate, as the settings were known to be relevant to the parent invocation but not necessarily to the child invocation. It's also possibly dangerous, since the settings are known to enable function calling, and that could lead to recursive function invocation ad nauseum. For now, better to just explicitly not pass down the execution settings. 3. We weren't validating that a function the model requested was actually one we told it about. We should be doing so; otherwise, it could end up hallucinating and invoking functions that do exist in the kernel but that we didn't ask it to use. 4. We weren't validating with EnableFunctions + auto-invoke that the supplied functions were actually in the kernel; if they're not, we're setting ourselves up for failure, as we won't be able to invoke such a function when the model requests it. 5. We weren't sending back error messages to the model. Every tool request needs a response. 6. We weren't protecting against runaway cases where the invocation of a prompt function triggered auto-invocation of a prompt function ad nauseum. With (2), it's now hard to get into such a situation, but if we do, this adds a backstop that limits the length of that chain and disables auto-invocation if it's hit. 7. We were logging the function requests after validating them rather than before, but from a logging perspective, it's better to get that information promptly. 8. The "required" function calling support (where you can force the model to request a particular function's invocation) isn't behaving as expected: it's sending back a tool request but with a "stop" finish reason and it then balks at attempts to send back a function response. Until we have a better understanding of why this is happening and the right recourse, I've marked the RequireFunction entrypoint to it as experimental.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.