Skip to content

Conversation

@xli1996
Copy link

@xli1996 xli1996 commented Nov 19, 2025

Linked issue: #319

Purpose of change

This PR adds support for the Open AI chat model as a foundational model.

Tests

Added one IT test, also tried manually with ReactAgentExample but replace Ollama model with OpenAI one.

API

No

Documentation

  • doc-needed
    Yes, will update docs once this get merged.

@github-actions github-actions bot added priority/major Default priority of the PR or issue. fixVersion/0.2.0 The feature or bug should be implemented/fixed in the 0.2.0 version. doc-label-missing The Bot applies this label either because none or multiple labels were provided. labels Nov 19, 2025
@github-actions
Copy link

@xli1996 Please add the following content to your PR description and select a checkbox:

- [ ] `doc-needed` 
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->

@github-actions github-actions bot added doc-needed Your PR changes impact docs. and removed doc-label-missing The Bot applies this label either because none or multiple labels were provided. labels Nov 19, 2025
@xli1996 xli1996 marked this pull request as ready for review November 19, 2025 01:14
@wenjin272 wenjin272 self-requested a review November 19, 2025 02:56
Copy link
Collaborator

@wenjin272 wenjin272 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @xli1996 , thanks for your contribution. Overall looks good to me. I left some minor comments.

Copy link
Collaborator

@lihaosky lihaosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Xiang!


Map<String, Object> paramSchema = getParamSchema(param);
paramSchema.put("description", paramDescription);
if (paramDescription != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this be null? Looks getParamSchema at least returns Map<String, Object> paramSchema = new HashMap<>();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit the issue when my Tool annotation has no description, seems it just skip

                if (!toolParam.description().isEmpty()) {
                    paramDescription = toolParam.description();
                }

and stay null, maybe we can just remove if (!toolParam.description().isEmpty()) {

@xli1996
Copy link
Author

xli1996 commented Nov 21, 2025

@wenjin272 @lihaosky Thanks for the review, can you take another look, I have addressed some comments.

@xli1996 xli1996 requested a review from wenjin272 November 25, 2025 17:38
@wenjin272
Copy link
Collaborator

LGTM. Please take a look at your convenience @xintongsong

@xintongsong
Copy link
Contributor

@xli1996
The implementation looks good to me. However, with #317 being merged, this PR also needs to be updated.

  1. There're some code conflicts in e2e-test/integration-test/pom.xml
  2. We have moved all the integration tests to e2e-test/flink-agents-end-to-end-tests-integration.
  3. We no longer create separate tests / examples for each of the supported model providers. Alternatively, we can reuse ChatModelIntegrationTest.

# Conflicts:
#	e2e-test/flink-agents-end-to-end-tests-integration/src/test/java/org/apache/flink/agents/integration/test/AgentWithOpenAI.java
#	e2e-test/flink-agents-end-to-end-tests-integration/src/test/java/org/apache/flink/agents/integration/test/AgentWithOpenAIExample.java
@xli1996
Copy link
Author

xli1996 commented Dec 1, 2025

@xintongsong
Thanks for the review.
I have fixed the merge conflicts by removing the tests and adding an optional openai IT test in ChatModelIntegrationTest, can you take another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-needed Your PR changes impact docs. fixVersion/0.2.0 The feature or bug should be implemented/fixed in the 0.2.0 version. priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants