fix(MistralAI): correctly build request with default chat model options

[cponfick]

No description provided.

[tzolov]

@cponfick i've checked current implementation it seems correct to me.
Currently the run-time options (e.g. those in the prompt request) are used to override the start time (e.g. default) options. The Chat Options section in https://docs.spring.io/spring-ai/reference/api/chatmodel.html provides some information about the expected life cycle.

[cponfick]

@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.
I would expect following priority when overwriting:

1. Prompt Options overwrite Chat Model Options
2. Chat Model Options overwrite Default Options

This is also how the OpenAiChatModel handles it. Currently, the default value is used even if I set the temperature in the Model options. I think this is a bug and should be fixed.

The documentation seems to not mention the ChatModel-Options. Maybe the documentation needs extension?

[tzolov]

@cponfick the

but you can also set the options in the chat model constructor
are the the default options.

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.
The Runtime options should override the Startup/Default options.
The Chat Model Options you are referring above are in fact the Default Options.

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?

[cponfick]

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 OpenAPIChatModel passes!

@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 MistralAiChatModel.java and how merge(Object source, Object target, Class<T> clazz) is used.

Currently:

request = ModelOptionsUtils.merge(request, this.defaultOptions, MistralAiApi.ChatCompletionRequest.class);

Why is the request the source and defaultOptions the target? This seems wrong to me. The request should not be the source.

The merge flow should be as follows:

1. The request is created.
2. If there are defaultOptions, they are merged into request.
3. If there are prompt options, they are merged into the request and possible overwrite the prev. merged defaultOptions.

Hence, I proposed following change:

request = ModelOptionsUtils.merge(this.defaultOptions, request, MistralAiApi.ChatCompletionRequest.class);

[ThomasVitale]

@cponfick thanks for your contribution! We recently updated the way to build options across all models and it looks like this issue has been addressed as part of that change: https://github.com/spring-projects/spring-ai/blob/main/models/spring-ai-mistral-ai/src/main/java/org/springframework/ai/mistralai/MistralAiChatModel.java#L472 There was indeed an error in the sequence used to pass arguments to ModelOptionsUtils.

[ilayaperumalg]

@cponfick Please re-open this if you still see the issue. Thanks

[cponfick]

@ilayaperumalg It seems to work as expected.

[ilayaperumalg]

@cponfick Thanks for confirming!