-
Notifications
You must be signed in to change notification settings - Fork 76
[Feature][runtime] Support the use of Python ChatModel in Java #314
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
base: main
Are you sure you want to change the base?
Conversation
|
@Sxnan @wenjin272 please help take a look this PR, thanks. |
wenjin272
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @GreatEugenius, thanks for your work. Overall looks good to me . I left some comments about details.
Besides, I think this pr should add some test cases to show how to declare an agent in java using python chat model, and verify the behavior.
plan/src/main/java/org/apache/flink/agents/plan/resourceprovider/PythonResourceProvider.java
Outdated
Show resolved
Hide resolved
plan/src/main/java/org/apache/flink/agents/plan/resourceprovider/PythonResourceProvider.java
Outdated
Show resolved
Hide resolved
plan/src/test/resources/resource_providers/python_resource_provider.json
Outdated
Show resolved
Hide resolved
plan/src/main/java/org/apache/flink/agents/plan/resourceprovider/PythonResourceProvider.java
Show resolved
Hide resolved
.../src/main/java/org/apache/flink/agents/plan/serializer/ResourceProviderJsonDeserializer.java
Outdated
Show resolved
Hide resolved
Sxnan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. However, the PR lacks tests. For such a complex feature, we need to have unit tests and e2e tests.
api/src/main/java/org/apache/flink/agents/api/prompt/Prompt.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/flink/agents/api/chat/model/python/PythonChatModelConnection.java
Show resolved
Hide resolved
api/src/main/java/org/apache/flink/agents/api/resource/python/PythonResourceAdapter.java
Outdated
Show resolved
Hide resolved
44f5690 to
e6c063f
Compare
|
Hi @Sxnan and @wenjin272, thank you for the review! I have updated this PR and added a PythonChatModel example for the end-to-end test. Please take a look. Thanks! |
plan/src/main/java/org/apache/flink/agents/plan/resourceprovider/PythonResourceProvider.java
Outdated
Show resolved
Hide resolved
...on-test/src/main/java/org/apache/flink/agents/integration/test/AgentWithPythonChatModel.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/flink/agents/api/prompt/Prompt.java
Outdated
Show resolved
Hide resolved
afb3b60 to
57b88dc
Compare
|
Hi @Sxnan, thank you for the review! I have updated this PR; please review it again. |
d19b8c3 to
8bc4c9c
Compare
99d1407 to
a1b6fe2
Compare
a1b6fe2 to
92dbb6c
Compare
Linked issue: #313
Purpose of change
Support the use of Python ChatModel in Java Flink Agents jobs.
Tests
ut
API
no
Documentation
doc-neededdoc-not-needed