wenjin272 commented on code in PR #314:
URL: https://github.com/apache/flink-agents/pull/314#discussion_r2525608647
##########
plan/src/main/java/org/apache/flink/agents/plan/resourceprovider/PythonResourceProvider.java:
##########
@@ -46,6 +54,25 @@ public PythonResourceProvider(
this.module = module;
this.clazz = clazz;
this.kwargs = kwargs;
+ this.descriptor = null;
+ }
+
+ public PythonResourceProvider(String name, ResourceType type,
ResourceDescriptor descriptor) {
+ super(name, type);
+ module = descriptor.getArgument("module");
+ if (module == null || module.isEmpty()) {
+ throw new IllegalArgumentException("module should not be null or
empty.");
+ }
+ clazz = descriptor.getArgument("clazz");
+ if (clazz == null || clazz.isEmpty()) {
+ throw new IllegalArgumentException("clazz should not be null or
empty.");
+ }
+ this.kwargs = new HashMap<>(descriptor.getInitialArguments());
Review Comment:
This kwargs may need remove `module` and `clazz`.
Or we can just add a new implementation `PythonResourceDescriptor`, which
has additional property `pythonMoudel` and `pythonClass`. User can directly use
it when declare a resource in Agent.
##########
plan/src/test/resources/resource_providers/python_resource_provider.json:
##########
@@ -3,6 +3,7 @@
"type" : "chat_model",
"module" : "flink_agents.plan.tests.test_resource_provider",
"clazz" : "MockChatModelImpl",
+ "descriptor" :null,
Review Comment:
This json is serialized from python agent plan, but `PythonResourceProvider`
in python won't generate `descriptor` field.
##########
plan/src/main/java/org/apache/flink/agents/plan/serializer/ResourceProviderJsonDeserializer.java:
##########
@@ -76,6 +76,15 @@ public ResourceProvider deserialize(
private PythonResourceProvider deserializePythonResourceProvider(JsonNode
node) {
String name = node.get("name").asText();
String type = node.get("type").asText();
+ try {
+ ResourceDescriptor descriptor =
+ mapper.treeToValue(node.get("descriptor"),
ResourceDescriptor.class);
+ if (descriptor != null) {
Review Comment:
We can check the `descriptor` field existence rather than if the field is
null.
##########
plan/src/main/java/org/apache/flink/agents/plan/resourceprovider/PythonResourceProvider.java:
##########
@@ -63,11 +90,22 @@ public Map<String, Object> getKwargs() {
@Override
public Resource provide(BiFunction<String, ResourceType, Resource>
getResource)
throws Exception {
- // TODO: Implement Python resource creation logic
- // This would typically involve calling into Python runtime to create
the
- // resource
- throw new UnsupportedOperationException(
- "Python resource creation not yet implemented in Java
runtime");
+ if (pythonResourceAdapter != null) {
+ Class<?> clazz = Class.forName(descriptor.getClazz());
+ Map<String, Object> kwargs = new
HashMap<>(descriptor.getInitialArguments());
Review Comment:
Why not use class data member but create `clazz` and `kwargs` again here?
##########
plan/src/main/java/org/apache/flink/agents/plan/resourceprovider/PythonResourceProvider.java:
##########
@@ -46,6 +54,25 @@ public PythonResourceProvider(
this.module = module;
this.clazz = clazz;
this.kwargs = kwargs;
+ this.descriptor = null;
Review Comment:
Should we add `descriptor` field for `PythonResourceProvider` deserialized
from agent plan declared using python api?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
