ToolCallingChatOptions support internalToolExecutionMaxIterations, to limit the maximum number of tool calls and prevent infinite recursive calls to LLM in special cases

[lambochen]

This PR primarily addresses the issue of infinite recursive calls to LLM under certain special conditions, with internalToolExecutionEnabled=true in place, ref issue: #3333.

By introducing support for toolExecutionMaxIterations in ToolCallingChatOptions and performing call count checks in each ChatModel, interrupt the process when the maximum call count is reached.

[x] Sign the contributor license agreement
[x] Rebase your changes on the latest main branch and squash your commits
[x] Add/Update unit tests as needed
[x] Run a build and make sure all tests pass prior to submission

[markpollack]

thanks, one comment is that we need to preserve api compatability and

public ChatResponse internalCall(Prompt prompt, ChatResponse previousChatResponse, int attempts) {

would need to be an overload vs adding a new method parameter.

[lambochen]

thanks, one comment is that we need to preserve api compatability and

public ChatResponse internalCall(Prompt prompt, ChatResponse previousChatResponse, int attempts) {

would need to be an overload vs adding a new method parameter.

Your suggestion is great, I'll optimize it, thanks

[lambochen]

Hi, @markpollack @tzolov , the PR is ready, please review it, thank you.

If there are any issues or if additional content is needed, please to comment, and I will promptly correct or supplement it. thank you

[ThomasVitale]

I would say it's a mistake that no limit was set in the original implementation. I would consider having a lower value as the default limit. We should think of a good default value here that avoids infinite loops.

[lambochen]

Yes, I agree with your point. To ensure downward compatibility, I have used Integer.MAX_VALUE (a very large number) to indicate no limit.

I tend to prefer setting a smaller value to limit the number of executions and the scope of impact on the program, but I don't have specific comparative data. If you have suggestions, please feel free to share them. Thank you

[ThomasVitale]

@tzolov what do you think? Should we keep it unbounded (i.e infinite loop) or find a good default value?

[lambochen]

@ThomasVitale Hi, I have optimized the code and replied to your suggestion. Please re-review this PR. Thank you for your hard work.

[aichemzee]

Thank you @lambochen for picking this up. This would be a very helpful change with tool calling.

[tzolov]

Thanks @lambochen!

I agreed on handling the infinite loop risk, but, IMO, should consider:

1. What should happen when max interactions are reached? Currently it silently returns the last tool response. I think we should throw an error instead - maybe ToolExecutionException?

2. Could we introduce a ToolExecutionRequest (like ToolExecutionResult) containing the prompt, chat response, and iteration counter? We could also extend ToolExecutionResult to include the call count. This would let us centralize the check logic in executeToolCalls.

@lambochen , @ThomasVitale what do you think?

[lambochen]

Thanks! @tzolov

1. What should happen when max interactions are reached? Currently it silently returns the last tool response. I think we should throw an error instead - maybe ToolExecutionException?

I agree, should we also consider other strategies?

1. Silently return the last tool response
2. Throw a ToolExecutionException
3. Custom handler

2. Could we introduce a ToolExecutionRequest (like ToolExecutionResult) containing the prompt, chat response, and iteration counter? We could also extend ToolExecutionResult to include the call count. This would let us centralize the check logic in executeToolCalls.

That's great, introducing ToolExecutionRequest will make the logic much cleaner.

[tzolov]

@lambochen

I agree, should we also consider other strategies?

I don't think we need this flexibility right now. I can't see a use case where "silently return the last tool response" would be helpful. Also, this change needs to work with internalToolExecutionEnabled=false, and returning a random tool response could cause issues there.

Let's save strategy support for a future PR if we find compelling reasons for it.

[ThomasVitale]

I'll have a look tomorrow at the updated PR and share my comments then. Thanks!

[lambochen]

@lambochen

I agree, should we also consider other strategies?

I don't think we need this flexibility right now. I can't see a use case where "silently return the last tool response" would be helpful. Also, this change needs to work with internalToolExecutionEnabled=false, and returning a random tool response could cause issues there.

Let's save strategy support for a future PR if we find compelling reasons for it.

@tzolov Thank you for your reply, I agree with your point, I will optimize the code and resubmit, thank you

[lambochen]

2. Could we introduce a ToolExecutionRequest (like ToolExecutionResult) containing the prompt, chat response, and iteration counter? We could also extend ToolExecutionResult to include the call count. This would let us centralize the check logic in executeToolCalls.

Hi, @tzolov
I didn't quite understand your suggestion. Are you expecting to centralize all tool execution logic into ToolCallingManager.executeToolCalls, including isToolExecutionRequired and isLimitExceeded?
I hope you can clarify my confusion. Thank you.

[ThomasVitale]

Should this be a public API or a utility method?

[ThomasVitale]

Same question here as in ToolExecutionEligibilityChecker. Should this be a public API or a utility method?

I would suggest moving this to ToolCallingChatOptions as a static method, similar to other utilities we have there to deal with tool execution.

[lambochen]

Hi, @ThomasVitale ，Thank you for your comment, your considerations are very valuable.

Regarding this issue, I think both are acceptable.

As a public API, it opens up possibilities for users to extend, allowing users to be more flexible when implementing ToolExecutionEligibilityPredicate, of course, this is on the premise that users expect to implement isLimitExceeded.

As a utility method, it's also a good design that can meet the business's customization for ToolExecutionEligibilityPredicate (by implementing the isToolExecutionRequired method). It's equivalent to users only needing to be aware of the isToolExecutionRequired SPI method, which would be more concise.

What do you think?