-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Wrong implementation of MethodToolCallback.validateToolContextSupport #3466
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
Comments
…lidateToolContextSupport` method caused by incorrect parameter order. Signed-off-by: Sun Yuhan <[email protected]>
Hi @jcgouveia , I checked the code and I agree with you and pushed a PR to try to fix it :#3478, can you help to review this PR?thanks. |
@jcgouveia Thanks for reporting the issue and the detailed writeup! Please feel free to contribute your suggestions as pull request as well. @sunyuhan1998 Thanks for the PR! |
…od caused by incorrect parameter order. Fixes #GH-3466 Signed-off-by: Sun Yuhan <[email protected]>
…od caused by incorrect parameter order. Fixes #GH-3466 Signed-off-by: Sun Yuhan <[email protected]>
Hi @jcgouveia @sunyuhan1998, I want to bring up a specific use case into discussion related to the logic applied as part of this change. With this tool context validation check, if a chat client has multiple Method tool callbacks but one of them doesn't need tool context to be processed, with a non-empty tool context value, the validation would fail. |
I think I understand what you mean. I believe the key issue lies in the following code: Lines 222 to 230 in 5cb5c90
It does not take into account whether the I think we have two ways to solve this issue:
I prefer the second approach because the first one violates the definition in the I think I can submit a PR to try to fix this issue, do we need to open a new issue? |
…validateToolContextSupport` method failed to validate correctly when some `MethodToolCallback` instances accepted a `ToolContext` parameter while others did not, even though the `ToolContext` itself was not null during model request invocation. Signed-off-by: Sun Yuhan <[email protected]>
Hi @ilayaperumalg , I have submitted an additional PR #3521 to try and fix the issue, could you help review it?thanks. |
Uh oh!
There was an error while loading. Please reload this page.
Consider the current implementation of validateToolContextSupport compared from 1.0.0 and 1.0.0-M6
M6
Both have the same serious bug also refered in The isFunctionalType method of MethodToolVNet Provider is incorrect, resulting in some cases where the tool cannot be registered #3355 , it seems that the writer does not know the semantics of ClassUtils.isAssignable(left, right) => right is assinable to left, so left is the "super" type. All this code has the arguments in incorrect order.
More, in the case of a toolContext not being provided, the change in the validation logic now make it fail due to error 1
M6 : No context
=> isToolContextRequired = false
=> isToolContextRequired && !isToolContextAcceptedByMethod = false => no exception
1.0.0 : No context
=> isNonEmptyToolContextProvided = false + isNonEmptyToolContextProvided = true (because of error 1)
=> isToolContextAcceptedByMethod && !isNonEmptyToolContextProvided = true
=> exception is thrown !
The text was updated successfully, but these errors were encountered: