-
Notifications
You must be signed in to change notification settings - Fork 743
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
fix(MistralAI): correctly build request with default chat model options #958
base: main
Are you sure you want to change the base?
Conversation
@cponfick i've checked current implementation it seems correct to me. |
@tzolov Yes, that is correct, but you can also set the options in the chat model constructor (see the test case): final MistralAiChatOptions.Builder optionsBuilder = MistralAiChatOptions.builder()
.withModel("model-name")
.withTemperature(0);
new MistralAiChatModel(new MistralAiApi(mistralAiApiKey), optionsBuilder.build()); In that case the default value of temperature is used and not 0.
This is also how the The documentation seems to not mention the |
@cponfick the
There are only 2 set of options. Startup (or default) options you pass via the constructor and Runtime options you can pass via the Prompt. If you use auto-configuration, later will converts the application properties into default options passed via the OpenAiChatModel constructor. What is particular application use case that the current merging model can not cover? |
In the current implementation, the test I provided fails. The same test with an @Test
void buildCorrectRequestFromChatModelWithOptions() {
var options = MistralAiChatOptions.builder()
.withModel(MistralAiApi.ChatModel.SMALL.getValue())
.withTemperature(0.0f)
.withTopP(0.0f)
.withMaxTokens(1)
.build();
var chatModelWithOptions = new MistralAiChatModel(new MistralAiApi("test"), options);
var request = chatModelWithOptions.createRequest(new Prompt("test content"), false);
assertThat(request.messages()).hasSize(1);
assertThat(request.topP()).isEqualTo(options.getTopP());
assertThat(request.temperature()).isEqualTo(options.getTemperature());
} Taking a look at Currently: request = ModelOptionsUtils.merge(request, this.defaultOptions, MistralAiApi.ChatCompletionRequest.class); Why is the The merge flow should be as follows:
Hence, I proposed following change: request = ModelOptionsUtils.merge(this.defaultOptions, request, MistralAiApi.ChatCompletionRequest.class); |
No description provided.