-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Allow additional tools at runtime #8190
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
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.
One minor comment
d7dbc74
to
1d335e2
Compare
.append("next_thought", dspy.OutputField(), type_=str) | ||
.append("next_tool_name", dspy.OutputField(), type_=Literal[tuple(tools.keys())]) | ||
.append("next_tool_name", dspy.OutputField(), type_=str) |
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.
This part feels a bit downgraded, but at the same time, I wouldn't dynamically update the signature at runtime.
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.
This feels a little scary because it's hard to tell if this is going to cause regression or not.
@okhat From your experience, do you think LM can remember only to choose from provided tool without the Literal
constrant?
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.
Yeah, tool names are passed to the LLM in the same call, so LLMs should have enough context. We could dynamically construct the literal at runtime, but I think it would increase the complexity so want to avoid it unless it causes regression.
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.
A few minor comments, otherwise LGTM!
.append("next_thought", dspy.OutputField(), type_=str) | ||
.append("next_tool_name", dspy.OutputField(), type_=Literal[tuple(tools.keys())]) | ||
.append("next_tool_name", dspy.OutputField(), type_=str) |
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.
This feels a little scary because it's hard to tell if this is going to cause regression or not.
@okhat From your experience, do you think LM can remember only to choose from provided tool without the Literal
constrant?
This PR allows tools to maintain states during the lifespan of each
ReAct.forward
without affecting other executions by allowing users to pass additional tools toforward
. Users can create a tool class that mutates instance variables.