-
Notifications
You must be signed in to change notification settings - Fork 167
[integration][openai] Align default parameters for openai chat model between java and python #883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
fca1fcb
566b80a
f406363
fb342a3
0a0ded7
bd49d9b
f54f58f
88f03da
82eb320
8e6609b
e8f894a
abd88f5
2d50791
e2e7786
1f97bc1
cf9be98
d44ebe6
3a7102e
295e263
44c3128
705f05d
518463a
c5eb32b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,7 @@ | |
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
|
|
||
| /** | ||
| * A chat model integration for the OpenAI Chat Completions service using the official Java SDK. | ||
|
|
@@ -53,8 +54,8 @@ | |
| * <li><b>api_key</b> (required): OpenAI API key | ||
| * <li><b>api_base_url</b> (optional): Base URL for OpenAI API (defaults to | ||
| * https://api.openai.com/v1) | ||
| * <li><b>timeout</b> (optional): Timeout in seconds for API requests | ||
| * <li><b>max_retries</b> (optional): Maximum number of retry attempts (default: 2) | ||
| * <li><b>timeout</b> (optional): Timeout in seconds for API requests (default: 60) | ||
| * <li><b>max_retries</b> (optional): Maximum number of retry attempts (default: 3) | ||
| * <li><b>default_headers</b> (optional): Map of default headers to include in all requests | ||
| * <li><b>model</b> (optional): Default model to use if not specified in setup | ||
| * </ul> | ||
|
|
@@ -81,6 +82,8 @@ public class OpenAICompletionsConnection extends BaseChatModelConnection { | |
| private static final ObjectMapper mapper = new ObjectMapper(); | ||
| private final OpenAIClient client; | ||
| private final String defaultModel; | ||
| private final int timeoutSeconds; | ||
| private final int maxRetries; | ||
|
|
||
| public OpenAICompletionsConnection( | ||
| ResourceDescriptor descriptor, ResourceContext resourceContext) { | ||
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling The same pattern is also present in AzureOpenAIChatModelConnection and OpenAIResponsesModelConnection.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks -- validation now checks the raw numeric value before conversion. |
||
| .orElse(OpenAIChatCompletionsUtils.DEFAULT_TIMEOUT_SECONDS); | ||
| this.timeoutSeconds = | ||
| rawTimeout > 0 ? rawTimeout : OpenAIChatCompletionsUtils.DEFAULT_TIMEOUT_SECONDS; | ||
|
rob-9 marked this conversation as resolved.
Outdated
|
||
| builder.timeout(Duration.ofSeconds(this.timeoutSeconds)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The same pattern is also present in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| int rawRetries = | ||
| Optional.ofNullable(descriptor.<Number>getArgument("max_retries")) | ||
| .map(Number::intValue) | ||
| .orElse(OpenAIChatCompletionsUtils.DEFAULT_MAX_RETRIES); | ||
| this.maxRetries = | ||
| rawRetries >= 0 ? rawRetries : OpenAIChatCompletionsUtils.DEFAULT_MAX_RETRIES; | ||
| builder.maxRetries(this.maxRetries); | ||
|
|
||
| Map<String, String> defaultHeaders = descriptor.getArgument("default_headers"); | ||
| if (defaultHeaders != null && !defaultHeaders.isEmpty()) { | ||
|
|
@@ -119,6 +128,16 @@ public OpenAICompletionsConnection( | |
| this.client = builder.build(); | ||
| } | ||
|
|
||
| // visible for testing | ||
| int getTimeoutSeconds() { | ||
| return timeoutSeconds; | ||
| } | ||
|
|
||
| // visible for testing | ||
| int getMaxRetries() { | ||
| return maxRetries; | ||
| } | ||
|
|
||
| @Override | ||
| public ChatMessage chat( | ||
| List<ChatMessage> messages, List<Tool> tools, Map<String, Object> modelParams) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.flink.agents.integrations.chatmodels.openai; | ||
|
|
||
| import org.apache.flink.agents.api.chat.model.BaseChatModelConnection; | ||
| import org.apache.flink.agents.api.resource.ResourceContext; | ||
| import org.apache.flink.agents.api.resource.ResourceDescriptor; | ||
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| /** | ||
| * Unit tests for {@link OpenAICompletionsConnection} — constructor validation and default | ||
| * resolution only, no network access. | ||
| */ | ||
| class OpenAICompletionsConnectionTest { | ||
|
|
||
| private static final ResourceContext NOOP = ResourceContext.fromGetResource((a, b) -> null); | ||
|
|
||
| private static ResourceDescriptor.Builder connectionDescriptor() { | ||
| return ResourceDescriptor.Builder.newBuilder(OpenAICompletionsConnection.class.getName()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Constructor throws when api_key is missing") | ||
| void testConstructorMissingApiKey() { | ||
| ResourceDescriptor desc = connectionDescriptor().build(); | ||
| assertThatThrownBy(() -> new OpenAICompletionsConnection(desc, NOOP)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("api_key"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Constructor succeeds with api_key only (no network call)") | ||
| void testConstructorMinimal() { | ||
| ResourceDescriptor desc = | ||
| connectionDescriptor().addInitialArgument("api_key", "test-key").build(); | ||
| OpenAICompletionsConnection conn = new OpenAICompletionsConnection(desc, NOOP); | ||
| assertThat(conn).isInstanceOf(BaseChatModelConnection.class); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Defaults resolve to timeout=60 and max_retries=3 when not specified") | ||
| void testDefaultTimeoutAndMaxRetries() { | ||
| ResourceDescriptor desc = | ||
| connectionDescriptor().addInitialArgument("api_key", "test-key").build(); | ||
| OpenAICompletionsConnection conn = new OpenAICompletionsConnection(desc, NOOP); | ||
|
|
||
| assertThat(conn.getTimeoutSeconds()) | ||
| .isEqualTo(OpenAIChatCompletionsUtils.DEFAULT_TIMEOUT_SECONDS); | ||
| assertThat(conn.getMaxRetries()).isEqualTo(OpenAIChatCompletionsUtils.DEFAULT_MAX_RETRIES); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Explicit timeout and max_retries override the defaults") | ||
| void testExplicitOverrides() { | ||
| ResourceDescriptor desc = | ||
| connectionDescriptor() | ||
| .addInitialArgument("api_key", "test-key") | ||
| .addInitialArgument("timeout", 120) | ||
| .addInitialArgument("max_retries", 5) | ||
| .build(); | ||
| OpenAICompletionsConnection conn = new OpenAICompletionsConnection(desc, NOOP); | ||
|
|
||
| assertThat(conn.getTimeoutSeconds()).isEqualTo(120); | ||
| assertThat(conn.getMaxRetries()).isEqualTo(5); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.