spring-projects / spring-ai

An Application Framework for AI Engineering
https://docs.spring.io/spring-ai/reference/index.html
Apache License 2.0
3.19k stars 810 forks source link

[AzureOpenAiChatOptions] Newly added options are null checked in .with* builder methods and are made indirectly mandatory #1417

Closed pradu3 closed 1 month ago

pradu3 commented 1 month ago

Bug description The Assert.notNull checks in AzureOpenAiChatOptions.Builder.with* methods make the options actually mandatory. AzureOpenAiChatOptions.fromOptions is called from AzureOpenAiChatModel.getDefaultOptions. When the AzureOpenAiChatModel.defaultOption attribute doesn't have all the checked fields set (e.g. seed, enhancements), there will be an exception when AzureOpenAiChatModel.getDefaultOptions. This will cause, for example, ChatClient.create to fail. This happens even when the AzureOpenAiChatModel( OpenAIClient microsoftOpenAiClient) constructor is used.

New options introduced by the fix of https://github.com/spring-projects/spring-ai/issues/889 are the one causing this.

Environment 1.0.0-SNAPSHOT

Steps to reproduce call ChatClient.create(new AzureOpenAiChatModel(microsoftOpenAiClient)) and it will throw an exception:

Caused by: java.lang.IllegalArgumentException: responseFormat must not be null
    at org.springframework.util.Assert.notNull(Assert.java:172) ~[spring-core-6.1.13.jar:6.1.13]
    at org.springframework.ai.azure.openai.AzureOpenAiChatOptions$Builder.withSeed(AzureOpenAiChatOptions.java:297) ~[spring-ai-azure-openai-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.azure.openai.AzureOpenAiChatOptions.fromOptions(AzureOpenAiChatOptions.java:525) ~[spring-ai-azure-openai-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.azure.openai.AzureOpenAiChatModel.getDefaultOptions(AzureOpenAiChatModel.java:143) ~[spring-ai-azure-openai-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.azure.openai.AzureOpenAiChatModel.getDefaultOptions(AzureOpenAiChatModel.java:100) ~[spring-ai-azure-openai-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.chat.client.DefaultChatClient$DefaultChatClientRequestSpec.<init>(DefaultChatClient.java:722) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.chat.client.DefaultChatClientBuilder.<init>(DefaultChatClientBuilder.java:62) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.chat.client.ChatClient.builder(ChatClient.java:72) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.chat.client.ChatClient.create(ChatClient.java:63) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.chat.client.ChatClient.create(ChatClient.java:58) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
    at org.springframework.ai.chat.client.ChatClient.create(ChatClient.java:54) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]

Expected behavior No null checks should be enforced for non mandatory options. Since we would anyhow ignore null options with @JsonInclude(Include.NON_NULL) on AzureOpenAiChatOptions, there is no added value for having those checks on .with* methods.

Minimal Complete Reproducible example

@Test
    void fromOptionsCheckWithNullValuesExpectSuccess() {
        var sourceOptions = AzureOpenAiChatOptions.builder().withMaxTokens(100).withPresencePenalty(0.5).build();

        var targetOptions = AzureOpenAiChatOptions.fromOptions(sourceOptions);

        assertThat(targetOptions.getMaxTokens()).isEqualTo(100);
        assertThat(targetOptions.getPresencePenalty()).isEqualTo(0.5);
        assertThat(targetOptions.getFrequencyPenalty()).isNull();
        assertThat(targetOptions.getResponseFormat()).isNull();
        assertThat(targetOptions.getSeed()).isNull();

    }
pradu3 commented 1 month ago

PR proposed with fixes: https://github.com/spring-projects/spring-ai/pull/1418

pradu3 commented 1 month ago

obsolete, replaced by reopening of https://github.com/spring-projects/spring-ai/issues/889