-
Notifications
You must be signed in to change notification settings - Fork 29
fix: conv agent tool calling #485
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
3220358 to
0da7d23
Compare
| self.tool_call_to_ai_message: dict[str, str] = {} | ||
| self.current_message: AIMessageChunk | ||
| self.seen_message_ids: set[str] = set() | ||
| self.pending_message_tool_call_count: dict[str, int] = {} |
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 won't survive suspend/resume (interruptable tool calls, create_process_tool, create_escalation_tool, deep rag etc)
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.
Isn't that also true for tool_call_to_ai_message? If that map is empty after an interrupt, then none of the tool results would be associated with the correct message id (the existing code is creating a new guid for these, but CAS wouldn't know what to do with it and would produce an error event).
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.
Exactly. That's why we were suggesting to loosen up the CAS contracts
| def __init__(self): | ||
| """Initialize the mapper with empty state.""" | ||
| self.tool_call_to_ai_message: dict[str, str] = {} | ||
| self.current_message: AIMessageChunk |
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 needs to be kept as otherwise the tool input won't be properly mapped. When the tool input is streamed by langchain, only the first toolCallChunk contains the tool_id . What will happen with the current changes from this PR is that only the first toolCallChunk will be mapped to a startToolCall (since that's the only one containing and Id) and the rest will be lost. Verified this using a dev version with the changes from this PR : tool_Input appears as empty (since the first tool_call_chunk has no args):
Here are the logs from the agent as well:
Also, if I remember right, CAS does not support streaming the tool input either way so even if the tool_id was present in all chunks, this still wouldn't work, only the first event will create a tool call and the rest wouldn't be discarded I think
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.
ok, after submitting the PR I did notice the inputs were missing....
Development Package