Fixing a FunctionCallback inside a container that pollutes the global model's ChatOptions
The SpingBoot autoconfiguration class adds the container's FunctionCallback directly to the model's ChatOptions, which results in the FunctionCallback being included in the request each time it is called.
The modification registers the container's FunctionCallback directly to the model's functionCallbackRegister.
@Component
class ChatService(
private val chatClient: ChatClient,
) {
fun chat(message: String): ChatClient.CallResponseSpec =
chatClient.prompt().user(message).call()
}
function: ```kotlin class DateFunction : FunctionCallback { @JvmRecord data class Request(val unit: Unit?) override fun getName(): String { return "dateFunction" }
override fun getDescription(): String {
return "get current time"
}
override fun getInputTypeSchema(): String {
return ModelOptionsUtils.getJsonSchema(Request::class.java,false)
}
override fun call(functionInput: String?): String {
println("dateFunction : $functionInput")
return "the current time is${LocalDateTime.now()}"
}
} ```
The code above defines a function that is not declared for use in the request, but it gets called.
input:
dateFunction : {}
Comment From: tzolov
which results in the FunctionCallback being included in the request each time it is called. You mean that the functions gets re-registered on each request not enabled by default? They are not enabled by default? If that is not true can you please add integration test to reproduce the issue?
IMO, we need to add a List<FunctionCallback> toolFunctionCallbacks argument to the AbstractToolCallSupport constructor and then move the
Map<String, FunctionCallback> toolFunctionCallbackMap = toolFunctionCallbacks.stream()
.collect(Collectors.toMap(FunctionCallback::getName, Function.identity(), (a, b) -> b));
chatModel.getFunctionCallbackRegister().putAll(toolFunctionCallbackMap);
Inside the AbstractToolCallSupport constructor.
What do you think?
Comment From: KAMO030
- I was trying to reproduce the problem in the autoconfigure module, but the Huggingface module seems to be refactoring and causing the compilation to fail, is there a better solution other than excluding this module?
- I think adding that constructor would make the model's constructor too complex, I think we can change
getFunctionCallbackRegister()to the following
AbstractToolCallSupport:
public void registerFunctionCallback(List<FunctionCallback> functionCallbacks) {
Map<String, FunctionCallback> toolFunctionCallbackMap = functionCallbacks.stream()
.collect(Collectors.toMap(FunctionCallback::getName, Function.identity(), (a, b) -> b));
this.functionCallbackRegister.putAll(toolFunctionCallbackMap);
}
AutoConfiguration:
//....
if (!CollectionUtils.isEmpty(toolFunctionCallbacks)) {
chatModel.registerFunctionCallback(toolFunctionCallbacks);
}
//....
And I think it's better to remove getFunctionCallbackRegister() to prevent external modifications to the map.
Secondly, I think the change to
functionCallbackRegisterhas to take into account theisRuntimeCallissue
Comment From: KAMO030
@tzolov Hello, in order to verify my suspicions, I wrote a demo program to try to reproduce the problem and found that it only occurs when using ChatClient, but not when using only ChatModel:
ChatModel:
@EnabledIfEnvironmentVariable(named = "OPENAI_API_KEY", matches = ".*")
public class FunctionCallbackPollutesTest {
private final Logger logger = LoggerFactory.getLogger(FunctionCallbackPollutesTest.class);
private final ApplicationContextRunner contextRunner = new ApplicationContextRunner()
.withPropertyValues("spring.ai.openai.apiKey=" + System.getenv("OPENAI_API_KEY"))
.withPropertyValues("spring.ai.openai.baseUrl=" + System.getenv("OPENAI_BASE_URL"))
.withBean("MockWeatherService", FunctionCallback.class,()->
FunctionCallbackWrapper.builder(new MockWeatherService())
.withName("MockWeatherService")
.withDescription("Get the weather in location")
.withResponseConverter((response) -> "" + response.temp() + response.unit())
.build())
.withBean(MockWeatherService.class)
.withConfiguration(AutoConfigurations.of(OpenAiAutoConfiguration.class));
@Test
void functionCallTest() {
contextRunner
.withPropertyValues("spring.ai.openai.chat.options.model=gpt-4o",
"spring.ai.openai.chat.options.temperature=0.1")
.run(context -> {
OpenAiChatModel chatModel = context.getBean(OpenAiChatModel.class);
UserMessage userMessage = new UserMessage(
"What's the weather like in San Francisco, Tokyo, and Paris?");
var promptOptions = OpenAiChatOptions.builder()
.build();
ChatResponse response = chatModel.call(new Prompt(List.of(userMessage), promptOptions));
logger.info("Response: {}", response.getResult().getOutput().getContent());
});
}
}
logResult:
Response: I don't have real-time capabilities to provide current weather updates. However, you can easily check the weather for San Francisco, Tokyo, and Paris using a weather website or app like Weather.com, AccuWeather, or even a quick search on Google. If you need historical weather data or general climate information, I can help with that!
ChatClient:
contextRunner
.withPropertyValues("spring.ai.openai.chat.options.model=gpt-4o","spring.ai.openai.chat.options.temperature=0.1")
.run(context -> {
OpenAiChatModel chatModel = context.getBean(OpenAiChatModel.class);
String textContent = "What's the weather like in San Francisco, Tokyo, and Paris?";
ChatClient client = ChatClient.builder(chatModel).build();
String content = client.prompt().user(textContent).call().content();
logger.info("Response: {}", content);
});
logResult: Response: The current weather is as follows: - San Francisco, CA: 30.0°C - Tokyo, Japan: 10.0°C - Paris, France: 15.0°C
The reason for this is that the defaultOptions property of DefaultChatClient's defaultRequest copies the defaultOptions property of ChatModel, so if the autoconfiguration class adds FunctionCallback directly to ChatModel‘s defaultOptions property, then it is equivalent to all DefaultChatClient’s defaultRequest's chatOptions property being added.
if (!CollectionUtils.isEmpty(toolFunctionCallbacks)) {
chatProperties.getOptions().getFunctionCallbacks().addAll(toolFunctionCallbacks);
}
ChatModel.defaultOptions-(copy)->DefaultChatClient.defaultRequest.defaultOptions-(call)->Prompt
Comment From: KAMO030
Afterwards, I debugged carefully and found that the reason the above issue didn't occur when using ChatModel directly is because the isRuntimeCall parameter in the handleFunctionCallbackConfigurations method called when the model merges the defaultOptions during the method call is false,which causes it not to be added to the return value of the functionToCall Set:
options.getFunctionCallbacks().stream().forEach(functionCallback -> {
// Register the tool callback.
if (isRuntimeCall) {
this.functionCallbackRegister.put(functionCallback.getName(), functionCallback);
}
else {
this.functionCallbackRegister.putIfAbsent(functionCallback.getName(), functionCallback);
}
// Automatically enable the function, usually from prompt callback.
if (isRuntimeCall) {
functionToCall.add(functionCallback.getName());
}
});
So that following calls to the resolveFunctionCallbacks(Set<String> functionNames) method don't incorrectly add the FunctionCallback from defaultOptions to the return value.
At this point, I'm thinking: what exactly does the isRuntimeCall parameter do.
Found that in the source code it is true when merging options for this request and false when merging defaultOptions:
if (prompt.getOptions() != null) {
// ...
Set<String> promptEnabledFunctions = this.handleFunctionCallbackConfigurations(updatedRuntimeOptions,
IS_RUNTIME_CALL);
// ...
}
if (this.defaultOptions != null) {
// ...
Set<String> defaultEnabledFunctions = this.handleFunctionCallbackConfigurations(this.defaultOptions,
!IS_RUNTIME_CALL);
// ...
}
So I'd like to ask: what is the special consideration for the isRuntimeCall parameter? Is it possible to simplify the logic?
And I think this leads to a new bug, due to the following code:
// Register the tool callback.
if (isRuntimeCall) {
this.functionCallbackRegister.put(functionCallback.getName(), functionCallback);
}
else {
this.functionCallbackRegister.putIfAbsent(functionCallback.getName(), functionCallback);
}
When functionCallback has the same name, the options of the merged request will be directly put, thus affecting other requests:
@EnabledIfEnvironmentVariable(named = "OPENAI_API_KEY", matches = ".*")
public class FunctionCallbackPollutesTest {
private final Logger logger = LoggerFactory.getLogger(FunctionCallbackInPromptIT.class);
private final ApplicationContextRunner contextRunner = new ApplicationContextRunner()
.withPropertyValues("spring.ai.openai.apiKey=" + System.getenv("OPENAI_API_KEY"))
.withPropertyValues("spring.ai.openai.baseUrl=" + System.getenv("OPENAI_BASE_URL"))
.withBean("MockWeatherService", FunctionCallback.class,()->
FunctionCallbackWrapper.builder(new MockWeatherService())
.withName("MockWeatherService")
.withDescription("Get the weather in location")
.withResponseConverter((response) -> "" + response.temp() + response.unit())
.build())
.withBean(MockWeatherService.class)
.withConfiguration(AutoConfigurations.of(OpenAiAutoConfiguration.class));
@Test
void functionCallTest() {
contextRunner
.withPropertyValues("spring.ai.openai.chat.options.model=gpt-4o",
"spring.ai.openai.chat.options.temperature=0.1")
.run(context -> {
OpenAiChatModel chatModel = context.getBean(OpenAiChatModel.class);
FunctionCallback callback = FunctionCallbackWrapper.builder(new MockWeatherService())
.withName("MockWeatherService")
.withDescription("Get the weather in location")
.withResponseConverter((response) -> "I dont know")
.build();
UserMessage userMessage = new UserMessage(
"What's the weather like in San Francisco, Tokyo, and Paris?");
var promptOptions = OpenAiChatOptions.builder()
.withFunctionCallbacks(List.of(callback))
.build();
ChatResponse response = chatModel.call(new Prompt(List.of(userMessage), promptOptions));
logger.info("Response: {}", response.getResult().getOutput().getContent());
var promptOptions2 = OpenAiChatOptions.builder()
.withFunctions(Set.of("MockWeatherService"))
.build();
ChatResponse response2 = chatModel.call(new Prompt(List.of(userMessage), promptOptions2));
logger.info("Response: {}", response2.getResult().getOutput().getContent());
});
}
}
logResult: * Response:Response: It seems there was an issue retrieving the weather information for San Francisco, Tokyo, and Paris. Let's try again later. * Response:Response: It seems there was an issue retrieving the weather information for San Francisco, Tokyo, and Paris. Let's try again later.
If you remove the first call you can use the FunctionCallback defined in the ioc container:
// var promptOptions = OpenAiChatOptions.builder()
// .withFunctionCallbacks(List.of(callback))
// .build();
// ChatResponse response = chatModel.call(new Prompt(List.of(userMessage), promptOptions));
// logger.info("Response: {}", response.getResult().getOutput().getContent());
var promptOptions2 = OpenAiChatOptions.builder()
.withFunctions(Set.of("MockWeatherService"))
.build();
ChatResponse response2 = chatModel.call(new Prompt(List.of(userMessage), promptOptions2));
logger.info("Response: {}", response2.getResult().getOutput().getContent());
logResult: Response: The current weather is as follows: - San Francisco, CA: 30.0°C - Tokyo, Japan: 10.0°C - Paris, France: 15.0°C
In this case (including concurrent calls) a FunctionCallback with the same name can be confusing; we should disable the registration of FunctionCallbacks with the same name; extract the registration logic into a separate method, rather than adding it to options as the default FunctionCallback.
Or we could use a
ThreadLocalto temporarily store theFunctionCallbackdefined for the current request, instead of putting it directly into thefunctionCallbackRegister.
Finally this is where the isRuntimeCall parameter becomes unnecessary.
Then, if we remove this parameter, we'll get the bug I guessed at the beginning.
What do you think?
Comment From: tzolov
Thank you for the exploration and suggestions @KAMO030
I made some changes during the review to improve consistency and reduce repetition. Feel free to review the changes and raise another issue if necessary.
extended, rebased, squashed and merged at 7c800f35b8c590c4ec44ba0cd3afb8ccd1be2e18