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.

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

Add multimodality support on ChatClient with Azure OpenAI API #830

Closed
wants to merge 1 commit into from

Conversation

bmoussaud
Copy link
Contributor

The 1.0.0-beta.8 supports multimodality only if the external resources are added to the request.
This is what this PR add to the Azure Open AI provide.

Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @bmoussaud !

I left few comments for code style alignments. Once those are resolved I will give it a try and merge it!
Als, please consider updating the multimodality docs page: https://docs.spring.io/spring-ai/reference/api/multimodality.html
You can add the Azure model to the list there.

Thank you again!

import com.azure.ai.openai.models.ChatCompletionsJsonResponseFormat;
import com.azure.ai.openai.models.ChatCompletionsTextResponseFormat;
import com.azure.ai.openai.models.ChatCompletionsResponseFormat;
import com.azure.ai.openai.models.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -88,14 +66,14 @@ public class AzureOpenAiChatModel
private final Logger logger = LoggerFactory.getLogger(getClass());

/**
* The configuration information for a chat completions request.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of churning those lines?


import java.io.IOException;
import java.net.URL;
import java.util.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same , no wild card imports

fix import and add documentation
restore the order of the declaration
@tzolov
Copy link
Contributor

tzolov commented Jun 17, 2024

Hi @bmoussaud, thanks for the contribution.
I've just merged it, removing the deployment-name configs in the ITs as later are optimised for our GH IT runs.
Noticed that you've tried to push a change after the branch i've merged. If important feel free to raise a separate PR.

@tzolov
Copy link
Contributor

tzolov commented Jun 17, 2024

rebased, squashed and merged at c9dd336

@tzolov tzolov closed this Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants