Repository: microsoft/autogen
Pull request: Fix some type annotations and edge cases
## 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.

<!-- Please give a short summary of the change and the problem this solves. -->

## Related issue number

#513

## Remaining problems

1. Now in the docstrings of `ConversableAgent.register_reply`, `reply_func` is documented as follows:
```python
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_func`s 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.

2. `ConversableAgent.(a)_send` and `receive` *actually* requires the corresponding target to also be `ConversableAgent`s 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.

3. 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.

4. AFAIK `oai` module and its related code is under a major refactoring. This PR won't touch these unstable code.

## Checks

- [x] I've included any doc changes needed for https://microsoft.github.io/autogen/. See https://microsoft.github.io/autogen/docs/Contribute#documentation to build and test documentation locally.
- [x] I've added tests (if relevant) corresponding to the changes introduced in this PR.
- [x] I've made sure all auto checks have passed.

No new tests are required and documentation has been changed accordingly.


Problem statement:
Type errors. So many of them.
During my exploration of the library, I found that many of the type annotations are incorrect. It greatly impacts the experience of the library.

So I dove into the code base, following hundreds of the `reportGeneralTypeIssues` diags, and bumped into some actual bugs that affects the library even if the type annotation is totally ignored.

Let me list some of the type errors in `ConversableAgent`:
- `.(a_)send` returns `None` while it claims to return `bool`.
- `.register_reply` have an `Optional` position parameter while `None` is not handled
- `.last_message` should be `Optional` since it can return `None`
- `._print_received_message` doesn't handle a `str` message. **A `self._message_to_dict` conversion is missing here**
- `.check_termination_and_human_reply`'s `sender` **defaults to `None` while nothing handles the `None` case**. Same type of problem also exists in `groupchat.py`.
- `Agent.receive` doesn't even accept keyword arguments while `ConversableAgent.receive` does, but `ConversableAgent` tries to pass `silent` to `receiver: Agent`.

The most horrible one is probably `Completion._get_response`. As far as I understand this function can return `Literal[-1]` while none of the call site handles this case.

These problems are immediately obvious when I run pyright on it.

Please consider fixing those incorrect type annotations and possibly enabling type checking in your development workflow.

PS: Sorry for not proposing this as a PR; I think some of the type errors are related to design decisions (like the `-> bool` one) and I don't know what is intended to be there.
```[tasklist]
### Tasks
- [x] .(a_)send
- [x] .register_reply
- [ ] .last_message
- [x] ._print_received_message
- [ ] .check_termination_and_human_reply
- [ ] Agent.receive
- [ ] `content`, `system_message`, and `messages` @BeibinLi
```

Hints from issue discussion:
I believe it greatly hinders my work flow as well

Commit message: Convert str message to dict before printing message

Patch to review:
diff --git a/autogen/agentchat/assistant_agent.py b/autogen/agentchat/assistant_agent.py
index 4a0200fb6720..b39b5f75f80e 100644
--- a/autogen/agentchat/assistant_agent.py
+++ b/autogen/agentchat/assistant_agent.py
@@ -1,5 +1,5 @@
 from .conversable_agent import ConversableAgent
-from typing import Callable, Dict, Optional, Union
+from typing import Callable, Dict, Literal, Optional, Union
 
 
 class AssistantAgent(ConversableAgent):
@@ -30,11 +30,11 @@ def __init__(
         self,
         name: str,
         system_message: Optional[str] = DEFAULT_SYSTEM_MESSAGE,
-        llm_config: Optional[Union[Dict, bool]] = None,
+        llm_config: Optional[Union[Dict, Literal[False]]] = None,
         is_termination_msg: Optional[Callable[[Dict], bool]] = None,
         max_consecutive_auto_reply: Optional[int] = None,
         human_input_mode: Optional[str] = "NEVER",
-        code_execution_config: Optional[Union[Dict, bool]] = False,
+        code_execution_config: Optional[Union[Dict, Literal[False]]] = False,
         **kwargs,
     ):
         """
diff --git a/autogen/agentchat/conversable_agent.py b/autogen/agentchat/conversable_agent.py
index 3146403ac6b2..e2fa103135db 100644
--- a/autogen/agentchat/conversable_agent.py
+++ b/autogen/agentchat/conversable_agent.py
@@ -3,7 +3,7 @@
 import copy
 import json
 import logging
-from typing import Any, Callable, Dict, List, Optional, Tuple, Type, Union
+from typing import Any, Callable, Dict, List, Literal, Optional, Tuple, Type, Union
 from autogen import OpenAIWrapper
 from .agent import Agent
 from autogen.code_utils import (
@@ -45,6 +45,8 @@ class ConversableAgent(Agent):
     }
     MAX_CONSECUTIVE_AUTO_REPLY = 100  # maximum number of consecutive auto replies (subject to future change)
 
+    llm_config: Union[Dict, Literal[False]]
+
     def __init__(
         self,
         name: str,
@@ -53,8 +55,8 @@ def __init__(
         max_consecutive_auto_reply: Optional[int] = None,
         human_input_mode: Optional[str] = "TERMINATE",
         function_map: Optional[Dict[str, Callable]] = None,
-        code_execution_config: Optional[Union[Dict, bool]] = None,
-        llm_config: Optional[Union[Dict, bool]] = None,
+        code_execution_config: Optional[Union[Dict, Literal[False]]] = None,
+        llm_config: Optional[Union[Dict, Literal[False]]] = None,
         default_auto_reply: Optional[Union[str, Dict, None]] = "",
     ):
         """
@@ -114,7 +116,9 @@ def __init__(
                 self.llm_config.update(llm_config)
             self.client = OpenAIWrapper(**self.llm_config)
 
-        self._code_execution_config = {} if code_execution_config is None else code_execution_config
+        self._code_execution_config: Union[Dict, Literal[False]] = (
+            {} if code_execution_config is None else code_execution_config
+        )
         self.human_input_mode = human_input_mode
         self._max_consecutive_auto_reply = (
             max_consecutive_auto_reply if max_consecutive_auto_reply is not None else self.MAX_CONSECUTIVE_AUTO_REPLY
@@ -135,7 +139,7 @@ def register_reply(
         self,
         trigger: Union[Type[Agent], str, Agent, Callable[[Agent], bool], List],
         reply_func: Callable,
-        position: Optional[int] = 0,
+        position: int = 0,
         config: Optional[Any] = None,
         reset_config: Optional[Callable] = None,
     ):
@@ -162,7 +166,7 @@ def reply_func(
             messages: Optional[List[Dict]] = None,
             sender: Optional[Agent] = None,
             config: Optional[Any] = None,
-        ) -> Union[str, Dict, None]:
+        ) -> Tuple[bool, Union[str, Dict, None]]:
         ```
             position (int): the position of the reply function in the reply function list.
                 The function registered later will be checked earlier by default.
@@ -221,7 +225,7 @@ def chat_messages(self) -> Dict[Agent, List[Dict]]:
         """A dictionary of conversations from agent to list of messages."""
         return self._oai_messages
 
-    def last_message(self, agent: Optional[Agent] = None) -> Dict:
+    def last_message(self, agent: Optional[Agent] = None) -> Optional[Dict]:
         """The last message exchanged with the agent.
 
         Args:
@@ -304,7 +308,7 @@ def send(
         recipient: Agent,
         request_reply: Optional[bool] = None,
         silent: Optional[bool] = False,
-    ) -> bool:
+    ):
         """Send a message to another agent.
 
         Args:
@@ -353,7 +357,7 @@ async def a_send(
         recipient: Agent,
         request_reply: Optional[bool] = None,
         silent: Optional[bool] = False,
-    ) -> bool:
+    ):
         """(async) Send a message to another agent.
 
         Args:
@@ -399,6 +403,8 @@ async def a_send(
     def _print_received_message(self, message: Union[Dict, str], sender: Agent):
         # print the message received
         print(colored(sender.name, "yellow"), "(to", f"{self.name}):\n", flush=True)
+        message = self._message_to_dict(message)
+
         if message.get("role") == "function":
             func_print = f"***** Response from calling function \"{message['name']}\" *****"
             print(colored(func_print, "green"), flush=True)
@@ -606,7 +612,7 @@ def generate_oai_reply(
         self,
         messages: Optional[List[Dict]] = None,
         sender: Optional[Agent] = None,
-        config: Optional[Any] = None,
+        config: Optional[OpenAIWrapper] = None,
     ) -> Tuple[bool, Union[str, Dict, None]]:
         """Generate a reply using autogen.oai."""
         client = self.client if config is None else config
@@ -625,7 +631,7 @@ def generate_code_execution_reply(
         self,
         messages: Optional[List[Dict]] = None,
         sender: Optional[Agent] = None,
-        config: Optional[Any] = None,
+        config: Optional[Union[Dict, Literal[False]]] = None,
     ):
         """Generate a reply using code execution."""
         code_execution_config = config if config is not None else self._code_execution_config
diff --git a/autogen/agentchat/groupchat.py b/autogen/agentchat/groupchat.py
index 604eb5c209db..1a592562499e 100644
--- a/autogen/agentchat/groupchat.py
+++ b/autogen/agentchat/groupchat.py
@@ -1,6 +1,6 @@
 from dataclasses import dataclass
 import sys
-from typing import Dict, List, Optional, Union
+from typing import Dict, List, Optional, Tuple, Union
 from .agent import Agent
 from .conversable_agent import ConversableAgent
 import logging
@@ -22,7 +22,7 @@ class GroupChat:
         in its `function_map`.
     """
 
-    agents: List[Agent]
+    agents: List[ConversableAgent]
     messages: List[Dict]
     max_round: int = 10
     admin_name: str = "Admin"
@@ -37,11 +37,11 @@ def reset(self):
         """Reset the group chat."""
         self.messages.clear()
 
-    def agent_by_name(self, name: str) -> Agent:
+    def agent_by_name(self, name: str) -> ConversableAgent:
         """Returns the agent with a given name."""
         return self.agents[self.agent_names.index(name)]
 
-    def next_agent(self, agent: Agent, agents: List[Agent]) -> Agent:
+    def next_agent(self, agent: Agent, agents: List[ConversableAgent]) -> ConversableAgent:
         """Return the next agent in the list."""
         if agents == self.agents:
             return agents[(self.agent_names.index(agent.name) + 1) % len(agents)]
@@ -51,7 +51,7 @@ def next_agent(self, agent: Agent, agents: List[Agent]) -> Agent:
                 if self.agents[(offset + i) % len(self.agents)] in agents:
                     return self.agents[(offset + i) % len(self.agents)]
 
-    def select_speaker_msg(self, agents: List[Agent]):
+    def select_speaker_msg(self, agents: List[ConversableAgent]):
         """Return the message for selecting the next speaker."""
         return f"""You are in a role play game. The following roles are available:
 {self._participant_roles()}.
@@ -125,7 +125,7 @@ class GroupChatManager(ConversableAgent):
     def __init__(
         self,
         groupchat: GroupChat,
-        name: Optional[str] = "chat_manager",
+        name: str = "chat_manager",
         # unlimited consecutive auto reply by default
         max_consecutive_auto_reply: Optional[int] = sys.maxsize,
         human_input_mode: Optional[str] = "NEVER",
@@ -150,7 +150,7 @@ def run_chat(
         messages: Optional[List[Dict]] = None,
         sender: Optional[Agent] = None,
         config: Optional[GroupChat] = None,
-    ) -> Union[str, Dict, None]:
+    ) -> Tuple[bool, Union[str, Dict, None]]:
         """Run a group chat."""
         if messages is None:
             messages = self._oai_messages[sender]
diff --git a/autogen/agentchat/user_proxy_agent.py b/autogen/agentchat/user_proxy_agent.py
index d72c2bdceba8..8a218b1fe492 100644
--- a/autogen/agentchat/user_proxy_agent.py
+++ b/autogen/agentchat/user_proxy_agent.py
@@ -1,5 +1,5 @@
 from .conversable_agent import ConversableAgent
-from typing import Callable, Dict, Optional, Union
+from typing import Callable, Dict, Literal, Optional, Union
 
 
 class UserProxyAgent(ConversableAgent):
@@ -22,9 +22,9 @@ def __init__(
         max_consecutive_auto_reply: Optional[int] = None,
         human_input_mode: Optional[str] = "ALWAYS",
         function_map: Optional[Dict[str, Callable]] = None,
-        code_execution_config: Optional[Union[Dict, bool]] = None,
+        code_execution_config: Optional[Union[Dict, Literal[False]]] = None,
         default_auto_reply: Optional[Union[str, Dict, None]] = "",
-        llm_config: Optional[Union[Dict, bool]] = False,
+        llm_config: Optional[Union[Dict, Literal[False]]] = False,
         system_message: Optional[str] = "",
     ):
         """


Code review comments:
diff_hunk: @@ -22,7 +22,7 @@ class GroupChat:
         in its `function_map`.
     """
 
-    agents: List[Agent]
+    agents: List[ConversableAgent]
path: autogen/agentchat/groupchat.py
text: @user2:
Good catch! For our current GroupChat design, the agents are all Conversable Agent, because we require the `system_message` member variable. 

@user1 What do you think? This change to "Conversable Agent" is accurate and reasonable. However, do we want to redesign GroupChat so that it will be compatible with Agent?

@user2:
Actually, I thought about this again after lunch. It is still better to use "Agent" here, and we may need to redesign the whole group chat.

For instance, I am going to add "DALLE" as an Agent (inherit from Agent), but it is obviously not Conversable. Then, the DALLE agent could not join the group chat, which is not ideal. It would be the same for Tool Agent, and some other non-conversable agents. @user1  

@user1:
I think Agent is better for generality.

@author:
As a user I think it will be nice to have other kinds of agents in the group chat.