You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "rob-9 (via GitHub)" <gi...@apache.org> on 2026/07/07 21:31:07 UTC
[PR] [fix][java] align connection default parameters with python SDK [flink-agents]
rob-9 opened a new pull request, #883:
URL: https://github.com/apache/flink-agents/pull/883
fixes #464
### purpose
the Java OpenAI chat model connections (`OpenAICompletionsConnection` and `AzureOpenAIChatModelConnection`) didn't set explicit defaults for `timeout` and `max_retries`, falling thru to the openai-java SDK's internal values. the Python SDK explicitly defaults to `timeout=60s` and `max_retries=3`, meaning the same agent config behaves differently across languages.
this PR aligns Java to match Python:
- adds shared `DEFAULT_TIMEOUT_SECONDS = 60` / `DEFAULT_MAX_RETRIES = 3` constants.
- both Java connections now resolve these via `Optional.orElse(DEFAULT_*)` so absent args get the explicit default.
- invalid values (timeout ≤ 0, retries < 0) are clamped to the default instead of silently skipped.
- javadoc updated to document the new defaults.
### testing
- new `OpenAICompletionsConnectionTest` asserts correct defaults, explicit overrides work, and api_key validation holds.
- extended `AzureOpenAIChatModelConnectionTest` with default-resolution and override assertions.
- new Python test pinning `timeout == 60.0` and `max_retries == 3`.
### docs
- [x] `doc-not-needed`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [integration][openai] Align default parameters for openai chat model between java and python [flink-agents]
Posted by "weiqingy (via GitHub)" <gi...@apache.org>.
weiqingy commented on code in PR #883:
URL: https://github.com/apache/flink-agents/pull/883#discussion_r3563380956
##########
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:
Confirmed — matches Python's `Field(ge=0)` now (throws on negatives, accepts `0`), and the tests across all three connections close the branch. Thanks!
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [integration][openai] Align default parameters for openai chat model between java and python [flink-agents]
Posted by "weiqingy (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [integration][openai] Align default parameters for openai chat model between java and python [flink-agents]
Posted by "wenjin272 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [integration][openai] Align default parameters for openai chat model between java and python [flink-agents]
Posted by "rob-9 (via GitHub)" <gi...@apache.org>.
rob-9 commented on code in PR #883:
URL: https://github.com/apache/flink-agents/pull/883#discussion_r3561904119
##########
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:
updating to match python contract; both fields will throw `IllegalArgumentException` on negative values and accept zero
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [integration][openai] Align default parameters for openai chat model between java and python [flink-agents]
Posted by "rob-9 (via GitHub)" <gi...@apache.org>.
rob-9 commented on PR #883:
URL: https://github.com/apache/flink-agents/pull/883#issuecomment-4922506350
@wenjin272 thx
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [integration][openai] Align default parameters for openai chat model between java and python [flink-agents]
Posted by "wenjin272 (via GitHub)" <gi...@apache.org>.
wenjin272 merged PR #883:
URL: https://github.com/apache/flink-agents/pull/883
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [integration][openai] Align default parameters for openai chat model between java and python [flink-agents]
Posted by "rob-9 (via GitHub)" <gi...@apache.org>.
rob-9 commented on code in PR #883:
URL: https://github.com/apache/flink-agents/pull/883#discussion_r3763659149
##########
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` now disables timeouts in both Java and Python. added SDK/transport-level tests.
##########
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:
thanks -- validation now checks the raw numeric value before conversion. `max_retries` must be an exact bounded integer, while timeouts preserve fractional values and reject invalid inputs. also added boundary coverage.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org