-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Support functions removing in ConversableAgent #1786
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1786 +/- ##
===========================================
+ Coverage 36.96% 48.02% +11.05%
===========================================
Files 62 62
Lines 6525 6528 +3
Branches 1445 1571 +126
===========================================
+ Hits 2412 3135 +723
+ Misses 3920 3144 -776
- Partials 193 249 +56
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.
This new feature looks good to me.
However, the usage of using "None" for removal inside a "register" function might be a little bit confusing for some users. If we can change this function name to "update_function_registration", it would be more correct...
Let's forget about the function name for backward compatibility for now.
@BeibinLi Agree. We may consider changing the function name in future PR. |
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.
I agree with @BeibinLi. Setting parameters to None to remove functions seems not a straightforward way and can cause confusion. The code looks good to me.
Perhaps we can create a new function Though I still would like to understand the usage case and why you need to have this feature. |
@ekzhu I’m currently working on the AutoGen learning component. Having a convenient API to manipulate registered functions is crucial for me. |
Good suggestion. I will make it in the next PR. We can merge this one first. |
* fix * update * reformat --------- Co-authored-by: “skzhang1” <“[email protected]”>
Why are these changes needed?
there are scenarios where we not only need to register functions but also remove previously registered ones in UserProxyAgent. Similar to the existing
update_function_signature
in the assistant, we also need an interface to remove functions in UserProxyAgent.Related issue number
Checks