refactor: toolkits #1012
refactor: toolkits #1012
Conversation
- Refactor toolkit imports in `camel/toolkits/__init__.py` - Add `ToolManager` class to manage dynamically loading and accessing toolkits in `camel/toolkits/toolkits_manager.py`
camel/toolkits/toolkits_manager.py
Outdated
| if hasattr(module, '__all__'): | ||
| for name in module.__all__: | ||
| func = getattr(module, name, None) | ||
| if callable(func) and func.__module__ == module.__name__: | ||
| self.toolkits[name] = func | ||
| else: | ||
| for name, func in inspect.getmembers( | ||
| module, inspect.isfunction | ||
| ): | ||
| if ( | ||
| not name.startswith('_') | ||
| and func.__module__ == module.__name__ | ||
| ): | ||
| self.toolkits[name] = func |
There was a problem hiding this comment.
It might be because I previously had limited understanding of discussions about refactoring, but could you explain why the get_tools method from BaseToolkit wasn't used here to obtain all available tools? Thank you for your explanation!
There was a problem hiding this comment.
Hi~ @yiyiyi0817 Since using the get_tools method requires instantiating BaseToolkit, calling it to list all tools would cause all BaseToolkit classes to be instantiated, which could lead to unnecessary resource usage. It might be more efficient to instantiate the BaseToolkit class only when a specific tool is being used.
There was a problem hiding this comment.
It might be because I previously had limited understanding of discussions about refactoring, but could you explain why the
get_toolsmethod fromBaseToolkitwasn't used here to obtain all available tools? Thank you for your explanation!
I believe it might be better if we could list all functional tools as well as the BaseToolkit tools in one place, rather than listing all tools within a single BaseToolkit class.
There was a problem hiding this comment.
Zack and I discussed in slack that there may be some duplication in the functions of our two PRs(#1012 and #1036). This may depend on how we want users to use all tookits. Does BaseTookit itself needlist_toolsand get_a_tool methods, or do we need to load all toolkits through an external Manager to complete this process?
In addition, the design of BaseTookit.get_tools method and @export_to_toolkit decorator may also be duplicated. Further and more extensive discussion is needed.
Regarding the instantiation problem of using get_tools design, one solution is to make get_tools, list_tools and get_a_tool methods as class methods, but the premise is that these methods do not really need to be instantiated.
And open_api_function need to be considered also.
Co-authored-by: Wendong <[email protected]> Co-authored-by: Wendong-Fan <[email protected]>
|
At this point, all toolkit usage methods are fully supported, and the presentation of these methods has been unified (with the exception of |
|
Initial support has been added for using multiple tool instances within the same toolkit class, but further improvements will require adding support for modifying the docstrings of tool instances. |
If needed, I can help to integrate |
Description
Describe your changes in detail.
Motivation and Context
Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.
You can use the syntax
close #15213if this solves the issue #15213Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Implemented Tasks
Checklist
Go over all the following points, and put an
xin all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!