-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[Draft] Make FunctionTools Serializable (Declarative) #5052
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5052 +/- ##
==========================================
+ Coverage 69.41% 69.54% +0.13%
==========================================
Files 163 163
Lines 10494 10538 +44
==========================================
+ Hits 7284 7329 +45
+ Misses 3210 3209 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
source_code: str | ||
name: str | ||
description: str | ||
global_imports: Sequence[Import] |
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.
Trying to understand this part: Import
looks like a union of str, ImportModule, and Alias, it's a bit unclear to me what it is. Another thing, since import will be also included in the FunctionToolConfig, are they going to be imported at the load time of the FunctionTool
or at call time?
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.
Good question.
It will only be called at call time, when we load the component (wont be called when we dump component).
global_imports=[
ImportFromModule("typing", ("Literal",)),
ImportFromModule("autogen_core", ("CancellationToken",)),
ImportFromModule("autogen_core.tools", ("FunctionTool",))
]
when serialized looks like
'global_imports': [{'module': 'typing', 'imports': ('Literal',)}, {'module': 'autogen_core', 'imports': ('CancellationToken',)}, {'module': 'autogen_core.tools', 'imports': ('FunctionTool',)}]
The goal here is to ensure we can dump and load.
-
dump:
- convert _func to string, as source_code in
FunctionToolConfig
, store specifiedglobal_imports
needed in function (e.g, type variables)
- convert _func to string, as source_code in
-
load (goal is to return an instance of the serialized FunctionTool)
- use exec to load
global_imports
. Import is used here because it lets us specify something likefrom autogen_core import CancellationToken
, it is already used in other parts of the lib. This is when the import is called - use exec to convert
source_code
from string to func - return an instance of FunctionTool(func=func)
- use exec to load
Happy to review other proposals.
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.
So the global imports will be evaluated at FunctionTool.load(...)
time.
While this has the same effect as importing any module in python, what it does effectively is, during application running, it can load and execute arbitrary code when deserializing a component. Not great for security.
I think what needs to happen by default instead, is to best-effort check the current global scope for whether the imports have already been loaded and provide an extra keyword argument to optionally automatically evaluate the required imports. Make sure the label this keyword argument as "insecure!" with
```{caution}
Evaluating the global_imports can lead to arbitrary code execution in your application's environment. It can reveal secretes and keys. Make sure you understand the implications.
```
In the API doc.
In fact, we should do the same for FunctionTool.load(...)
. While it doesn't execute the function directly, it allows arbitrary code to be loaded and potentially executed when the application is running. Make sure to label it as such as well.
Maybe this isn't directly related to this PR, but I was wondering if any thought has been put into alternate ways of running these tools. Perhaps a REST API, or |
There is a great extension for using MCP tools with AutoGen here https://github.com/richard-gyiko/autogen-ext-mcp |
Repeat another comment: it is not making function tools declartive, it's serializable configuration. The function is still written in procedure code so it's not delcarative, it's still procedural. |
I have seen this extension, and I understand that it's not declarative, my point here is more of a larger question around the UI as a generally useful tool going forward. I for example would love to use it as a driver for building agents with tools, but I cannot because realistically the framework is limited by not being able to bring custom code. Custom code for both tools and agents. The GRPC runtime already exists, and things like MCP, so I'm envisioning a world where I as a user of the UI could include additional agents/tools via the network. Does that make sense? |
@Eltanya My comment was for the PR not to you. Apologize for the confusion. I agree MCP or remote API tools are necessary. Would you like to create an issue with a rough sketch of how it may look like, implementation wise? We are just getting started here. We are happy to have you contribute to this effort also, whether through discussion or code. |
Why are these changes needed?
Enable declarative representation of FunctionTool.
Open questions
@jackgerrits (thoughts welcome)
Related issue number
Closes #5036
Checks