[integration][openai] Align default parameters for openai chat model between java and python#883
[integration][openai] Align default parameters for openai chat model between java and python#883rob-9 wants to merge 10 commits into
Conversation
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for taking this on @rob-9. The code looks good to me, but I think we should also update the documentation.
The user-facing docs still describe the old Java defaults. After this PR, Java OpenAI/Azure connections explicitly default to timeout=60 and max_retries=3, but these tables still mention SDK fallback/None and max_retries=2. Please update the docs to match the new constructor behavior.
|
@wenjin272 thx |
| .map(Number::intValue) | ||
| .orElse(OpenAIChatCompletionsUtils.DEFAULT_TIMEOUT_SECONDS); | ||
| this.timeoutSeconds = | ||
| rawTimeout > 0 ? rawTimeout : OpenAIChatCompletionsUtils.DEFAULT_TIMEOUT_SECONDS; |
There was a problem hiding this 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.
|
|
||
| private OpenAIChatCompletionsUtils() {} | ||
|
|
||
| /** default timeout in seconds for openai api requests (aligned with python sdk). */ |
There was a problem hiding this 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.
fixes #464
purpose
the Java OpenAI chat model connections (
OpenAICompletionsConnectionandAzureOpenAIChatModelConnection) didn't set explicit defaults fortimeoutandmax_retries, falling thru to the openai-java SDK's internal values. the Python SDK explicitly defaults totimeout=60sandmax_retries=3, meaning the same agent config behaves differently across languages.this PR aligns Java to match Python:
DEFAULT_TIMEOUT_SECONDS = 60/DEFAULT_MAX_RETRIES = 3constants.Optional.orElse(DEFAULT_*)so absent args get the explicit default.testing
OpenAICompletionsConnectionTestasserts correct defaults, explicit overrides work, and api_key validation holds.AzureOpenAIChatModelConnectionTestwith default-resolution and override assertions.timeout == 60.0andmax_retries == 3.docs
doc-not-needed