-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 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.
Why are these changes needed?
Enable declarative representation of FunctionTool.
Open questions
@jackgerrits (thoughts welcome)
Related issue number
Closes #5036
Checks