Provide the ability to configure OpenAI client read timeout

[thingersoft]

Created a new java.time.Duration read timeout configuration property with sensible default of 1 minute.
Introduced at org.springframework.ai.autoconfigure.openai.OpenAiParentProperties level so that it can be overriden by more specific configurations.

Did a bit of refactoring of org.springframework.ai.autoconfigure.openai.OpenAiAutoConfiguration to stay DRY.

Eg:
spring.ai.openai.read-timeout = 5m
or
spring.ai.openai.chat.read-timeout = 10m

Closes #354

[making]

As commented in #354 (comment) , using RestClientCustomer is a simple and effective method for many clients.

[thingersoft]

@making
See my comment on related issue.

[thingersoft]

@tzolov or @markpollack
Please see my latest comment on the issue

[markpollack]

Hi. I added comments here, I think for 0.8.1 we can make an interim approach before tackling the general problem.

[markpollack]

Sorry, i was confusing retry timeout with http timeout. We will address this issue, sorry for the confusion.

[thingersoft]

Sorry, i was confusing retry timeout with http timeout. We will address this issue, sorry for the confusion.

@markpollack
Ok, so you don't want me to contribute on it?

[markpollack]

Hi. I think we can achieve what is needed but in a different way. The code you added to merge the property spring.ai.openai.base-url with spring.ai.openai.chat.base-url is already in the code base. See here.

The read timeout can be added by declaring a @Bean that returns an appropriately configured ClientHttpRequestFactory if the bean is not provided. Then the ClientHttpRequestFactory is added as an argument to the OpenAiChatClient and other clients. The ClientHttpRequestFactory can then be added as additional constructor argument to an OpenAiApi constructor.

Let me know what you think.

[thingersoft]

Hi Mark,
having a constructor with both a RestClient.Builder and a ClientHttpRequestFactory parameter wouldn't be measleading for the end user?
I mean when both are provided wich instance of ClientHttpRequestFactory would be used, the one coming with the RestClient.Builder or the ClientHttpRequestFactory parameter?
Maybe we could just declare a conditional RestClient.Builder bean giving it some custom name like openAiRestClientBuilder, and inject that bean into the clients.
This way end users could override that bean without potentials side effects.

What you think?

Also I'm not sure if you intended to keep the new timeout property or just rely on overridable beans declarations.

The code you added to merge the property spring.ai.openai.base-url with spring.ai.openai.chat.base-url is already in the code base.

I see but the properties merge and validation logic is still duplicated for OpenAiImageClient and OpenAiAudioTranscriptionClient beans.
So i merged my previous refactoring with the latest changes to stay DRY.

Let me know.

[markpollack]

I understand the confusion with a constructor that has both a RestClient.Builder and a ClientHttpRequestFactory I need to think about it more.

I would like to have the top level property, so it is easy to use, vs writing code that uses the ClientHttpRequestFactory as that type of code isn't very obvious write.

The merge logic is duplicated for OpenAiImageClient and OpenAiAudioTranscriptionClient beans, but I don't see a way around it as each of those clients rely on their own options class to retrieve the value, e.g. spring.ai.openai.image.base-url

[thingersoft]

The merge logic is duplicated for OpenAiImageClient and OpenAiAudioTranscriptionClient beans, but I don't see a way around it as each of those clients rely on their own options class to retrieve the value, e.g. spring.ai.openai.image.base-url

The specific option classes inherit from OpenAiParentProperties, so logic can be centralized for common properties.
If you look at the code I already did it in OpenAiAutoConfiguration:

private static <T extends OpenAiParentProperties> OpenAiConnectionProperties checkAndOverrideProperties(
	OpenAiConnectionProperties commonProperties, T specificProperties) {

    String apiKey = StringUtils.hasText(specificProperties.getApiKey()) ? specificProperties.getApiKey()
		    : commonProperties.getApiKey();
    
    String baseUrl = StringUtils.hasText(specificProperties.getBaseUrl()) ? specificProperties.getBaseUrl()
		    : commonProperties.getBaseUrl();
    
    Duration readTimeout = specificProperties.getReadTimeout() != null ? specificProperties.getReadTimeout()
		    : commonProperties.getReadTimeout();
    
    Assert.hasText(apiKey, "OpenAI API key must be set");
    Assert.hasText(baseUrl, "OpenAI base URL must be set");
    Assert.notNull(readTimeout, "OpenAI base read timeout must be set");
    
    OpenAiConnectionProperties overridenCommonProperties = new OpenAiConnectionProperties();
    overridenCommonProperties.setApiKey(apiKey);
    overridenCommonProperties.setBaseUrl(baseUrl);
    overridenCommonProperties.setReadTimeout(readTimeout);
    
    return overridenCommonProperties;

}

[thingersoft]

I understand the confusion with a constructor that has both a RestClient.Builder and a ClientHttpRequestFactory I need to think about it more.

Hello,
any news?

[markpollack]

Just to update, there are many issues floating around on this topic, i'm trying to reconcile and figure out the best approach.

[markpollack]

I found out that the only way to do this on a per-call request is to use the netty infra.

https://projectreactor.io/docs/netty/release/reference/index.html#response-timeout

https://docs.spring.io/spring-framework/reference/web/webflux-webclient/client-builder.html#webflux-client-builder-reactor-timeout

WebClient.create().get()
		.uri("https://example.org/path")
		.httpRequest(httpRequest -> {
			HttpClientRequest reactorRequest = httpRequest.getNativeRequest();
			reactorRequest.responseTimeout(Duration.ofSeconds(2));
		})
		.retrieve()
		.bodyToMono(String.class);

Thought I don't know if folks are comfortable with defaulting to use ReactorClientHttpConnector in order to provide this feature.