-
Notifications
You must be signed in to change notification settings - Fork 30
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
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.
It isn't just CAS, it's a breaking change for the contract with all the clients. It also makes the protocol more complicated. You can no longer depend on start/end events to scope event handlers, and that makes implementing clients harder.
I'm pretty sure all the state we need is in the graph's state and can be recovered on resume. But I'm not sure exactly how interrupts work with streaming in langgraph. Figuring it out now.
105406c to
88e1b0f
Compare
88e1b0f to
1a5a9a0
Compare
Development Package