Skip to content

fix: Handling default implementation of ToolCallback#call(String,ToolContext) #3784

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ilayaperumalg
Copy link
Member

Context: Currently, the default implementation of ToolCallback#call(String,ToolContext) throws exception when an non-empty ToolContext is passed. This gives bad user experience when the application has multiple tool callbacks with one of them doesn't use toolcontext but the chat client passes non-empty toolcontext. For more information ##3389

  • This fix removes the explicit exception when the toolcontext is non empty for the default implementation of call(String, ToolContext)

    • Instead, log debug message that tool context will be ignored
  • Also, add explicit null check at the DefaultToolCallingManager when invoking ToolCallback's methods

Resolves #3389

…Context)

  Context: Currently, the default implementation of ToolCallback#call(String,ToolContext) throws exception when an non-empty ToolContext is passed. This gives bad user experience when the application has multiple tool callbacks with one of them doesn't use toolcontext but the chat client passes non-empty toolcontext. For more information #spring-projects#3389

  - This fix removes the explicit exception when the toolcontext is non empty for the default implementation of call(String, ToolContext)
    - Instead, log debug message that tool context will be ignored

  - Also, add explicit null check at the DefaultToolCallingManager when invoking ToolCallback's methods

Resolves spring-projects#3389

Signed-off-by: Ilayaperumal Gopinathan <[email protected]>
Copy link
Contributor

@filiphr filiphr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this changes @ilayaperumalg. I've left a comment. The toolContext can never be null, which means the if check is entirely obsolete. Additionally, this only solves #3389 partially.

As a user it is still cumbersome to implement a ToolCallback which needs to use the ToolContext. I need to override the call(String) method and the call(String, ToolContext) method. Is there a reason why you are against providing a new interface ToolContextAwareToolCallback that would look like:

public interface ToolContextAwareToolCallback extends ToolCallback {

	@Override
	default String call(String toolInput) {
		throw new UnsupportedOperationException("This method is not supported. Use call(String toolInput, ToolContext toolContext) instead.");
	}

	@Override
	String call(String toolInput, @NonNull ToolContext toolContext);
}

This interface is using the same pattern as other Spring ecosystem implementations.

@@ -217,7 +217,12 @@ private InternalToolExecutionResult executeToolCall(Prompt prompt, AssistantMess
.observe(() -> {
String toolResult;
try {
toolResult = toolCallback.call(toolInputArguments, toolContext);
if (toolContext != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can never be null. The toolContext cromes from buildToolContext, which never returns null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. This can never be null and I will remove this unnecessary check.

@ilayaperumalg
Copy link
Member Author

Is there a reason why you are against providing a new interface ToolContextAwareToolCallback

We want to have a solution that can work for 1.0.x as well. Obviously, the new interface can only exist in 1.1.x.

The simplest option is to ignore the ToolContext in the default implementation of call(String,ToolContext).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ToolCallback fails if tool context is provided
3 participants