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

Fix some type annotations and edge cases #572

Merged
merged 6 commits into from
Nov 16, 2023

Conversation

s-cerevisiae
Copy link
Collaborator

@s-cerevisiae s-cerevisiae commented Nov 6, 2023

Why are these changes needed?

This PR is a rather conservative attempt to partially fix #513, i.e. to provide a set of more correct type annotations for the library and fix unhandled edge cases found by type checker along the way.

It adjusts some wrong type signatures to fit the actual code implementation.

Related issue number

#513

Remaining problems

  1. Now in the docstrings of ConversableAgent.register_reply, reply_func is documented as follows:
def reply_func(
    recipient: ConversableAgent,
    messages: Optional[List[Dict]] = None,
    sender: Optional[Agent] = None, # Some of the registered reply function don't support a `None` parameter
    config: Optional[Any] = None, # Same as above
) -> Tuple[bool, Union[str, Dict, None]] # Originally `Union[str, Dict, None]`, changed after checking many call sites

In reality some of the reply_funcs support None as sender and some don't. I don't know if the documentation or the implementation should be changed so I left it as-is.

  1. ConversableAgent.(a)_send and receive actually requires the corresponding target to also be ConversableAgents because the extra silent parameter. There's no way to fix this without breaking changes for many. Since silent is declared experimental I decided to leave it there.

  2. Some other rough edges either caused by type checkers or subtle interactions between many functions. They don't cause noticeable bugs as of now, and I'm not going to "fix" them by refactoring the whole codebase.

  3. AFAIK oai module and its related code is under a major refactoring. This PR won't touch these unstable code.

Checks

No new tests are required and documentation has been changed accordingly.

@s-cerevisiae
Copy link
Collaborator Author

@microsoft-github-policy-service agree

Copy link
Contributor

@sonichi sonichi left a comment

Choose a reason for hiding this comment

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

Try to be more conservative 😸 I provide some examples of changes that can be undone.

autogen/agentchat/conversable_agent.py Outdated Show resolved Hide resolved
autogen/agentchat/conversable_agent.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2023

Codecov Report

Merging #572 (4c24437) into main (2a96e4d) will increase coverage by 5.46%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
+ Coverage   34.04%   39.50%   +5.46%     
==========================================
  Files          25       25              
  Lines        3005     3007       +2     
  Branches      668      668              
==========================================
+ Hits         1023     1188     +165     
+ Misses       1906     1722     -184     
- Partials       76       97      +21     
Flag Coverage Δ
unittests 39.44% <100.00%> (+5.46%) ⬆️

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

Files Coverage Δ
autogen/agentchat/assistant_agent.py 83.33% <100.00%> (ø)
autogen/agentchat/conversable_agent.py 67.47% <100.00%> (+0.14%) ⬆️
autogen/agentchat/groupchat.py 61.20% <100.00%> (ø)
autogen/agentchat/user_proxy_agent.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@s-cerevisiae
Copy link
Collaborator Author

Rebased to last commit and removed controversial breaking change.

@sonichi sonichi added the documentation Improvements or additions to documentation label Nov 13, 2023
Copy link
Collaborator

@BeibinLi BeibinLi left a comment

Choose a reason for hiding this comment

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

Nice work!

autogen/agentchat/conversable_agent.py Show resolved Hide resolved
autogen/agentchat/conversable_agent.py Show resolved Hide resolved
autogen/agentchat/groupchat.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@BeibinLi BeibinLi left a comment

Choose a reason for hiding this comment

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

LGTM. We need to fix "GroupChat" after this PR.

@sonichi sonichi added this pull request to the merge queue Nov 16, 2023
Merged via the queue into microsoft:main with commit d340159 Nov 16, 2023
55 of 58 checks passed
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
* Fix some type annotations in agents

This fixes some errors in type annotations of `ConversableAgent`,
`UserProxyAgent`, `GroupChat` and `AssistantAgent` by adjusting the type
signature according to the actual implementation. There should be no
change in code behavior.

* Fix agent types in `GroupChat`

Some `Agent`s are actually required to be `ConversableAgent` because
they are used as one.

* Convert str message to dict before printing message

* Revert back to Agent for GroupChat

* GroupChat revert update

---------

Co-authored-by: Beibin Li <[email protected]>
Co-authored-by: Beibin Li <[email protected]>
jackgerrits added a commit that referenced this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type errors. So many of them.
4 participants