wenjin272 commented on code in PR #883:
URL: https://github.com/apache/flink-agents/pull/883#discussion_r3568933960
##########
integrations/chat-models/openai/src/main/java/org/apache/flink/agents/integrations/chatmodels/openai/OpenAICompletionsConnection.java:
##########
@@ -98,15 +101,23 @@ public OpenAICompletionsConnection(
builder.baseUrl(apiBaseUrl);
}
- Integer timeoutSeconds = descriptor.getArgument("timeout");
- if (timeoutSeconds != null && timeoutSeconds > 0) {
- builder.timeout(Duration.ofSeconds(timeoutSeconds));
+ this.timeoutSeconds =
+ Optional.ofNullable(descriptor.<Number>getArgument("timeout"))
+ .map(Number::intValue)
Review Comment:
Calling `Number.intValue()` before validation silently truncates or
overflows the input. For example, `-0.5` becomes `0` and bypasses the negative
check, while fractional timeouts lose precision and fractional retry counts
differ from Python validation. Please validate the original value first,
preserve fractional timeout values, and require an exact bounded integer for
`max_retries`.
The same pattern is also present in AzureOpenAIChatModelConnection and
OpenAIResponsesModelConnection.
##########
integrations/chat-models/openai/src/main/java/org/apache/flink/agents/integrations/chatmodels/openai/OpenAICompletionsConnection.java:
##########
@@ -98,15 +101,23 @@ public OpenAICompletionsConnection(
builder.baseUrl(apiBaseUrl);
}
- Integer timeoutSeconds = descriptor.getArgument("timeout");
- if (timeoutSeconds != null && timeoutSeconds > 0) {
- builder.timeout(Duration.ofSeconds(timeoutSeconds));
+ this.timeoutSeconds =
+ Optional.ofNullable(descriptor.<Number>getArgument("timeout"))
+ .map(Number::intValue)
+
.orElse(OpenAIChatCompletionsUtils.DEFAULT_TIMEOUT_SECONDS);
+ if (this.timeoutSeconds < 0) {
+ throw new IllegalArgumentException("timeout must be >= 0, got: " +
this.timeoutSeconds);
}
-
- Integer maxRetries = descriptor.getArgument("max_retries");
- if (maxRetries != null && maxRetries >= 0) {
- builder.maxRetries(maxRetries);
+ builder.timeout(Duration.ofSeconds(this.timeoutSeconds));
Review Comment:
`timeout=0` has opposite semantics in Java and Python. openai-java
interprets `Duration.ZERO` as no timeout, while Python forwards `0.0` to HTTPX
as a zero-second timeout. Please define one consistent zero-value contract
across both languages and test the effective SDK timeout rather than only the
stored field.
The same pattern is also present in `AzureOpenAIChatModelConnection` and
`OpenAIResponsesModelConnection`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
