weiqingy commented on code in PR #883:
URL: https://github.com/apache/flink-agents/pull/883#discussion_r3556876695
##########
integrations/chat-models/openai/src/main/java/org/apache/flink/agents/integrations/chatmodels/openai/OpenAICompletionsConnection.java:
##########
@@ -98,15 +101,21 @@ public OpenAICompletionsConnection(
builder.baseUrl(apiBaseUrl);
}
- Integer timeoutSeconds = descriptor.getArgument("timeout");
- if (timeoutSeconds != null && timeoutSeconds > 0) {
- builder.timeout(Duration.ofSeconds(timeoutSeconds));
- }
-
- Integer maxRetries = descriptor.getArgument("max_retries");
- if (maxRetries != null && maxRetries >= 0) {
- builder.maxRetries(maxRetries);
- }
+ int rawTimeout =
+ Optional.ofNullable(descriptor.<Number>getArgument("timeout"))
+ .map(Number::intValue)
+
.orElse(OpenAIChatCompletionsUtils.DEFAULT_TIMEOUT_SECONDS);
+ this.timeoutSeconds =
+ rawTimeout > 0 ? rawTimeout :
OpenAIChatCompletionsUtils.DEFAULT_TIMEOUT_SECONDS;
Review Comment:
Python validates both fields with `Field(ge=0)`
(`openai_chat_model.py:62-71`, `azure_openai_chat_model.py:70-79`), so
`timeout=-5` or `max_retries=-1` raise a `ValidationError` at construction.
Here — and identically in `AzureOpenAIChatModelConnection.java:134-143` — an
invalid value is silently rewritten to the default. `ge=0` also means Python
accepts `timeout=0`, where this `> 0` check turns it into `60`. So the clamp
ends up being the one behavior a parity PR leaves divergent. Is that
intentional, or would failing fast (or logging the override) match the Python
contract better?
Either way, the branch is currently unexercised — the Java tests cover only
the unset and valid-override cases (`timeout=120`, `max_retries=5`), so a test
pinning whichever behavior you settle on would be worth adding.
##########
integrations/chat-models/openai/src/main/java/org/apache/flink/agents/integrations/chatmodels/openai/OpenAIChatCompletionsUtils.java:
##########
@@ -52,6 +52,12 @@ final class OpenAIChatCompletionsUtils {
private OpenAIChatCompletionsUtils() {}
+ /** default timeout in seconds for openai api requests (aligned with
python sdk). */
Review Comment:
Small housekeeping on these two constants: the Javadoc starts lowercase
(`default timeout in seconds...`) whereas the rest of this file uses sentence
case (e.g. `Convert a list of...` a few lines below). They're also placed after
the private constructor and ahead of the `mapper`/`MAP_TYPE` fields — constants
conventionally sit at the top of the class. Minor, but tidying both would keep
the file consistent.
--
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]
