Validate llm_config passed to ConversableAgent (issue microsoft#1522) (…

…microsoft#1654)

* Validate llm_config passed to ConversableAgent

Based on microsoft#1522, this commit implements the additional validation checks in
`ConversableAgent.`

Add the following validation and `raise ValueError` if:

 - The `llm_config` is `None`.
 - The `llm_config` is valid, but `config_list` is missing or lacks elements.
 - The `config_list` is valid, but no `model` is specified.

The rest of the changes are code churn to adjust or add the test cases.

* Validate llm_config passed to ConversableAgent

Based on microsoft#1522, this commit implements the additional validation checks in
`ConversableAgent.`

Add the following validation and `raise ValueError` if:

 - The `llm_config` is `None` (validated in `ConversableAgent`).
 - The `llm_config` has no `model` specified and `config_list` is empty
   (validated in `OpenAIWrapper`).
 - The `config_list` has at least one entry, but not all the entries have
   the `model` is specified (validated in `OpenAIWrapper`).

The rest of the changes are code churn to adjust or add the test cases.

* Validate llm_config passed to ConversableAgent

Based on microsoft#1522, this commit implements the additional validation checks in
`ConversableAgent.`

Add the following validation and `raise ValueError` if:

 - The `llm_config` is `None` (validated in `ConversableAgent`).
 - The `llm_config` has no `model` specified and `config_list` is empty
   (validated in `OpenAIWrapper`).
 - The `config_list` has at least one entry, but not all the entries have
   the `model` is specified (validated in `OpenAIWrapper`).

The rest of the changes are code churn to adjust or add the test cases.

* Validate llm_config passed to ConversableAgent

Based on microsoft#1522, this commit implements the additional validation checks in
`ConversableAgent.`

Add the following validation and `raise ValueError` if:

 - The `llm_config` is `None` (validated in `ConversableAgent`).
 - The `llm_config` has no `model` specified and `config_list` is empty
   (validated in `OpenAIWrapper`).
 - The `config_list` has at least one entry, but not all the entries have
   the `model` is specified (validated in `OpenAIWrapper`).

The rest of the changes are code churn to adjust or add the test cases.

* Validate llm_config passed to ConversableAgent

Based on microsoft#1522, this commit implements the additional validation checks in
`ConversableAgent.`

Add the following validation and `raise ValueError` if:

 - The `llm_config` is `None` (validated in `ConversableAgent`).
 - The `llm_config` has no `model` specified and `config_list` is empty
   (validated in `OpenAIWrapper`).
 - The `config_list` has at least one entry, but not all the entries have
   the `model` is specified (validated in `OpenAIWrapper`).

The rest of the changes are code churn to adjust or add the test cases.

* Fix the test_web_surfer issue

For anyone reading this: you need to `pip install markdownify` for the
`import WebSurferAgent` to succeed. That is needed to run the
`test_web_surfer.py` locally.

Test logic needs `llm_config` that is not `None` and that is not
`False`.

Let us pray that this works as part of GitHub actions ...

* One more fix for llm_config validation contract

autogen/agentchat/assistant_agent.py

@@ -46,7 +46,7 @@ def __init__(
             name (str): agent name.
             system_message (str): system message for the ChatCompletion inference.
                 Please override this attribute if you want to reprogram the agent.
-            llm_config (dict): llm inference configuration.
+            llm_config (dict or False or None): llm inference configuration.
                 Please refer to [OpenAIWrapper.create](/docs/reference/oai/client#create)
                 for available options.
             is_termination_msg (function): a function that takes a message in the form of a dictionary

autogen/agentchat/conversable_agent.py

@@ -80,7 +80,7 @@ def __init__(
         function_map: Optional[Dict[str, Callable]] = None,
         code_execution_config: Union[Dict, Literal[False]] = False,
         llm_config: Optional[Union[Dict, Literal[False]]] = None,
-        default_auto_reply: Optional[Union[str, Dict, None]] = "",
+        default_auto_reply: Union[str, Dict] = "",
         description: Optional[str] = None,
     ):
         """
@@ -118,11 +118,11 @@ def __init__(
                 - timeout (Optional, int): The maximum execution time in seconds.
                 - last_n_messages (Experimental, int or str): The number of messages to look back for code execution.
                     If set to 'auto', it will scan backwards through all messages arriving since the agent last spoke, which is typically the last time execution was attempted. (Default: auto)
-            llm_config (dict or False): llm inference configuration.
+            llm_config (dict or False or None): llm inference configuration.
                 Please refer to [OpenAIWrapper.create](/docs/reference/oai/client#create)
                 for available options.
                 To disable llm-based auto reply, set to False.
-            default_auto_reply (str or dict or None): default auto reply when no code execution or llm-based reply is generated.
+            default_auto_reply (str or dict): default auto reply when no code execution or llm-based reply is generated.
             description (str): a short description of the agent. This description is used by other agents
                 (e.g. the GroupChatManager) to decide when to call upon this agent. (Default: system_message)
         """
@@ -144,6 +144,11 @@ def __init__(
             self.llm_config = self.DEFAULT_CONFIG.copy()
             if isinstance(llm_config, dict):
                 self.llm_config.update(llm_config)
+            # We still have a default `llm_config` because the user didn't
+            # specify anything. This won't work, so raise an error to avoid
+            # an obscure message from the OpenAI service.
+            if self.llm_config == {}:
+                raise ValueError("Please specify the value for 'llm_config'.")
             self.client = OpenAIWrapper(**self.llm_config)
 
         if logging_enabled():

autogen/oai/client.py

@@ -361,15 +361,27 @@ def __init__(self, *, config_list: Optional[List[Dict[str, Any]]] = None, **base
         if logging_enabled():
             log_new_wrapper(self, locals())
         openai_config, extra_kwargs = self._separate_openai_config(base_config)
+        # This *may* work if the `llm_config` has specified the `model` attribute,
+        # so just warn here.
         if type(config_list) is list and len(config_list) == 0:
-            logger.warning("openai client was provided with an empty config_list, which may not be intended.")
+            logger.warning("OpenAI client was provided with an empty config_list, which may not be intended.")
+            # If the `llm_config` has no `model` then the call will fail. Abort now.
+            if "model" not in extra_kwargs:
+                raise ValueError("Please specify a value for the 'model' in 'llm_config'.")
 
         self._clients: List[ModelClient] = []
         self._config_list: List[Dict[str, Any]] = []
 
         if config_list:
             config_list = [config.copy() for config in config_list]  # make a copy before modifying
             for config in config_list:
+                # We require that each element of `config_list` has a non-empty value
+                # for `model` specified unless `extra_kwargs` contains "model".
+                model = None
+                if "model" in config:
+                    model = config["model"]
+                if "model" not in extra_kwargs and (model is None or len(model) == 0):
+                    raise ValueError("Please specify a non-empty 'model' value for every item in 'config_list'.")
                 self._register_default_client(config, openai_config)  # could modify the config
                 self._config_list.append(
                     {**extra_kwargs, **{k: v for k, v in config.items() if k not in self.openai_kwargs}}

test/agentchat/contrib/test_web_surfer.py

@@ -51,7 +51,9 @@ def test_web_surfer() -> None:
         mp.setenv("OPENAI_API_KEY", MOCK_OPEN_AI_API_KEY)
         page_size = 4096
         web_surfer = WebSurferAgent(
-            "web_surfer", llm_config={"config_list": []}, browser_config={"viewport_size": page_size}
+            "web_surfer",
+            llm_config={"model": "gpt-4", "config_list": []},
+            browser_config={"viewport_size": page_size},
         )
 
         # Sneak a peak at the function map, allowing us to call the functions for testing here

test/agentchat/test_conversable_agent.py

@@ -474,7 +474,7 @@ async def test_a_generate_reply_raises_on_messages_and_sender_none(conversable_a
 def test_update_function_signature_and_register_functions() -> None:
     with pytest.MonkeyPatch.context() as mp:
         mp.setenv("OPENAI_API_KEY", MOCK_OPEN_AI_API_KEY)
-        agent = ConversableAgent(name="agent", llm_config={})
+        agent = ConversableAgent(name="agent", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
 
         def exec_python(cell: str) -> None:
             pass
@@ -618,9 +618,9 @@ def get_origin(d: Dict[str, Callable[..., Any]]) -> Dict[str, Callable[..., Any]
 def test_register_for_llm():
     with pytest.MonkeyPatch.context() as mp:
         mp.setenv("OPENAI_API_KEY", MOCK_OPEN_AI_API_KEY)
-        agent3 = ConversableAgent(name="agent3", llm_config={"config_list": []})
-        agent2 = ConversableAgent(name="agent2", llm_config={"config_list": []})
-        agent1 = ConversableAgent(name="agent1", llm_config={"config_list": []})
+        agent3 = ConversableAgent(name="agent3", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
+        agent2 = ConversableAgent(name="agent2", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
+        agent1 = ConversableAgent(name="agent1", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
 
         @agent3.register_for_llm()
         @agent2.register_for_llm(name="python")
@@ -691,9 +691,9 @@ async def exec_sh(script: Annotated[str, "Valid shell script to execute."]) -> s
 def test_register_for_llm_api_style_function():
     with pytest.MonkeyPatch.context() as mp:
         mp.setenv("OPENAI_API_KEY", MOCK_OPEN_AI_API_KEY)
-        agent3 = ConversableAgent(name="agent3", llm_config={"config_list": []})
-        agent2 = ConversableAgent(name="agent2", llm_config={"config_list": []})
-        agent1 = ConversableAgent(name="agent1", llm_config={"config_list": []})
+        agent3 = ConversableAgent(name="agent3", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
+        agent2 = ConversableAgent(name="agent2", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
+        agent1 = ConversableAgent(name="agent1", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
 
         @agent3.register_for_llm(api_style="function")
         @agent2.register_for_llm(name="python", api_style="function")
@@ -762,7 +762,7 @@ async def exec_sh(script: Annotated[str, "Valid shell script to execute."]) -> s
 def test_register_for_llm_without_description():
     with pytest.MonkeyPatch.context() as mp:
         mp.setenv("OPENAI_API_KEY", MOCK_OPEN_AI_API_KEY)
-        agent = ConversableAgent(name="agent", llm_config={})
+        agent = ConversableAgent(name="agent", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
 
         with pytest.raises(ValueError) as e:
 
@@ -774,25 +774,33 @@ def exec_python(cell: Annotated[str, "Valid Python cell to execute."]) -> str:
 
 
 def test_register_for_llm_without_LLM():
-    with pytest.MonkeyPatch.context() as mp:
-        mp.setenv("OPENAI_API_KEY", MOCK_OPEN_AI_API_KEY)
-        agent = ConversableAgent(name="agent", llm_config=None)
-        agent.llm_config = None
-        assert agent.llm_config is None
+    try:
+        ConversableAgent(name="agent", llm_config=None)
+        assert False, "Expected ConversableAgent to throw ValueError."
+    except ValueError as e:
+        assert e.args[0] == "Please specify the value for 'llm_config'."
 
-        with pytest.raises(RuntimeError) as e:
 
-            @agent.register_for_llm(description="run cell in ipython and return the execution result.")
-            def exec_python(cell: Annotated[str, "Valid Python cell to execute."]) -> str:
-                pass
+def test_register_for_llm_without_configuration():
+    try:
+        ConversableAgent(name="agent", llm_config={"config_list": []})
+        assert False, "Expected ConversableAgent to throw ValueError."
+    except ValueError as e:
+        assert e.args[0] == "Please specify a value for the 'model' in 'llm_config'."
+
 
-        assert e.value.args[0] == "LLM config must be setup before registering a function for LLM."
+def test_register_for_llm_without_model_name():
+    try:
+        ConversableAgent(name="agent", llm_config={"config_list": [{"model": "", "api_key": ""}]})
+        assert False, "Expected ConversableAgent to throw ValueError."
+    except ValueError as e:
+        assert e.args[0] == "Please specify a non-empty 'model' value for every item in 'config_list'."
 
 
 def test_register_for_execution():
     with pytest.MonkeyPatch.context() as mp:
         mp.setenv("OPENAI_API_KEY", MOCK_OPEN_AI_API_KEY)
-        agent = ConversableAgent(name="agent", llm_config={"config_list": []})
+        agent = ConversableAgent(name="agent", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
         user_proxy_1 = UserProxyAgent(name="user_proxy_1")
         user_proxy_2 = UserProxyAgent(name="user_proxy_2")
 
@@ -827,7 +835,7 @@ async def exec_sh(script: Annotated[str, "Valid shell script to execute."]):
 def test_register_functions():
     with pytest.MonkeyPatch.context() as mp:
         mp.setenv("OPENAI_API_KEY", MOCK_OPEN_AI_API_KEY)
-        agent = ConversableAgent(name="agent", llm_config={"config_list": []})
+        agent = ConversableAgent(name="agent", llm_config={"config_list": [{"model": "gpt-4", "api_key": ""}]})
         user_proxy = UserProxyAgent(name="user_proxy")
 
         def exec_python(cell: Annotated[str, "Valid Python cell to execute."]) -> str: