Bug description When using the OpenAI search models the following error is returned: Model incompatible request argument supplied: temperature
There is no way to remove the temperature
parameter from the OpenAiChatOptions programmatically without changing the global temperature.
Environment Spring AI latest snapshot
Steps to reproduce
var options = OpenAiChatOptions.builder()
.model(MODEL)
.build();
Prompt prompt = new Prompt(messages, options);
return client.prompt(prompt)
.call()
.chatResponse();
Expected behavior Either allow setting the temperature to null or remove it when a search model is used.
Comment From: LEEJaeHyeok97
Hi, I'd like to work on this issue. Currently, in OpenAiChatOptions
, there's no way to explicitly remove the temperature
parameter, which leads to incompatibility when using search models.
As a solution, I propose:
- Changing the
temperature
field inOpenAiChatOptions
toOptional<Double>
to allow explicit absence of the parameter. - Modifying the
ModelOptionsUtils.merge()
method to avoid merging default values when the field isOptional.empty()
. - Updating
OpenAiChatModel
to exclude thetemperature
parameter from the request when it'sOptional.empty()
.
Does this approach align with the project's direction? If so, I'll proceed to fork the repository, implement the changes, and submit a PR. Thank you!
Comment From: therepanic
Hi, @LEEJaeHyeok97.
I think I discovered a deeper issue (which I mentioned on this issue) that involves more flexible scenarios when we don't want the data from the properties to merge with internal data. That would solve probably many problems, including this one.
I think your solution solves this problem quite well, however it seems to me that labeling each field as Optional would be redundant for us and we can look at this problem from another angle as I described in #3254. It's not for me to decide either way though.
Comment From: LEEJaeHyeok97
Thanks @therepanic for the thoughtful perspective!
I agree that introducing a global flag like isPreventDefaultMerge
can provide broader flexibility, especially for advanced use cases where merging should be suppressed entirely.
That said, I’d like to move forward with the more localized fix for the temperature
issue first. Since this is a concrete problem with a well-defined cause, making temperature
explicitly skippable via Optional
seems like a practical and low-risk change for now.
Of course, if a more general solution (like the flag) is implemented later on, we could revisit and refactor to adopt that approach uniformly across all options.
Appreciate the feedback — let’s keep the discussion going!
Comment From: therepanic
I realize that using Optional for temperature looks like a quick way to solve the problem, and it will essentially solve it, but in my opinion it is not a good way. The problem is that wrapping every field in Optional can lead to excessive code complexity and make it harder to support in the future, especially if there are more such parameters. After all, you should agree that such a problem may occur not only with OpenaiOptions This is more like a patch than a system solution, i think. And I don't think it's what we need, it's not the kind of bug to be plugged in this way.
It seems to me that everyone will be satisfied with a reliable solution. I think it makes no sense to do one thing and then another, since both changes are of the same complexity, it's better to concentrate on one thing. On the other hand, the solution I have described is not risky or cumbersome, but it can solve several problems in a reliable way.
In any case, as I said earlier, I am not the one making the decisions, and you can try to make your own changes and the maintainers will decide how best to proceed. However, this is my point of view. Thanks.
Comment From: LEEJaeHyeok97
I completely understand your point about avoiding excessive use of Optional
fields and your idea for a broader, flag-based solution like isPreventDefaultMerge
. I agree that your approach is more systematic and extensible, and it could definitely prevent similar issues across multiple model options in the long term.
In this PR, however, my intention is to provide a minimal and low-risk fix that directly addresses the specific issue reported in #3253 — the inability to suppress the temperature
parameter during merging. I believe this patch solves the current blocking problem without introducing significant structural changes.
Of course, if the maintainers decide to implement a more general mechanism like the one you proposed, I’d be happy to help adapt this logic or refactor accordingly.
Thanks again for the insight!
While I understand the desire to design for future flexibility, at the moment this issue is only related to the temperature
field. Introducing a global isPreventDefaultMerge
flag may be more powerful, but it also introduces new complexity and may require users to manually configure all parameters when set.
Since this PR aims to fix a single well-scoped issue in a safe and practical way, I believe it offers a good short-term solution that doesn't preclude future improvements.
Comment From: LEEJaeHyeok97
Hi @hstaudacher , I’ve addressed #3253 by omitting temperature for search models and adding unit tests. Could you please take a look at PR #3315 and let me know if it meets your expectations? Thanks!
Comment From: markpollack
I didn't fully digest the issue yet, but as a note, proposed changes such as
Changing the temperature field in OpenAiChatOptions to Optional<Double> to allow explicit absence of the parameter.
is not possible in the 1.0.x line as it would be a breaking API chage.
Comment From: tzolov
@hstaudacher looks like you're trying to use an Embedding model with the Chat models API abstractions?
Check the Open AI Embedding API with the corresponding OpenAiEmbeddingOptions
:
EmbeddingResponse embeddingResponse = embeddingModel.call(
new EmbeddingRequest(List.of("Hello World", "World is big and salvation is near"),
OpenAiEmbeddingOptions.builder()
.model("Different-Embedding-Model-Deployment-Name")
.build()));