-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
GroupChat handle is_termination_msg #804
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #804 +/- ##
===========================================
+ Coverage 27.76% 46.29% +18.52%
===========================================
Files 27 27
Lines 3493 3497 +4
Branches 791 835 +44
===========================================
+ Hits 970 1619 +649
+ Misses 2452 1713 -739
- Partials 71 165 +94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 adding this @afourney .
The justifications here makes sense
- enabling better conversation control via the GroupChatManager
- better mental model on GroupChat behavior .. its easier to reason about setting termination logic via the central groupchatmanager than a specific agent or all agents
- also improves efficiency when the groupchatmanger can terminate conversations early/ correctly.
@afourney Any chance may be side effects here e.g. GroupChat notebooks where some results might benefit from multiple back and forths?
Approving in the mean time.
There should not be any side effects. The superclass default for is_termination_msg is None. In that case, this new PR acts identically to the published version. It's only when setting is_termination_msg to something other than None that a change in behavior occurs. If other notebooks were setting is_termination_msg for the GroupChatManager then this is actually a problem for those notebooks, because they were propagating the false belief that the is_terminiation_msg would be considered. In those cases, the revised behavior will indeed differ (and will likely more closely align to the author's intent). However, I'd argue than any such notebooks are already broken without this PR. |
* Have GroupChatManager check is_termination_msg * Added test cases.
Why are these changes needed?
Setting the is_termination_msg function handler in the GroupChatManager has no effect. This is because the GroupChatManager simply forwards the message to the next agent, and it falls on those agents to stop the conversation. This is problematic as it wastes orchestration LLM calls when the conversation is over, and because it prevents the ability to use the GroupChatManager as a central place to control group chat termination.
This PR fixes the issue by checking the termination message before rebroadcasting.
Related issue number
Closes #802
Checks