-
Notifications
You must be signed in to change notification settings - Fork 3.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: hard limit to functions descriptions for OAS imports #9841
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
var operation = new RestApiOperation( | ||
id: operationItem.OperationId, | ||
servers: operationServers, | ||
path: path, | ||
method: new HttpMethod(method), | ||
description: string.IsNullOrEmpty(operationItem.Description) ? operationItem.Summary : operationItem.Description, | ||
description: description?.Substring(0, Math.Min(description.Length, 1000)), |
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.
Some scenarios may benefit from this change, while others may not. We shouldn't assume that this change works for all situations that SK addresses. So, if the CopilotAgentPlugin scenarios need it, consider applying it there.
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.
Could/should the value be an option somewhere without any default? If so where?
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.
It's not specific to copilot agent plugins. Anything openapi based is going to have the same issue since this description comes from the open API document. Hence why I patched it here
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.
Also beyond a configurable option, there probably should be a hard limit for service protection.
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.
The SK OpenAPI functionality provides customization capabilities that can be extended and used for cases like this - OpenApiPlugin_Customization.
SK should tend to make as minimal assumptions about user scenarios as possible to remain maintainable and cater to a wide range of user scenarios, while simultaneously allowing for the tweaking of certain aspects for specific scenarios via customization.
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 guess my question is: what would be an acceptable API to have those sorts protections in semantic kernel (maybe off by default) which people could configure, without having to implement a decorator themselves?
this PR adds a hard limit to function descriptions to avoid hitting the limit from the model planner