From 27353da5ec03289900a9207aa4be5b44bf9d73c3 Mon Sep 17 00:00:00 2001 From: Eli Fajardo Date: Mon, 13 May 2024 17:19:35 -0400 Subject: [PATCH 1/7] fixes and unit test updates --- morpheus/llm/services/openai_chat_service.py | 9 +++++-- tests/llm/services/test_openai_chat_client.py | 25 ++++++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/morpheus/llm/services/openai_chat_service.py b/morpheus/llm/services/openai_chat_service.py index 82bdd467ab..09ed0f469e 100644 --- a/morpheus/llm/services/openai_chat_service.py +++ b/morpheus/llm/services/openai_chat_service.py @@ -326,8 +326,8 @@ def __init__(self, *, api_key: str = None, base_url: str = None, default_model_k The API key for the LLM service, by default None. If `None` the API key will be read from the `OPENAI_API_KEY` environment variable. If neither are present an error will be raised. base_url : str, optional - The api host url, by default None. If `None` the url will be read from the `OPENAI_API_BASE` environment - variable. If neither are present the OpenAI default will be used., by default None + The api host url, by default None. If the `OPENAI_BASE_URL` environment variable is present, it will always + take precedence over this parameter. If neither are present the OpenAI default will be used., by default None default_model_kwargs : dict, optional Default arguments to use when creating a client via the `get_client` function. Any argument specified here will automatically be used when calling `get_client`. Arguments specified in the `get_client` function will @@ -356,6 +356,9 @@ def __init__(self, *, api_key: str = None, base_url: str = None, default_model_k log_file = os.path.join(appdirs.user_log_dir(appauthor="NVIDIA", appname="morpheus"), "openai.log") + # Ensure the log directory exists + os.makedirs(os.path.dirname(log_file), exist_ok=True) + # Add a file handler file_handler = logging.FileHandler(log_file) @@ -398,6 +401,8 @@ def get_client(self, """ final_model_kwargs = {**self._default_model_kwargs, **model_kwargs} + del final_model_kwargs["base_url"] + del final_model_kwargs["api_key"] return OpenAIChatClient(self, model_name=model_name, diff --git a/tests/llm/services/test_openai_chat_client.py b/tests/llm/services/test_openai_chat_client.py index 577c83c7bb..67d36f8ec4 100644 --- a/tests/llm/services/test_openai_chat_client.py +++ b/tests/llm/services/test_openai_chat_client.py @@ -24,13 +24,30 @@ from morpheus.llm.services.openai_chat_service import OpenAIChatService +@pytest.mark.parametrize("api_key", ["12345", None]) +@pytest.mark.parametrize("base_url", ["http://test.openai.com/v1", None]) @pytest.mark.parametrize("max_retries", [5, 10]) -def test_constructor(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock], max_retries: int): +def test_constructor(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock], + api_key: str, + base_url: str, + max_retries: int): + client = OpenAIChatClient(OpenAIChatService(api_key=api_key, base_url=base_url), + model_name="test_model", + max_retries=max_retries) + assert isinstance(client, LLMClient) + + for mock_client in mock_chat_completion: + mock_client.assert_called_once_with(api_key=api_key, base_url=base_url, max_retries=max_retries) + + +@pytest.mark.parametrize("max_retries", [5, 10]) +def test_constructor_default_service_constructor(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock], + max_retries: int): client = OpenAIChatClient(OpenAIChatService(), model_name="test_model", max_retries=max_retries) assert isinstance(client, LLMClient) for mock_client in mock_chat_completion: - mock_client.assert_called_once_with(max_retries=max_retries) + mock_client.assert_called_once_with(api_key=None, base_url=None, max_retries=max_retries) @pytest.mark.parametrize("use_async", [True, False]) @@ -56,7 +73,9 @@ def test_generate(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock], expected_messages: list[dict], temperature: int): (mock_client, mock_async_client) = mock_chat_completion - client = OpenAIChatClient(OpenAIChatService(), + api_key = "12345" + base_url = "http://test.openai.com/v1" + client = OpenAIChatClient(OpenAIChatService(api_key=api_key, base_url=base_url), model_name="test_model", set_assistant=set_assistant, temperature=temperature) From 53dfeb6947427aeb77ab7fe0dfec600b6cb20e60 Mon Sep 17 00:00:00 2001 From: Eli Fajardo Date: Mon, 13 May 2024 17:44:20 -0400 Subject: [PATCH 2/7] pylint fixes to openai_chat_service.py --- morpheus/llm/services/openai_chat_service.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/morpheus/llm/services/openai_chat_service.py b/morpheus/llm/services/openai_chat_service.py index 09ed0f469e..43008d214d 100644 --- a/morpheus/llm/services/openai_chat_service.py +++ b/morpheus/llm/services/openai_chat_service.py @@ -114,7 +114,9 @@ def __init__(self, # Create the client objects for both sync and async self._client = openai.OpenAI(api_key=parent._api_key, base_url=parent._base_url, max_retries=max_retries) - self._client_async = openai.AsyncOpenAI(api_key=parent._api_key, base_url=parent._base_url, max_retries=max_retries) + self._client_async = openai.AsyncOpenAI(api_key=parent._api_key, + base_url=parent._base_url, + max_retries=max_retries) def get_input_names(self) -> list[str]: input_names = [self._prompt_key] @@ -326,8 +328,9 @@ def __init__(self, *, api_key: str = None, base_url: str = None, default_model_k The API key for the LLM service, by default None. If `None` the API key will be read from the `OPENAI_API_KEY` environment variable. If neither are present an error will be raised. base_url : str, optional - The api host url, by default None. If the `OPENAI_BASE_URL` environment variable is present, it will always - take precedence over this parameter. If neither are present the OpenAI default will be used., by default None + The api host url, by default None. If the `OPENAI_BASE_URL` environment variable is present, + it will always take precedence over this parameter. If neither are present the OpenAI default will + be used., by default None default_model_kwargs : dict, optional Default arguments to use when creating a client via the `get_client` function. Any argument specified here will automatically be used when calling `get_client`. Arguments specified in the `get_client` function will From 44cabe4ee9c71d86db6a1d7072e0187e865bdeba Mon Sep 17 00:00:00 2001 From: Eli Fajardo Date: Mon, 13 May 2024 18:06:58 -0400 Subject: [PATCH 3/7] revert test_generate updates --- tests/llm/services/test_openai_chat_client.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/llm/services/test_openai_chat_client.py b/tests/llm/services/test_openai_chat_client.py index 67d36f8ec4..da4431f01f 100644 --- a/tests/llm/services/test_openai_chat_client.py +++ b/tests/llm/services/test_openai_chat_client.py @@ -73,9 +73,7 @@ def test_generate(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock], expected_messages: list[dict], temperature: int): (mock_client, mock_async_client) = mock_chat_completion - api_key = "12345" - base_url = "http://test.openai.com/v1" - client = OpenAIChatClient(OpenAIChatService(api_key=api_key, base_url=base_url), + client = OpenAIChatClient(OpenAIChatService(), model_name="test_model", set_assistant=set_assistant, temperature=temperature) From f4cc093ac44625f56e8dcc3c761acc93f95cf869 Mon Sep 17 00:00:00 2001 From: Eli Fajardo Date: Mon, 13 May 2024 18:07:38 -0400 Subject: [PATCH 4/7] fix model_kwarg key deletes --- morpheus/llm/services/openai_chat_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/morpheus/llm/services/openai_chat_service.py b/morpheus/llm/services/openai_chat_service.py index 43008d214d..76335181f7 100644 --- a/morpheus/llm/services/openai_chat_service.py +++ b/morpheus/llm/services/openai_chat_service.py @@ -404,8 +404,8 @@ def get_client(self, """ final_model_kwargs = {**self._default_model_kwargs, **model_kwargs} - del final_model_kwargs["base_url"] - del final_model_kwargs["api_key"] + final_model_kwargs.pop("base_url", None) + final_model_kwargs.pop("api_key", None) return OpenAIChatClient(self, model_name=model_name, From d172d0119a6adee30add217a829cef646c9496af Mon Sep 17 00:00:00 2001 From: Eli Fajardo Date: Tue, 14 May 2024 14:57:48 -0400 Subject: [PATCH 5/7] pr feedback updates --- morpheus/llm/services/openai_chat_service.py | 7 ++--- tests/llm/services/test_openai_chat_client.py | 27 +++++++------------ 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/morpheus/llm/services/openai_chat_service.py b/morpheus/llm/services/openai_chat_service.py index 76335181f7..8abfa578ae 100644 --- a/morpheus/llm/services/openai_chat_service.py +++ b/morpheus/llm/services/openai_chat_service.py @@ -328,9 +328,8 @@ def __init__(self, *, api_key: str = None, base_url: str = None, default_model_k The API key for the LLM service, by default None. If `None` the API key will be read from the `OPENAI_API_KEY` environment variable. If neither are present an error will be raised. base_url : str, optional - The api host url, by default None. If the `OPENAI_BASE_URL` environment variable is present, - it will always take precedence over this parameter. If neither are present the OpenAI default will - be used., by default None + The api host url, by default None. If `None` the url will be read from the `OPENAI_BASE_URL` environment + variable. If neither are present the OpenAI default will be used., by default None default_model_kwargs : dict, optional Default arguments to use when creating a client via the `get_client` function. Any argument specified here will automatically be used when calling `get_client`. Arguments specified in the `get_client` function will @@ -404,8 +403,6 @@ def get_client(self, """ final_model_kwargs = {**self._default_model_kwargs, **model_kwargs} - final_model_kwargs.pop("base_url", None) - final_model_kwargs.pop("api_key", None) return OpenAIChatClient(self, model_name=model_name, diff --git a/tests/llm/services/test_openai_chat_client.py b/tests/llm/services/test_openai_chat_client.py index da4431f01f..4976eb1655 100644 --- a/tests/llm/services/test_openai_chat_client.py +++ b/tests/llm/services/test_openai_chat_client.py @@ -19,8 +19,6 @@ import pytest from _utils.llm import mk_mock_openai_response -from morpheus.llm.services.llm_service import LLMClient -from morpheus.llm.services.openai_chat_service import OpenAIChatClient from morpheus.llm.services.openai_chat_service import OpenAIChatService @@ -31,10 +29,7 @@ def test_constructor(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock] api_key: str, base_url: str, max_retries: int): - client = OpenAIChatClient(OpenAIChatService(api_key=api_key, base_url=base_url), - model_name="test_model", - max_retries=max_retries) - assert isinstance(client, LLMClient) + OpenAIChatService(api_key=api_key, base_url=base_url).get_client(model_name="test_model", max_retries=max_retries) for mock_client in mock_chat_completion: mock_client.assert_called_once_with(api_key=api_key, base_url=base_url, max_retries=max_retries) @@ -43,8 +38,7 @@ def test_constructor(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock] @pytest.mark.parametrize("max_retries", [5, 10]) def test_constructor_default_service_constructor(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock], max_retries: int): - client = OpenAIChatClient(OpenAIChatService(), model_name="test_model", max_retries=max_retries) - assert isinstance(client, LLMClient) + OpenAIChatService().get_client(model_name="test_model", max_retries=max_retries) for mock_client in mock_chat_completion: mock_client.assert_called_once_with(api_key=None, base_url=None, max_retries=max_retries) @@ -73,10 +67,10 @@ def test_generate(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMock], expected_messages: list[dict], temperature: int): (mock_client, mock_async_client) = mock_chat_completion - client = OpenAIChatClient(OpenAIChatService(), - model_name="test_model", - set_assistant=set_assistant, - temperature=temperature) + client = OpenAIChatService().get_client(model_name="test_model", + set_assistant=set_assistant, + temperature=temperature) + if use_async: results = asyncio.run(client.generate_async(**input_dict)) mock_async_client.chat.completions.create.assert_called_once_with(model="test_model", @@ -125,10 +119,9 @@ def test_generate_batch(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMo expected_messages: list[list[dict]], temperature: int): (mock_client, mock_async_client) = mock_chat_completion - client = OpenAIChatClient(OpenAIChatService(), - model_name="test_model", - set_assistant=set_assistant, - temperature=temperature) + client = OpenAIChatService().get_client(model_name="test_model", + set_assistant=set_assistant, + temperature=temperature) expected_results = ["test_output" for _ in range(len(inputs["prompt"]))] expected_calls = [ @@ -151,7 +144,7 @@ def test_generate_batch(mock_chat_completion: tuple[mock.MagicMock, mock.MagicMo @pytest.mark.parametrize("completion", [[], [None]], ids=["no_choices", "no_content"]) @pytest.mark.usefixtures("mock_chat_completion") def test_extract_completion_errors(completion: list): - client = OpenAIChatClient(OpenAIChatService(), model_name="test_model") + client = OpenAIChatService().get_client(model_name="test_model") mock_completion = mk_mock_openai_response(completion) with pytest.raises(ValueError): From ea8e9af2e13b152001c362bd56abb950f2ddc860 Mon Sep 17 00:00:00 2001 From: Eli Fajardo Date: Wed, 15 May 2024 16:20:28 -0400 Subject: [PATCH 6/7] test base_url and api_key --- tests/llm/services/conftest.py | 2 +- tests/llm/services/test_openai_chat_client.py | 45 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/tests/llm/services/conftest.py b/tests/llm/services/conftest.py index 8e128406b2..11c5d8f27c 100644 --- a/tests/llm/services/conftest.py +++ b/tests/llm/services/conftest.py @@ -36,7 +36,7 @@ def openai_fixture(openai): yield openai -@pytest.fixture(name="mock_chat_completion", autouse=True) +@pytest.fixture(name="mock_chat_completion", autouse=False) def mock_chat_completion_fixture(mock_chat_completion): yield mock_chat_completion diff --git a/tests/llm/services/test_openai_chat_client.py b/tests/llm/services/test_openai_chat_client.py index 4976eb1655..aa28643afa 100644 --- a/tests/llm/services/test_openai_chat_client.py +++ b/tests/llm/services/test_openai_chat_client.py @@ -14,6 +14,8 @@ # limitations under the License. import asyncio +import openai +import os from unittest import mock import pytest @@ -44,6 +46,49 @@ def test_constructor_default_service_constructor(mock_chat_completion: tuple[moc mock_client.assert_called_once_with(api_key=None, base_url=None, max_retries=max_retries) +@pytest.mark.usefixtures("openai") +@pytest.mark.parametrize('use_env', [True, False]) +def test_constructor_api_key_base_url(use_env: bool): + + if use_env: + env_api_key = "env-12345" + env_base_url = "http://env.openai.com/v1/" + os.environ["OPENAI_API_KEY"] = env_api_key + os.environ["OPENAI_BASE_URL"] = env_base_url + + # Test when api_key and base_url are not passed + client = OpenAIChatService().get_client(model_name="test_model") + assert client._client.api_key == env_api_key + assert str(client._client.base_url) == env_base_url + + # Test when api_key and base_url are passed + arg_api_key = "arg-12345" + arg_base_url = "http://arg.openai.com/v1/" + client = OpenAIChatService(api_key=arg_api_key, base_url=arg_base_url).get_client(model_name="test_model") + assert client._client.api_key == arg_api_key + assert str(client._client.base_url) == arg_base_url + else: + os.environ.pop("OPENAI_API_KEY") + os.environ.pop("OPENAI_BASE_URL") + # Test when api_key and base_url are not passed + with pytest.raises(openai.OpenAIError) as excinfo: + client = OpenAIChatService().get_client(model_name="test_model") + + assert "api_key client option must be set" in str(excinfo.value) + + # Test when only api_key is passed + arg_api_key = "arg-12345" + client = OpenAIChatService(api_key=arg_api_key).get_client(model_name="test_model") + assert client._client.api_key == arg_api_key + assert str(client._client.base_url) == "https://api.openai.com/v1/" + + # Test when both api_key and base_url are passed + arg_base_url = "http://arg.openai.com/v1/" + client = OpenAIChatService(api_key=arg_api_key, base_url=arg_base_url).get_client(model_name="test_model") + assert client._client.api_key == arg_api_key + assert str(client._client.base_url) == arg_base_url + + @pytest.mark.parametrize("use_async", [True, False]) @pytest.mark.parametrize( "input_dict, set_assistant, expected_messages", From 889549743fdd3faffff7c28cb6df14c397675ed8 Mon Sep 17 00:00:00 2001 From: Eli Fajardo Date: Wed, 15 May 2024 16:49:29 -0400 Subject: [PATCH 7/7] pr feedback update --- tests/llm/services/conftest.py | 2 +- tests/llm/services/test_openai_chat_client.py | 45 ------------------- 2 files changed, 1 insertion(+), 46 deletions(-) diff --git a/tests/llm/services/conftest.py b/tests/llm/services/conftest.py index 11c5d8f27c..8e128406b2 100644 --- a/tests/llm/services/conftest.py +++ b/tests/llm/services/conftest.py @@ -36,7 +36,7 @@ def openai_fixture(openai): yield openai -@pytest.fixture(name="mock_chat_completion", autouse=False) +@pytest.fixture(name="mock_chat_completion", autouse=True) def mock_chat_completion_fixture(mock_chat_completion): yield mock_chat_completion diff --git a/tests/llm/services/test_openai_chat_client.py b/tests/llm/services/test_openai_chat_client.py index aa28643afa..4976eb1655 100644 --- a/tests/llm/services/test_openai_chat_client.py +++ b/tests/llm/services/test_openai_chat_client.py @@ -14,8 +14,6 @@ # limitations under the License. import asyncio -import openai -import os from unittest import mock import pytest @@ -46,49 +44,6 @@ def test_constructor_default_service_constructor(mock_chat_completion: tuple[moc mock_client.assert_called_once_with(api_key=None, base_url=None, max_retries=max_retries) -@pytest.mark.usefixtures("openai") -@pytest.mark.parametrize('use_env', [True, False]) -def test_constructor_api_key_base_url(use_env: bool): - - if use_env: - env_api_key = "env-12345" - env_base_url = "http://env.openai.com/v1/" - os.environ["OPENAI_API_KEY"] = env_api_key - os.environ["OPENAI_BASE_URL"] = env_base_url - - # Test when api_key and base_url are not passed - client = OpenAIChatService().get_client(model_name="test_model") - assert client._client.api_key == env_api_key - assert str(client._client.base_url) == env_base_url - - # Test when api_key and base_url are passed - arg_api_key = "arg-12345" - arg_base_url = "http://arg.openai.com/v1/" - client = OpenAIChatService(api_key=arg_api_key, base_url=arg_base_url).get_client(model_name="test_model") - assert client._client.api_key == arg_api_key - assert str(client._client.base_url) == arg_base_url - else: - os.environ.pop("OPENAI_API_KEY") - os.environ.pop("OPENAI_BASE_URL") - # Test when api_key and base_url are not passed - with pytest.raises(openai.OpenAIError) as excinfo: - client = OpenAIChatService().get_client(model_name="test_model") - - assert "api_key client option must be set" in str(excinfo.value) - - # Test when only api_key is passed - arg_api_key = "arg-12345" - client = OpenAIChatService(api_key=arg_api_key).get_client(model_name="test_model") - assert client._client.api_key == arg_api_key - assert str(client._client.base_url) == "https://api.openai.com/v1/" - - # Test when both api_key and base_url are passed - arg_base_url = "http://arg.openai.com/v1/" - client = OpenAIChatService(api_key=arg_api_key, base_url=arg_base_url).get_client(model_name="test_model") - assert client._client.api_key == arg_api_key - assert str(client._client.base_url) == arg_base_url - - @pytest.mark.parametrize("use_async", [True, False]) @pytest.mark.parametrize( "input_dict, set_assistant, expected_messages",