Skip to content
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

remove useless code in groupchat; simplify the handling of termination msg #1532

Merged
merged 2 commits into from
Feb 4, 2024

Conversation

sonichi
Copy link
Contributor

@sonichi sonichi commented Feb 4, 2024

Why are these changes needed?

  • Removing an if condition that's never True.
  • Include termination message for every agent's conversation history to keep it simple.

Related issue number

Checks

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1abbcf3) 34.26% compared to head (d21f97e) 46.58%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1532       +/-   ##
===========================================
+ Coverage   34.26%   46.58%   +12.32%     
===========================================
  Files          42       42               
  Lines        5102     5098        -4     
  Branches     1167     1234       +67     
===========================================
+ Hits         1748     2375      +627     
+ Misses       3210     2523      -687     
- Partials      144      200       +56     
Flag Coverage Δ
unittests 46.54% <100.00%> (+12.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@afourney afourney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Not sure why it was ever doing that check around line #406

@sonichi
Copy link
Contributor Author

sonichi commented Feb 4, 2024

Looks good.

Not sure why it was ever doing that check around line #406

I also changed the way to deal with termination message. It is broadcast to every other agent before exiting the chat. Then all the agents have the same messages in their conversation history. I think it simplifies things. Please let me know if this breaks anything for you.

@sonichi sonichi added the group chat/teams group-chat-related issues label Feb 4, 2024
@afourney
Copy link
Member

afourney commented Feb 4, 2024

Yeah saw that. I agree this is a better way to handle it. I am not aware of any scenarios this would harm.

@sonichi sonichi enabled auto-merge February 4, 2024 19:19
@sonichi sonichi added this pull request to the merge queue Feb 4, 2024
Merged via the queue into main with commit c6c5fb0 Feb 4, 2024
46 of 56 checks passed
@sonichi sonichi deleted the patch0204 branch February 4, 2024 19:27
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
…n msg (microsoft#1532)

* remove useless code in groupchat

* include termination msg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
group chat/teams group-chat-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants