From cff9ca9a11f4943fee95436a4db7dcf5e634a808 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 14 Feb 2024 20:51:22 -0800 Subject: [PATCH] do model check properly (#1686) * do model check properly * bug in logging test * set llm_config --- autogen/agentchat/conversable_agent.py | 12 ++-- autogen/oai/client.py | 18 ++---- .../contrib/test_compressible_agent.py | 10 ++-- test/agentchat/test_agent_logging.py | 2 +- test/agentchat/test_conversable_agent.py | 60 +++++++------------ 5 files changed, 40 insertions(+), 62 deletions(-) diff --git a/autogen/agentchat/conversable_agent.py b/autogen/agentchat/conversable_agent.py index 2f545f1e78a8..0ba44ef23069 100644 --- a/autogen/agentchat/conversable_agent.py +++ b/autogen/agentchat/conversable_agent.py @@ -144,11 +144,13 @@ 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'.") + if "model" not in self.llm_config and ( + not self.llm_config.get("config_list") + or any(not config.get("model") for config in self.llm_config["config_list"]) + ): + raise ValueError( + "Please either set llm_config to False, or specify a non-empty 'model' either in 'llm_config' or in each config of 'config_list'." + ) self.client = OpenAIWrapper(**self.llm_config) if logging_enabled(): diff --git a/autogen/oai/client.py b/autogen/oai/client.py index 0e9d24ed3ac5..ccde5f0a6a31 100644 --- a/autogen/oai/client.py +++ b/autogen/oai/client.py @@ -361,13 +361,8 @@ 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.") - # 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'.") + # It's OK if "model" is not provided in base_config or config_list + # Because one can provide "model" at `create` time. self._clients: List[ModelClient] = [] self._config_list: List[Dict[str, Any]] = [] @@ -375,13 +370,6 @@ def __init__(self, *, config_list: Optional[List[Dict[str, Any]]] = None, **base 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}} @@ -610,6 +598,7 @@ def yes_or_no_filter(context, response): if logging_enabled(): # Log the cache hit + # TODO: log the config_id and pass_filter etc. log_chat_completion( invocation_id=invocation_id, client_id=id(client), @@ -671,6 +660,7 @@ def yes_or_no_filter(context, response): cache.set(key, response) if logging_enabled(): + # TODO: log the config_id and pass_filter etc. log_chat_completion( invocation_id=invocation_id, client_id=id(client), diff --git a/test/agentchat/contrib/test_compressible_agent.py b/test/agentchat/contrib/test_compressible_agent.py index c5ef784f64b4..2d41587aa323 100644 --- a/test/agentchat/contrib/test_compressible_agent.py +++ b/test/agentchat/contrib/test_compressible_agent.py @@ -216,14 +216,14 @@ def test_mode_terminate(): reason="do not run on MacOS or windows OR dependency is not installed OR requested to skip", ) def test_new_compressible_agent_description(): - assistant = CompressibleAgent(name="assistant", description="this is a description") + assistant = CompressibleAgent(name="assistant", description="this is a description", llm_config=False) assert assistant.description == "this is a description", "description is not set correctly" if __name__ == "__main__": - test_mode_compress() - test_mode_customized() - test_compress_message() - test_mode_terminate() + # test_mode_compress() + # test_mode_customized() + # test_compress_message() + # test_mode_terminate() test_new_compressible_agent_description() diff --git a/test/agentchat/test_agent_logging.py b/test/agentchat/test_agent_logging.py index 3c99867b5fd4..1cd2629f3bf8 100644 --- a/test/agentchat/test_agent_logging.py +++ b/test/agentchat/test_agent_logging.py @@ -208,7 +208,7 @@ def test_groupchat_logging(db_connection): # Verify chat_completions message cur.execute(CHAT_COMPLETIONS_QUERY) rows = cur.fetchall() - assert len(rows) == 2 # max_round - 1 + assert len(rows) >= 2 # some config may fail # Verify group chat manager agent cur.execute(AGENTS_QUERY) diff --git a/test/agentchat/test_conversable_agent.py b/test/agentchat/test_conversable_agent.py index 5249fa20ab45..8fb6ad327938 100644 --- a/test/agentchat/test_conversable_agent.py +++ b/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={"config_list": [{"model": "gpt-4", "api_key": ""}]}) + agent = ConversableAgent(name="agent", llm_config={"config_list": [{"model": "gpt-4"}]}) 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": [{"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 = ConversableAgent(name="agent3", llm_config={"config_list": [{"model": "gpt-4"}]}) + agent2 = ConversableAgent(name="agent2", llm_config={"config_list": [{"model": "gpt-4"}]}) + agent1 = ConversableAgent(name="agent1", llm_config={"config_list": [{"model": "gpt-4"}]}) @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": [{"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 = ConversableAgent(name="agent3", llm_config={"config_list": [{"model": "gpt-4"}]}) + agent2 = ConversableAgent(name="agent2", llm_config={"config_list": [{"model": "gpt-4"}]}) + agent1 = ConversableAgent(name="agent1", llm_config={"config_list": [{"model": "gpt-4"}]}) @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={"config_list": [{"model": "gpt-4", "api_key": ""}]}) + agent = ConversableAgent(name="agent", llm_config={"config_list": [{"model": "gpt-4"}]}) with pytest.raises(ValueError) as e: @@ -774,33 +774,33 @@ def exec_python(cell: Annotated[str, "Valid Python cell to execute."]) -> str: def test_register_for_llm_without_LLM(): - try: + with pytest.raises( + ValueError, + match="Please either set llm_config to False, or specify a non-empty 'model' either in 'llm_config' or in each config of 'config_list'.", + ): 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'." def test_register_for_llm_without_configuration(): - try: + with pytest.raises( + ValueError, + match="Please either set llm_config to False, or specify a non-empty 'model' either in 'llm_config' or in each config of 'config_list'.", + ): 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'." 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'." + with pytest.raises( + ValueError, + match="Please either set llm_config to False, or specify a non-empty 'model' either in 'llm_config' or in each config of 'config_list'.", + ): + ConversableAgent(name="agent", llm_config={"config_list": [{"model": ""}]}) 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": [{"model": "gpt-4", "api_key": ""}]}) + agent = ConversableAgent(name="agent", llm_config={"config_list": [{"model": "gpt-4"}]}) user_proxy_1 = UserProxyAgent(name="user_proxy_1") user_proxy_2 = UserProxyAgent(name="user_proxy_2") @@ -835,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": [{"model": "gpt-4", "api_key": ""}]}) + agent = ConversableAgent(name="agent", llm_config={"config_list": [{"model": "gpt-4"}]}) user_proxy = UserProxyAgent(name="user_proxy") def exec_python(cell: Annotated[str, "Valid Python cell to execute."]) -> str: @@ -1028,20 +1028,6 @@ def stopwatch(num_seconds: Annotated[str, "Number of seconds in the stopwatch."] stopwatch_mock.assert_called_once_with(num_seconds="5") -@pytest.mark.skipif( - skip, - reason="do not run if skipping openai", -) -def test_no_llm_config(): - # We expect a TypeError when the model isn't specified - with pytest.raises(TypeError, match=r".*Missing required arguments.*"): - agent1 = ConversableAgent(name="agent1", llm_config=False, human_input_mode="NEVER", default_auto_reply="") - agent2 = ConversableAgent( - name="agent2", llm_config={"api_key": "Intentionally left blank."}, human_input_mode="NEVER" - ) - agent1.initiate_chat(agent2, message="hi") - - if __name__ == "__main__": # test_trigger() # test_context()