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

Allow multi-deployment configuration for Azure OpenAI #598

Conversation

csotiriou
Copy link
Contributor

@csotiriou csotiriou commented May 17, 2024

Relevant to #237, this will allow overriding the following properties per model in the Azure OpenAI configuration

  • endpoint
  • resource-name
  • deployment-name

Example configuration that is now possible:

quarkus.langchain4j.azure-openai.ad-token=token
quarkus.langchain4j.azure-openai.api-version=2023-07-01-preview
quarkus.langchain4j.azure-openai.chat-model.enabled=true
quarkus.langchain4j.azure-openai.deployment-name=my-deployment-name
quarkus.langchain4j.azure-openai.embedding-model.enabled=true
quarkus.langchain4j.azure-openai.resource-name=name
quarkus.langchain4j.azure-openai.embedding-model.deployment-name=another-deployment-name

In the above example, all models in the parent configuration will have the my-deployment-name, but the Embedding model will have the another-deployment-name specifically as a name.

Copy link
Collaborator

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Very nice!

My only ask is to not use lambdas as we generally try to avoid them in runtime code

@csotiriou csotiriou marked this pull request as ready for review May 17, 2024 14:29
@csotiriou csotiriou requested a review from a team as a code owner May 17, 2024 14:29
@csotiriou
Copy link
Contributor Author

csotiriou commented May 17, 2024

@geoand I removed the lamdas in favor of Suppliers I hope that's ok (I saw that there were other cases with Suppliers as well. I also made the PR ready for review, as I have tested it and it seems to be working.

I suppose the reason for the change is performance?

@geoand
Copy link
Collaborator

geoand commented May 17, 2024

Yeah, at some point in the past we saw there was a tiny impact related to lambdas so we decided to try and limit their use in our code

Copy link
Collaborator

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks!

Now all we need is to have the commits squashed 😀

@csotiriou csotiriou force-pushed the feature/azureai-allow-multiple-resource-groups branch from 64c367e to 506e4a8 Compare May 17, 2024 14:43
@csotiriou
Copy link
Contributor Author

Ah, yes. done.

Copy link
Collaborator

@geoand geoand left a comment

Choose a reason for hiding this comment

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

🎉

@geoand geoand force-pushed the feature/azureai-allow-multiple-resource-groups branch from 506e4a8 to 9cd4d3c Compare May 17, 2024 15:44
@geoand
Copy link
Collaborator

geoand commented May 17, 2024

I force pushed an update that fixes the formatter complaints

@geoand geoand merged commit 84c98f2 into quarkiverse:main May 17, 2024
12 checks passed
@csotiriou csotiriou deleted the feature/azureai-allow-multiple-resource-groups branch May 17, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants