Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[AzureOpenAiChatOptions] Unable to set all possible options when calling AzureOpenAiChatModel #889

Closed
RikJux opened this issue Jun 18, 2024 · 3 comments
Closed

[AzureOpenAiChatOptions] Unable to set all possible options when calling AzureOpenAiChatModel #889

RikJux opened this issue Jun 18, 2024 · 3 comments
Assignees
@sobychacko @tzolov
Labels
azure chat options model client
Milestone
1.0.0-M3

Comments

@RikJux
Copy link

RikJux commented Jun 18, 2024 •
edited
Loading

Bug description
Consider the attributes that can be set through AzureOpenAiChatOptions:

  • maxTokens
  • temperature
  • topP
  • logitBias
  • user
  • n
  • stop
  • presencePenalty
  • frequencyPenalty
  • deploymentName
  • responseFormat
  • functionCallbacks
  • functions

The AzureOpenAiChatOptions object is used in the AzureOpenAiChatModel to create the instance of ChatCompletionsOptions.
ChatCompletionsOptions has the following attributes:

  • messages
  • maxTokens
  • temperature
  • topP
  • logitBias
  • user
  • n
  • stop
  • presencePenalty
  • frequencyPenalty
  • stream
  • model
  • functions
  • functionCall
  • functionCallConfig
  • dataSources
  • enhancements
  • seed
  • responseFormat
  • tools
  • toolChoice
  • logprobs
  • topLogprobs

In code it seems like there's no way to set options like logprobs, enhancements or seed.

Furthermore, in the method that should merge two ChatCompletionsOptions:

private ChatCompletionsOptions merge(ChatCompletionsOptions fromOptions, ChatCompletionsOptions toOptions)

such options are not taken into account

Expected behavior
I'm expecting the AzureOpenAiChatOptions to be aligned with the currently available options for Azure OpenAi
@tzolov tzolov added this to the 1.0.0-M2 milestone Jul 9, 2024
@tzolov tzolov self-assigned this Jul 9, 2024
@markpollack markpollack modified the milestones: 1.0.0-M2, 1.0.0-RC1 Sep 4, 2024
@csterwa csterwa modified the milestones: 1.0.0-RC1, 1.0.0-M3 Sep 10, 2024
@sobychacko sobychacko self-assigned this Sep 20, 2024
sobychacko added a commit to sobychacko/spring-ai that referenced this issue Sep 20, 2024
@sobychacko
spring-projectsGH-889: Align AzureOpenAiChatOptions with Azure ChatCo…
56a41e6 
…mpletionsOptions

Add missing options from Azure ChatCompletionsOptions to Spring AI
AzureOpenAiChatOptions. The following fields have been added:

- seed
- logprobs
- topLogprobs
- enhancements

This change ensures better alignment between the two option sets,
improving compatibility and feature parity.

Resolves spring-projects#889
@sobychacko sobychacko mentioned this issue Sep 20, 2024
Closed
@pradu3
Copy link

pradu3 commented Sep 25, 2024

Hi All!

This change is not working and is breaking things. It is adding the new options but makes more of them indirectly mandatory.

The Assert.notNull checks in AzureOpenAiChatOptions.Builder.with* make the attributes 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), there will be an exception when AzureOpenAiChatModel.getDefaultOptions. This will cause, for example, ChatClient.create to fail. This happen even when the AzureOpenAiChatModel( OpenAIClient microsoftOpenAiClient) constructor is used.

Caused by: java.lang.IllegalArgumentException: seed 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]

Since we would anyhow ignore null options with @JsonInclude(nclude.NON_NULL) on AzureOpenAiChatOptions, there is no added value for having those checks on .with* methods.

@pradu3 pradu3 mentioned this issue Sep 26, 2024
Closed
@markpollack
Copy link
Member

will take a look, thanks for reporting.

@markpollack markpollack reopened this Sep 26, 2024
sobychacko added a commit to sobychacko/spring-ai that referenced this issue Sep 26, 2024
@sobychacko
Remove unnecessary null checks in AzureOpenAiChatOptions.Builder
2956866 
This is related to spring-projects#889

This commit addresses a bug where certain fields were being made
indirectly mandatory due to Assert.notNull checks in the Builder's
with* methods. Specifically:

- Removed Assert.notNull checks from withResponseFormat, withSeed,
  withLogprobs, withTopLogprobs, and withEnhancements methods.

These checks were causing exceptions in AzureOpenAiChatModel.getDefaultOptions
when not all fields were set, leading to failures in methods like
ChatClient.create, even when using the AzureOpenAiChatModel constructor
with OpenAIClient.

The removal of these checks aligns with the @JsonInclude(Include.NON_NULL)
annotation on AzureOpenAiChatOptions, which already ignores null options.
This change maintains the intended flexibility while preventing unintended
mandatory requirements.
@sobychacko sobychacko mentioned this issue Sep 26, 2024
Closed
markpollack pushed a commit that referenced this issue Sep 27, 2024
@sobychacko @markpollack
Remove unnecessary null checks in AzureOpenAiChatOptions.Builder
e29d38d 
This is related to #889

This commit addresses a bug where certain fields were being made
indirectly mandatory due to Assert.notNull checks in the Builder's
with* methods. Specifically:

- Removed Assert.notNull checks from withResponseFormat, withSeed,
  withLogprobs, withTopLogprobs, and withEnhancements methods.

These checks were causing exceptions in AzureOpenAiChatModel.getDefaultOptions
when not all fields were set, leading to failures in methods like
ChatClient.create, even when using the AzureOpenAiChatModel constructor
with OpenAIClient.

The removal of these checks aligns with the @JsonInclude(Include.NON_NULL)
annotation on AzureOpenAiChatOptions, which already ignores null options.
This change maintains the intended flexibility while preventing unintended
mandatory requirements.
@markpollack
Copy link
Member

this issue has been fixed. thanks for hanging in there... see 56a41e6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sobychacko sobychacko

@tzolov tzolov

Labels
azure chat options model client
Projects
None yet
Milestone
1.0.0-M3
Development

Successfully merging a pull request may close this issue.

GH-889: Align AzureOpenAiChatOptions with Azure ChatCompletionsOptions sobychacko/spring-ai
6 participants
@markpollack @csterwa @sobychacko @tzolov @RikJux @pradu3