Bringing domain-name on par with the rest of the optional configurations #707

csotiriou · 2024-06-27T14:11:05Z

Addition to #698, this brings the domainName() configuration property to feature parity with similar properties, like deploymentName.

geoand

Awesome, thanks!

csotiriou · 2024-06-27T14:13:04Z

perhaps @duchamk should also check this out?

@duchamk would you kindly check out this one as well which is an improvement of your previous PR and check if that also works for you? I cannot replicate your use case, so your input would be great.

duchamk · 2024-06-27T18:45:27Z

I tested from the bugfix/improve-azure-domain-configuration branch.
For the configuration quarkus.langchain4j.azure-openai.domain-name=azure-api.net does not work, it inserts the domain openai.azure.com. However, for configuration

quarkus.langchain4j.azure-openai.chat-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.embedding-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.image-model.domain-name=azure-api.net

everything works fine.
I've looked through the code and I don't know why it doesn't work for the parent configuration.

csotiriou · 2024-06-27T21:07:56Z

I tested from the bugfix/improve-azure-domain-configuration branch. For the configuration quarkus.langchain4j.azure-openai.domain-name=azure-api.net does not work, it inserts the domain openai.azure.com. However, for configuration
quarkus.langchain4j.azure-openai.chat-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.embedding-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.image-model.domain-name=azure-api.net
everything works fine. I've looked through the code and I don't know why it doesn't work for the parent configuration.

@duchamk Can you please attach a reproducer including your entire application.properties?

csotiriou · 2024-06-28T10:42:29Z

@duchamk It seems that due to naming conventions, there are some conflicts between the values of the parent configuration, and the named configurations. It's fairly complicated to fix, so after discussing with @geoand we resorted to using "dummy" defaults in the embedded models, and filtering the value selection in the code by comparing them with the default Dummy value.

I made some minor corrections by leaving as much of the code intact at this point, until a better solution comes into play.

@duchamk, can you please check the new configuration and see if this works as expected?

geoand · 2024-06-28T10:43:15Z

Thanks for taking this up!

geoand · 2024-06-28T11:45:35Z

...ava/io/quarkiverse/langchain4j/azure/openai/runtime/config/LangChain4jAzureOpenAiConfig.java

-import io.smallrye.config.WithDefault;
-import io.smallrye.config.WithDefaults;
-import io.smallrye.config.WithParentName;
+import io.smallrye.config.*;


Please don't use star imports (which the IDE tends to add automatically).

geoand · 2024-06-28T11:46:24Z

...ava/io/quarkiverse/langchain4j/azure/openai/runtime/config/LangChain4jAzureOpenAiConfig.java

+            return deepDomainName
+                    .filter(v -> !ConfigConstants.DUMMY_VALUE.equals(v))
+                    .or(new Supplier<Optional<String>>() {
+                        @Override
+                        public Optional<String> get() {
+                            return domainName();
+                        }
+                    });


If would love it if we avoided lambas and just used imperative code here:)

duchamk · 2024-06-28T16:33:01Z

@duchamk Can you please attach a reproducer including your entire application.properties?

I tested from the bugfix/improve-azure-domain-configuration branch. For the configuration quarkus.langchain4j.azure-openai.domain-name=azure-api.net does not work, it inserts the domain openai.azure.com. However, for configuration
quarkus.langchain4j.azure-openai.chat-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.embedding-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.image-model.domain-name=azure-api.net
everything works fine. I've looked through the code and I don't know why it doesn't work for the parent configuration.
@duchamk Can you please attach a reproducer including your entire application.properties?

# server
quarkus.http.http2=true
quarkus.http.access-log.enabled=true
quarkus.http.access-log.pattern=%{REMOTE_HOST} %l %{REMOTE_USER} %{DATE_TIME} "%{REQUEST_LINE}" %{RESPONSE_CODE} %b %{RESPONSE_TIME}
quarkus.http.record-request-start-time=true
# rest client
quarkus.rest-client.http2=true
quarkus.langchain4j.log-requests=true
quarkus.langchain4j.log-responses=true
# azure AI
quarkus.langchain4j.azure-openai.api-key=mykey
# embed 1536
quarkus.langchain4j.azure-openai.resource-name=secret
quarkus.langchain4j.azure-openai.domain-name=azure-api.net
#quarkus.langchain4j.azure-openai.chat-model.domain-name=azure-api.net
#quarkus.langchain4j.azure-openai.embedding-model.domain-name=azure-api.net
#quarkus.langchain4j.azure-openai.image-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.embedding-model.deployment-name=usersandbox-open-ai-automated-deployment
quarkus.langchain4j.azure-openai.embedding-model.max-tokens=120000
#opl-usersandbox-open-ai-automated-deployment/embeddings?api-version=2023-05-15
quarkus.langchain4j.azure-openai.embedding-model.api-version=2023-05-15
quarkus.langchain4j.azure-openai.deployment-name=gpt-35-turbo
quarkus.langchain4j.azure-openai.chat-model=gpt-35-turbo
quarkus.langchain4j.azure-openai.api-version=2024-02-15-preview
quarkus.langchain4j.azure-openai.log-requests=true
quarkus.langchain4j.azure-openai.log-responses=true
# 1.0
quarkus.langchain4j.azure-openai.chat-model.temperature=0.2
# 1.0
quarkus.langchain4j.azure-openai.chat-model.top-p=0.1
# qdrant
quarkus.langchain4j.qdrant.host=localhost
quarkus.langchain4j.qdrant.collection.name=demo-test
# easy rag
quarkus.langchain4j.easy-rag.path=src/main/resources/documents
quarkus.langchain4j.easy-rag.max-segment-size=300
quarkus.langchain4j.easy-rag.max-overlap-size=30
quarkus.langchain4j.easy-rag.max-results=5
quarkus.langchain4j.easy-rag.path-matcher=glob:**
# build
quarkus.native.additional-build-args=--initialize-at-run-time=io.grpc.netty.shaded.io.netty.util.internal.logging.Log4JLogger

duchamk · 2024-06-28T16:34:34Z

@duchamk It seems that due to naming conventions, there are some conflicts between the values of the parent configuration, and the named configurations. It's fairly complicated to fix, so after discussing with @geoand we resorted to using "dummy" defaults in the embedded models, and filtering the value selection in the code by comparing them with the default Dummy value.

I made some minor corrections by leaving as much of the code intact at this point, until a better solution comes into play.

@duchamk, can you please check the new configuration and see if this works as expected?

No changes, same as before.

geoand · 2024-07-02T11:14:16Z

Can you please rebase this onto main in order to pick up a CI fix?

geoand · 2024-07-03T11:33:20Z

@duchamk can you attach the sample you are using that shows the problematic behavior?
Asking because the latest version of this PR seems to me to work as expected.

Thanks

duchamk · 2024-07-03T15:51:04Z

I will can do it after 14.07, I'm on vacation now.

geoand · 2024-07-03T16:27:44Z

No problem, in that case we'll merge this for the time being

geoand · 2024-07-03T16:27:56Z

@csotiriou can you rebase please?

csotiriou · 2024-07-03T16:37:50Z

Just made the rebase.

From what we have been discussing with @geoand , there are some conflicts in the namings of the configuration properties, which - if the next version wants to keep backwards compatibility with properties - is non-trivial and dangerous to try to solve. Those conflicts are the cause of the behaviour you are experiencing, @duchamk

From what I can gather from george, there may be a possible fix in the future, but it's somewhat dangerous to try to squeeze it right now into what we have.

geoand · 2024-07-03T16:39:32Z

I actually don't think there are any conflicts with your latest fix

handled embedded configurations conflict with the parent configurations. visibility change fixing imports formatting

duchamk · 2024-07-16T17:58:28Z

@duchamk can you attach the sample you are using that shows the problematic behavior? Asking because the latest version of this PR seems to me to work as expected.

Thanks

@geoand
I've included a simple example here https://github.com/duchamk/quarkus-ai-azure

geoand · 2024-07-16T18:07:05Z

Thanks

geoand · 2024-07-18T08:40:12Z

Which exactly is the problematic part you are seeing in that sample?

duchamk · 2024-07-21T14:35:04Z

For configuration in application.properties:

# for all models - don't works
#quarkus.langchain4j.azure-openai.domain-name=azure-api.net
# for each one model - works
quarkus.langchain4j.azure-openai.chat-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.embedding-model.domain-name=azure-api.net
quarkus.langchain4j.azure-openai.image-model.domain-name=azure-api.net

RAG, embedding and chat work properly, the domain is entered correctly.
However, if we change the configuration to:

# for all models - don't works
quarkus.langchain4j.azure-openai.domain-name=azure-api.net
# for each one model - works
#quarkus.langchain4j.azure-openai.chat-model.domain-name=azure-api.net
#quarkus.langchain4j.azure-openai.embedding-model.domain-name=azure-api.net
#quarkus.langchain4j.azure-openai.image-model.domain-name=azure-api.net

then the domain stops substituting.
This is probably related to the problem that @csotiriou wrote about.

geoand · 2024-07-22T06:45:56Z

I tried your sample using mvn quarkus:dev and I got:

 io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: localhost/127.0.0.1:443

Am I missing something?

csotiriou requested a review from a team as a code owner June 27, 2024 14:11

geoand approved these changes Jun 27, 2024

View reviewed changes

csotiriou force-pushed the bugfix/improve-azure-domain-configuration branch 2 times, most recently from 59d057b to 0480927 Compare June 28, 2024 10:41

geoand reviewed Jun 28, 2024

View reviewed changes

csotiriou force-pushed the bugfix/improve-azure-domain-configuration branch 2 times, most recently from 7e46f71 to 1f5bc97 Compare July 3, 2024 16:33

bringing domain-name on par with the rest of the optional configurations

401b02b

handled embedded configurations conflict with the parent configurations. visibility change fixing imports formatting

csotiriou force-pushed the bugfix/improve-azure-domain-configuration branch from 1f5bc97 to 401b02b Compare July 3, 2024 17:15

geoand merged commit 0aa7e26 into quarkiverse:main Jul 4, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bringing domain-name on par with the rest of the optional configurations #707

Bringing domain-name on par with the rest of the optional configurations #707

csotiriou commented Jun 27, 2024 •

edited

Loading

geoand left a comment

csotiriou commented Jun 27, 2024 •

edited

Loading

duchamk commented Jun 27, 2024

csotiriou commented Jun 27, 2024 •

edited

Loading

csotiriou commented Jun 28, 2024

geoand commented Jun 28, 2024

geoand Jun 28, 2024

geoand Jun 28, 2024

duchamk commented Jun 28, 2024

duchamk commented Jun 28, 2024

geoand commented Jul 2, 2024

geoand commented Jul 3, 2024 •

edited

Loading

duchamk commented Jul 3, 2024

geoand commented Jul 3, 2024

geoand commented Jul 3, 2024

csotiriou commented Jul 3, 2024 •

edited

Loading

geoand commented Jul 3, 2024

duchamk commented Jul 16, 2024

geoand commented Jul 16, 2024

geoand commented Jul 18, 2024

duchamk commented Jul 21, 2024

geoand commented Jul 22, 2024

Bringing domain-name on par with the rest of the optional configurations #707

Bringing domain-name on par with the rest of the optional configurations #707

Conversation

csotiriou commented Jun 27, 2024 • edited Loading

geoand left a comment

Choose a reason for hiding this comment

csotiriou commented Jun 27, 2024 • edited Loading

duchamk commented Jun 27, 2024

csotiriou commented Jun 27, 2024 • edited Loading

csotiriou commented Jun 28, 2024

geoand commented Jun 28, 2024

geoand Jun 28, 2024

Choose a reason for hiding this comment

geoand Jun 28, 2024

Choose a reason for hiding this comment

duchamk commented Jun 28, 2024

duchamk commented Jun 28, 2024

geoand commented Jul 2, 2024

geoand commented Jul 3, 2024 • edited Loading

duchamk commented Jul 3, 2024

geoand commented Jul 3, 2024

geoand commented Jul 3, 2024

csotiriou commented Jul 3, 2024 • edited Loading

geoand commented Jul 3, 2024

duchamk commented Jul 16, 2024

geoand commented Jul 16, 2024

geoand commented Jul 18, 2024

duchamk commented Jul 21, 2024

geoand commented Jul 22, 2024

csotiriou commented Jun 27, 2024 •

edited

Loading

csotiriou commented Jun 27, 2024 •

edited

Loading

csotiriou commented Jun 27, 2024 •

edited

Loading

geoand commented Jul 3, 2024 •

edited

Loading

csotiriou commented Jul 3, 2024 •

edited

Loading