[Feature][Java] Add OpenAI Responses API integration#556
[Feature][Java] Add OpenAI Responses API integration#556addu390 wants to merge 6 commits intoapache:mainfrom
Conversation
github-actions
bot
added
priority/major
addu390
changed the title
wenjin272
left a comment
wenjin272
left a comment
There was a problem hiding this comment.
Hi, @addu390, thanks for your contribution, LGTM. Just some minor comments. Please take a look at your convenience @xintongsong.
I noticed that you explained the additional capabilities of the Response API compared to the Chat Completions API in your comments; however, these capabilities seems do not appear to be reflected in the current implementation. To demonstrate these capabilities, we may need to extend the existing APIs, which could require a significant amount of work. Therefore, I think we can wait until there are actual user requirements before proceeding.
|
|
||
| private final ObjectMapper mapper = new ObjectMapper(); | ||
| private final OpenAIClient client; | ||
| private final String defaultModel; |
There was a problem hiding this comment.
This field maybe not needed. The OpenAIResponseModelSetup will always pass a model name when calling chat.
There was a problem hiding this comment.
True. Was following an existing pattern, as seen in AnthropicChatModelConnection and OpenAIChatModelConnection.
It could be useful, if the intention was to choose the default at connection and not set-up.
|
|
||
| private static final TypeReference<Map<String, Object>> MAP_TYPE = new TypeReference<>() {}; | ||
|
|
||
| private final ObjectMapper mapper = new ObjectMapper(); |
There was a problem hiding this comment.
Because ObjectMapper is thread safe, this could be a static field.
@wenjin272 Agreed, I removed the comment, the PR already does not cover any new capabilities of responses API. |
| return ResourceDescriptor.Builder.newBuilder(ResourceName.ChatModel.OPENAI_CONNECTION) | ||
| .addInitialArgument("api_key", apiKey) | ||
| .build(); | ||
| } else if (provider.equals("OPENAI_RESPONSE")) { |
There was a problem hiding this comment.
| } else if (provider.equals("OPENAI_RESPONSE")) { | |
| } else if (provider.equals("OPENAI_RESPONSES")) { |
| "org.apache.flink.agents.integrations.chatmodels.openai.OpenAIResponseModelConnection"; | ||
| public static final String OPENAI_RESPONSE_SETUP = | ||
| "org.apache.flink.agents.integrations.chatmodels.openai.OpenAIResponseModelSetup"; | ||
|
|
There was a problem hiding this comment.
-
According to the documents, the formal name of the API is "Responses API". Let's make it consistent and use RESPONSES (rather than RESPONSE) for the constants and class names.
-
I'd suggest to also change
OPENAI_CONNECTIONtoOPENAI_COMPLETIONS_CONNECTION, as well as the class names, to avoid confusion. -
There are some string constants for referencing java integrations from python codes in
resource.py, which should also be updated.
There was a problem hiding this comment.
Missed those in resource.py, I made the changes to fix naming across the code-base.
Should we also rename OpenAIChatModelConnection to OpenAICompletionsConnection, OpenAIChatModelSetup to OpenAICompletionsSetup for consistency in naming?
There was a problem hiding this comment.
Should we also rename
OpenAIChatModelConnectiontoOpenAICompletionsConnection,OpenAIChatModelSetuptoOpenAICompletionsSetupfor consistency in naming?
Yes, I think we should also change the class names to avoid confusion, as well as the resource.py.
There was a problem hiding this comment.
I think so too! Updated the class names and the references.
3 participants
Linked issue: #132
Purpose of change
Adds support for OpenAI's Responses API as a new chat model integration alongside the existing Chat Completions integration.
The Responses API is OpenAI's recommended API. This initial implementation covers just the core functionality, similar to the existing Chat Completions integration.
The Setup class shares several parameters with
OpenAIChatModelSetup. A shared base class could reduce duplication, happy to refactor in a follow-up if that's preferred.Tests
Verified via the existing
ChatModelIntegrationTeste2e integration test with a newOPENAI_RESPONSESparameterized variant. All tests pass.API
Yes, adds two new public classes (
OpenAIResponseModelConnection,OpenAIResponseModelSetup) and two new resource name constants. No changes to existing public APIs (intentionally left it as-is).Documentation
doc-neededdoc-not-neededdoc-included