wenjin272 commented on code in PR #94: URL: https://github.com/apache/flink-agents/pull/94#discussion_r2275231383
########## api/src/main/java/org/apache/flink/agents/api/tool/ToolRegistry.java: ########## @@ -0,0 +1,139 @@ +/* + * 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.api.tool; + +import org.apache.flink.agents.api.tool.annotation.Tool; + +import java.lang.reflect.Method; +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Registry for managing tools available to agents. Works with the merged design where BaseTool + * contains ToolMetadata. + */ +public class ToolRegistry { Review Comment: I think the both approaches are look good to me, and we can provide the first approach now, and provide the second approach later. For the first approach, we can use a plan-specific registry to help tool register, and maybe we need store the registered tools in registry directly in AgentPlan rather than the registry for keep compatibility with python AgentPlan. ########## api/src/main/java/org/apache/flink/agents/api/tool/ToolRegistry.java: ########## @@ -0,0 +1,139 @@ +/* + * 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.api.tool; + +import org.apache.flink.agents.api.tool.annotation.Tool; + +import java.lang.reflect.Method; +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Registry for managing tools available to agents. Works with the merged design where BaseTool + * contains ToolMetadata. + */ +public class ToolRegistry { + + private final Map<String, BaseTool> tools = new ConcurrentHashMap<>(); + + /** Register a tool instance directly. */ + public void registerTool(BaseTool tool) { + String name = tool.getName(); + if (tools.containsKey(name)) { + throw new IllegalArgumentException( + "Tool with name '" + name + "' is already registered"); + } + tools.put(name, tool); + } + + /** Register all @Tool annotated methods from an object instance. */ + public void registerToolsFromObject(Object toolProvider) { Review Comment: The reason I propose only support static method currently is as a part of Flink job, the AgentPlan will be compiled in JobManager, and will be distributed to TaskManagers, so the tools in AgentPlan need to support serialization. For tools from non-static methods, the object should also be serializable, which may cause some complexity. I think maybe we can let the ToolRegistry support register both static and non-static methods, but only support static method as Tool when declare an Agent. ########## api/src/main/java/org/apache/flink/agents/api/tool/example/ToolExample.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.api.tool.example; + +import org.apache.flink.agents.api.tool.*; +import org.apache.flink.agents.api.tool.annotation.Tool; +import org.apache.flink.agents.api.tool.annotation.ToolParam; + +/** Example demonstrating how to create and use tools */ +public class ToolExample { + + /** Example tool for mathematical calculations. */ + @Tool(name = "calculate", description = "Performs basic mathematical operations on two numbers") + public double calculate( + @ToolParam(description = "The operation to perform: add, subtract, multiply, divide") + String operation, + @ToolParam(description = "First number") double a, + @ToolParam(description = "Second number") double b) { + switch (operation.toLowerCase()) { + case "add": + return a + b; + case "subtract": + return a - b; + case "multiply": + return a * b; + case "divide": + if (b == 0) { + throw new IllegalArgumentException("Cannot divide by zero"); + } + return a / b; + default: + throw new IllegalArgumentException("Unknown operation: " + operation); + } + } + + /** Example tool for string manipulation. */ + @Tool(name = "format_text", description = "Formats text with various options") + public String formatText( + @ToolParam(description = "The text to format") String text, + @ToolParam(description = "Convert to uppercase", required = false) boolean uppercase, + @ToolParam(description = "Prefix to add", required = false) String prefix) { + String result = text; + + if (uppercase) { + result = result.toUpperCase(); + } + + if (prefix != null && !prefix.isEmpty()) { + result = prefix + result; + } + + return result; + } + + /** Static tool example. */ + @Tool(name = "get_timestamp", description = "Gets the current timestamp in milliseconds") + public static long getCurrentTimestamp() { + return System.currentTimeMillis(); + } + + /** Example of how to use the tool system. */ + public static void main(String[] args) { + // Create tool registry + ToolRegistry registry = new ToolRegistry(); + + // Register tools from an instance + ToolExample example = new ToolExample(); + registry.registerToolsFromObject(example); + + // Register static tools from class + registry.registerToolsFromClass(ToolExample.class); + + // Example 1: Use the calculate tool + ToolParameters calcParams = new ToolParameters(); + calcParams.addParameter("operation", "add"); + calcParams.addParameter("a", 10.5); + calcParams.addParameter("b", 5.2); + + ToolResponse calcResult = registry.invokeTool("calculate", calcParams); Review Comment: This will cause Invalid parameters error, for parameters name is not match. The parameter name in tool is arg0, arg1 and arg2 for `method.getParameters()` in `createSchemaFromMethod` not returns the declared name in method. ########## api/src/main/java/org/apache/flink/agents/api/tool/example/ToolExample.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.api.tool.example; + +import org.apache.flink.agents.api.tool.*; +import org.apache.flink.agents.api.tool.annotation.Tool; +import org.apache.flink.agents.api.tool.annotation.ToolParam; + +/** Example demonstrating how to create and use tools */ +public class ToolExample { + + /** Example tool for mathematical calculations. */ + @Tool(name = "calculate", description = "Performs basic mathematical operations on two numbers") + public double calculate( + @ToolParam(description = "The operation to perform: add, subtract, multiply, divide") + String operation, + @ToolParam(description = "First number") double a, + @ToolParam(description = "Second number") double b) { + switch (operation.toLowerCase()) { + case "add": + return a + b; + case "subtract": + return a - b; + case "multiply": + return a * b; + case "divide": + if (b == 0) { + throw new IllegalArgumentException("Cannot divide by zero"); + } + return a / b; + default: + throw new IllegalArgumentException("Unknown operation: " + operation); + } + } + + /** Example tool for string manipulation. */ + @Tool(name = "format_text", description = "Formats text with various options") + public String formatText( + @ToolParam(description = "The text to format") String text, + @ToolParam(description = "Convert to uppercase", required = false) boolean uppercase, + @ToolParam(description = "Prefix to add", required = false) String prefix) { + String result = text; + + if (uppercase) { + result = result.toUpperCase(); + } + + if (prefix != null && !prefix.isEmpty()) { + result = prefix + result; + } + + return result; + } + + /** Static tool example. */ + @Tool(name = "get_timestamp", description = "Gets the current timestamp in milliseconds") + public static long getCurrentTimestamp() { + return System.currentTimeMillis(); + } + + /** Example of how to use the tool system. */ + public static void main(String[] args) { + // Create tool registry + ToolRegistry registry = new ToolRegistry(); + + // Register tools from an instance + ToolExample example = new ToolExample(); + registry.registerToolsFromObject(example); + + // Register static tools from class + registry.registerToolsFromClass(ToolExample.class); Review Comment: This will cause already registered exception -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
