-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Store reference to original function when creating FunctionTool
#2146
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
base: main
Are you sure you want to change the base?
Conversation
seratch
left a comment
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.
Thanks for sending this pull request. Ideally, enabling my_func('hello ', 'world!') is the best. I will also explore solutions for it. As for your changes, make lint seems to be failing. Can you resolve it?
|
@seratch Thanks for the quick response. To achieve the functionality you are describing we could adopt the following approach: import inspect
from dataclasses import dataclass
import functools
@dataclass
class FunctionTool:
....
func: ToolFunction[...] | None = None
def __post_init__(self):
if self.func:
functools.update_wrapper(self, self.func)
def __call__(self, *args, **kwargs):
if not self.func:
raise FunctionNotSetError()
return self.func(*args, **kwargs)
def func(x: str) -> str:
"""this is a test"""
return x
func = FunctionTool(...,func=func)
sig = inspect.signature(func)
sig_func = inspect.signature(func.func)
assert sig == sig_func
assert func.__doc__ == func.func.__doc__This will ensure that signatures and docs of the original function are retained. What do you think? |
|
Yeah you're right. I actually did the same for Slack's official SDKs before joining OpenAI :) If you could apply the approach rather than holding func object in FunctionTool instance, it should be better for dev experience. I don't think this approach could cause any side effects but we can verify if nothing is broken due to that. |
|
@seratch I implemented the solution I proposed. A few notes:
|
When
@function_toolis used, no reference to the original function is retained, which makes testing the function directly cumbersome. The original function is now just stored as an attribute ofFunctionTool:The alternative is to require users to define helper functions duplicating signatures and doc strings: