-
Notifications
You must be signed in to change notification settings - Fork 1.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
Add tools calling for dspy.LM #2023
base: main
Are you sure you want to change the base?
Add tools calling for dspy.LM #2023
Conversation
4da9ae1
to
fa49bf2
Compare
@@ -86,7 +86,13 @@ def __init__( | |||
), "OpenAI's o1-* models require passing temperature=1.0 and max_tokens >= 5000 to `dspy.LM(...)`" | |||
|
|||
@with_callbacks | |||
def __call__(self, prompt=None, messages=None, **kwargs): | |||
def __call__(self, prompt=None, messages=None, tools=None, **kwargs): | |||
if tools is not None and not litellm.supports_function_calling(self.model): |
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 @chenmoneygithub ! Right now, we have a general solution that works well.
I'm not sure we should replace it with a special solution that doesn't work for many model providers.
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.
Yes that's a good point. Yuki from Mlflow team reached out asking if we can use the standard tool calling supported by litellm so that we can trace the tool calls. Which kinda makes sense because now we are mixing the tools calls into the message.content
, with this approach it's hard to trace the tool calls.
My plan is this:
- for OAI/Anthropic/Databricks providers, which has standard function calling, we can use the standard way, which is identical across these providers.
- for other models, e.g., local hosted models, we use our current logic.
I am seeing two downsides:
- When we change the logic in
dspy.ReAct
, there might be some performance change (drop or increase, I am not sure), we need to be cautious. dspy.ReAct
will have 2 branches, one for LLms that support tool calling, the other for LLMs that don't support tool calling. So the code will be slightly more complex: https://github.com/stanfordnlp/dspy/blob/main/dspy/predict/react.py#L85-L96.
And I am seeing 2 benefits:
- We can enable clean tool calling tracing, which is useful for debugging complex agents.
- I expect the tool calling to be more robust with OAI/Anthropic if we use their protocal.
Please let me know your thoughts!
fa49bf2
to
5ff0207
Compare
@@ -110,6 +116,8 @@ def __call__(self, prompt=None, messages=None, **kwargs): | |||
} | |||
for c in response["choices"] | |||
] | |||
elif tools: |
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.
I think the if statements here need to be refactored. Right now, you can only ask for logprobs or tools, but not both.
I think we should move the if statement about logprobs and tools etc inside the loop over choices
?
Add
tools
arg fordspy.LM
, since litellm natively supports tool calling. We may also consider havedspy.ReAct
use the default tool calling from LLM providers for robustness.A sample code looks like:
The output is: