Skip to content
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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

victordibia
Copy link
Collaborator

@victordibia victordibia commented Jan 14, 2025

Why are these changes needed?

Enable declarative representation of FunctionTool.

Open questions
@jackgerrits (thoughts welcome)

  • Currently we use exec to load passed in global_imports and also to get function from serialized string. suggestions welcome on how to improve this if at all.
  • Do we add specific warnings for cases where where the specified imports fail? What do we do to make it less easy to write functions that cannot be serialized properly (or detect when this is the case)?
from typing import Literal
from autogen_core import CancellationToken
from autogen_core.tools import FunctionTool, ImportFromModule


async def calculate(
    operation:  Literal["add", "subtract", "multiply", "divide"],
    x: float,
    y: float,
) -> float:
    """Perform basic arithmetic operations on two numbers."""
    if operation == "add":
        return x + y
    elif operation == "subtract":
        return x - y
    elif operation == "multiply":
        return x * y
    elif operation == "divide":
        if y == 0:
            raise ValueError("Cannot divide by zero")
        return x / y
    else:
        raise ValueError(f"Unknown operation: {operation}")


# Create the calculator tool
calculator_tool = FunctionTool(
    func=calculate,
    description="A calculator that can add, subtract, multiply, or divide two numbers.",
    global_imports=[
        ImportFromModule("typing", ("Literal",)),
        ImportFromModule("autogen_core", ("CancellationToken",)),
        ImportFromModule("autogen_core.tools", ("FunctionTool",))
    ]
)

tool_config = calculator_tool.dump_component()
print(tool_config.model_dump())
 
{'provider': 'autogen_core.tools.FunctionTool', 'component_type': 'tool', 'version': 1, 'component_version': 1, 'description': None, 'config': {'source_code': 'async def calculate(\n    operation:  Literal["add", "subtract", "multiply", "divide"],\n    x: float,\n    y: float,\n) -> float:\n    """Perform basic arithmetic operations on two numbers."""\n    if operation == "add":\n        return x + y\n    elif operation == "subtract":\n        return x - y\n    elif operation == "multiply":\n        return x * y\n    elif operation == "divide":\n        if y == 0:\n            raise ValueError("Cannot divide by zero")\n        return x [/](https://file+.vscode-resource.vscode-cdn.net/) y\n    else:\n        raise ValueError(f"Unknown operation: {operation}")\n', 'name': 'calculate', 'description': 'A calculator that can add, subtract, multiply, or divide two numbers.', 'global_imports': [{'module': 'typing', 'imports': ('Literal',)}, {'module': 'autogen_core', 'imports': ('CancellationToken',)}, {'module': 'autogen_core.tools', 'imports': ('FunctionTool',)}], 'has_cancellation_support': False}}
# load
loaded_calculator = FunctionTool.load_component(tool_config)

Related issue number

Closes #5036

Checks

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 89.79592% with 5 lines in your changes missing coverage. Please review.

Project coverage is 69.54%. Comparing base (ae98c9d) to head (f9c3667).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...ogen-core/src/autogen_core/tools/_function_tool.py 87.50% 5 Missing ⚠️
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     
Flag Coverage Δ
unittests 69.54% <89.79%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

source_code: str
name: str
description: str
global_imports: Sequence[Import]
Copy link
Collaborator

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?

Copy link
Collaborator Author

@victordibia victordibia Jan 15, 2025

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 specified global_imports needed in function (e.g, type variables)
  • 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 like from 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)

Happy to review other proposals.

Copy link
Collaborator

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.

@victordibia victordibia changed the title Make FunctionTools Declarative [Draft] Make FunctionTools Declarative Jan 15, 2025
@victordibia victordibia mentioned this pull request Jan 15, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support declarative configurations for FunctionTools
3 participants