From 5d15e422e890cac8d795ba673cb33ff572aec218 Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 24 Oct 2023 11:55:07 +0300 Subject: [PATCH 1/8] [test] Added test the compares the default config with code defaults Fixed a few cases where it wasn't --- src/resin/chat_engine/chat_engine.py | 3 +- .../query_generator/function_calling.py | 3 +- src/resin/knowledge_base/knowledge_base.py | 4 +- tests/unit/utils/test_config.py | 56 ++++++++++++++++++- 4 files changed, 61 insertions(+), 5 deletions(-) diff --git a/src/resin/chat_engine/chat_engine.py b/src/resin/chat_engine/chat_engine.py index fa1aee48..f0282322 100644 --- a/src/resin/chat_engine/chat_engine.py +++ b/src/resin/chat_engine/chat_engine.py @@ -20,7 +20,8 @@ DEFAULT_SYSTEM_PROMPT = """Use the following pieces of context to answer the user question at the next messages. This context retrieved from a knowledge database and you should use only the facts from the context to answer. Always remember to include the source to the documents you used from their 'source' field in the format 'Source: $SOURCE_HERE'. If you don't know the answer, just say that you don't know, don't try to make up an answer, use the context. -Don't address the context directly, but use it to answer the user question like it's your own knowledge.""" # noqa +Don't address the context directly, but use it to answer the user question like it's your own knowledge. +""" # noqa class BaseChatEngine(ABC, ConfigurableMixin): diff --git a/src/resin/chat_engine/query_generator/function_calling.py b/src/resin/chat_engine/query_generator/function_calling.py index 19c8d93c..b323774c 100644 --- a/src/resin/chat_engine/query_generator/function_calling.py +++ b/src/resin/chat_engine/query_generator/function_calling.py @@ -9,7 +9,8 @@ from resin.models.data_models import Messages, Query DEFAULT_SYSTEM_PROMPT = """Your task is to formulate search queries for a search engine, to assist in responding to the user's question. -You should break down complex questions into sub-queries if needed.""" # noqa: E501 +You should break down complex questions into sub-queries if needed. +""" # noqa: E501 DEFAULT_FUNCTION_DESCRIPTION = """Query search engine for relevant information""" diff --git a/src/resin/knowledge_base/knowledge_base.py b/src/resin/knowledge_base/knowledge_base.py index a6b68e59..91239c6a 100644 --- a/src/resin/knowledge_base/knowledge_base.py +++ b/src/resin/knowledge_base/knowledge_base.py @@ -152,7 +152,7 @@ def __init__(self, # Normally, index creation params are passed directly to the `.create_resin_index()` method. # noqa: E501 # However, when KnowledgeBase is initialized from a config file, these params # would be set by the `KnowledgeBase.from_config()` constructor. - self._index_params: Optional[Dict[str, Any]] = None + self._index_params: Dict[str, Any] = {} # The index object is initialized lazily, when the user calls `connect()` or # `create_resin_index()` @@ -306,7 +306,7 @@ def create_resin_index(self, ) # create index - index_params = index_params or self._index_params or {} + index_params = index_params or self._index_params try: create_index(name=self.index_name, dimension=dimension, diff --git a/tests/unit/utils/test_config.py b/tests/unit/utils/test_config.py index d7ebdde8..fe190f98 100644 --- a/tests/unit/utils/test_config.py +++ b/tests/unit/utils/test_config.py @@ -1,11 +1,22 @@ # noqa: F405 -import pytest +import os +import pytest +import yaml + +from resin.chat_engine import ChatEngine +from resin.context_engine import ContextEngine +from resin.knowledge_base import KnowledgeBase +# from resin.tokenizer import Tokenizer +from resin.utils.config import ConfigurableMixin +# from tests import StubTokenizer from ._stub_classes import (BaseStubChunker, StubChunker, StubKB, BaseStubContextEngine, StubOtherChunker, StubContextBuilder, StubContextEngine, BaseStubKB) +DEFAULT_COFIG_PATH = 'config/config.yaml' + def test_list_supported_classes(): supported_chunkers = BaseStubChunker.list_supported_types() assert len(supported_chunkers) == 2 @@ -328,3 +339,46 @@ def test_init_complex_missing_mandatory_dependency(): with pytest.raises(TypeError) as e: StubContextEngine() assert 'knowledge_base' in str(e.value) + + +@pytest.fixture(scope='module') +def temp_index_name(): + index_name_before = os.getenv("INDEX_NAME", None) + + os.environ["INDEX_NAME"] = "temp_index" + yield "temp_index" + + if index_name_before is None: + del os.environ["INDEX_NAME"] + else: + os.environ["INDEX_NAME"] = index_name_before + + +def test_default_config_matches_code_defaults(temp_index_name): + with open(DEFAULT_COFIG_PATH) as f: + default_config = yaml.safe_load(f) + chat_engine_config = default_config['chat_engine'] + + loaded_chat_engine = ChatEngine.from_config(chat_engine_config) + default_kb = KnowledgeBase(index_name=temp_index_name) + default_context_engine = ContextEngine(default_kb) + default_chat_engine = ChatEngine(default_context_engine) + + def assert_identical_components(loaded_component, default_component): + assert type(loaded_component) == type(default_component) + if not loaded_component.__module__.startswith("resin"): + return + + for key, value in default_component.__dict__.items(): + assert hasattr(loaded_component, key), ( + f"Missing attribute {key} in {type(loaded_component)}" + ) + if hasattr(value, '__dict__'): + assert_identical_components(getattr(loaded_component, key), value) + else: + assert getattr(loaded_component, key) == value, ( + f"Attribute {key} in {type(loaded_component)} is {value} in code " + f"but {getattr(loaded_component, key)} in config" + ) + + assert_identical_components(loaded_chat_engine, default_chat_engine) From a9d6fbad8efb601503c5c62fdbf5ffde77071afc Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 24 Oct 2023 11:58:27 +0300 Subject: [PATCH 2/8] [test] Fix pytest warning --- tests/unit/chat_engine/test_chat_engine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/chat_engine/test_chat_engine.py b/tests/unit/chat_engine/test_chat_engine.py index e14354c0..1a0e6dd5 100644 --- a/tests/unit/chat_engine/test_chat_engine.py +++ b/tests/unit/chat_engine/test_chat_engine.py @@ -19,7 +19,7 @@ class TestChatEngine: - def setup(self): + def setup_method(self): self.mock_llm = create_autospec(BaseLLM) self.mock_query_builder = create_autospec(QueryGenerator) self.mock_context_engine = create_autospec(ContextEngine) From c5d1e5b7e4db05ed33345a26585e893e90deeffd Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 24 Oct 2023 12:09:08 +0300 Subject: [PATCH 3/8] [test] remove unused imports --- tests/unit/utils/test_config.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/unit/utils/test_config.py b/tests/unit/utils/test_config.py index fe190f98..95badfee 100644 --- a/tests/unit/utils/test_config.py +++ b/tests/unit/utils/test_config.py @@ -7,9 +7,6 @@ from resin.chat_engine import ChatEngine from resin.context_engine import ContextEngine from resin.knowledge_base import KnowledgeBase -# from resin.tokenizer import Tokenizer -from resin.utils.config import ConfigurableMixin -# from tests import StubTokenizer from ._stub_classes import (BaseStubChunker, StubChunker, StubKB, BaseStubContextEngine, StubOtherChunker, StubContextBuilder, StubContextEngine, BaseStubKB) From f48cb128c29f7173019dc068bef19c7d28ffa1e6 Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 24 Oct 2023 12:12:58 +0300 Subject: [PATCH 4/8] linter --- tests/unit/utils/test_config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/utils/test_config.py b/tests/unit/utils/test_config.py index 95badfee..14915896 100644 --- a/tests/unit/utils/test_config.py +++ b/tests/unit/utils/test_config.py @@ -14,6 +14,7 @@ DEFAULT_COFIG_PATH = 'config/config.yaml' + def test_list_supported_classes(): supported_chunkers = BaseStubChunker.list_supported_types() assert len(supported_chunkers) == 2 @@ -362,7 +363,7 @@ def test_default_config_matches_code_defaults(temp_index_name): default_chat_engine = ChatEngine(default_context_engine) def assert_identical_components(loaded_component, default_component): - assert type(loaded_component) == type(default_component) + assert type(loaded_component) == type(default_component) # noqa: E721 if not loaded_component.__module__.startswith("resin"): return @@ -376,6 +377,6 @@ def assert_identical_components(loaded_component, default_component): assert getattr(loaded_component, key) == value, ( f"Attribute {key} in {type(loaded_component)} is {value} in code " f"but {getattr(loaded_component, key)} in config" - ) + ) assert_identical_components(loaded_chat_engine, default_chat_engine) From 84cd3c3dd421de7d0508b7bb4becb2a4b994242d Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 24 Oct 2023 14:29:55 +0300 Subject: [PATCH 5/8] [tests] Moved config defaults test to system tests Since this test instantiates acutual objects (namely `OpenAILLM`), it needs to have actual API keys etc. This shouldn't be part of the unit tests --- tests/system/utils/__init__.py | 0 tests/system/utils/test_config.py | 53 +++++++++++++++++++++++++++++++ tests/unit/utils/test_config.py | 50 ----------------------------- 3 files changed, 53 insertions(+), 50 deletions(-) create mode 100644 tests/system/utils/__init__.py create mode 100644 tests/system/utils/test_config.py diff --git a/tests/system/utils/__init__.py b/tests/system/utils/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/system/utils/test_config.py b/tests/system/utils/test_config.py new file mode 100644 index 00000000..84dbf1fe --- /dev/null +++ b/tests/system/utils/test_config.py @@ -0,0 +1,53 @@ +import os + +import pytest +import yaml + +from resin.chat_engine import ChatEngine +from resin.context_engine import ContextEngine +from resin.knowledge_base import KnowledgeBase + +DEFAULT_COFIG_PATH = 'config/config.yaml' + + +@pytest.fixture(scope='module') +def temp_index_name(): + index_name_before = os.getenv("INDEX_NAME", None) + + os.environ["INDEX_NAME"] = "temp_index" + yield "temp_index" + + if index_name_before is None: + del os.environ["INDEX_NAME"] + else: + os.environ["INDEX_NAME"] = index_name_before + + +def test_default_config_matches_code_defaults(temp_index_name): + with open(DEFAULT_COFIG_PATH) as f: + default_config = yaml.safe_load(f) + chat_engine_config = default_config['chat_engine'] + + loaded_chat_engine = ChatEngine.from_config(chat_engine_config) + default_kb = KnowledgeBase(index_name=temp_index_name) + default_context_engine = ContextEngine(default_kb) + default_chat_engine = ChatEngine(default_context_engine) + + def assert_identical_components(loaded_component, default_component): + assert type(loaded_component) == type(default_component) # noqa: E721 + if not loaded_component.__module__.startswith("resin"): + return + + for key, value in default_component.__dict__.items(): + assert hasattr(loaded_component, key), ( + f"Missing attribute {key} in {type(loaded_component)}" + ) + if hasattr(value, '__dict__'): + assert_identical_components(getattr(loaded_component, key), value) + else: + assert getattr(loaded_component, key) == value, ( + f"Attribute {key} in {type(loaded_component)} is {value} in code " + f"but {getattr(loaded_component, key)} in config" + ) + + assert_identical_components(loaded_chat_engine, default_chat_engine) diff --git a/tests/unit/utils/test_config.py b/tests/unit/utils/test_config.py index 14915896..b25d1b51 100644 --- a/tests/unit/utils/test_config.py +++ b/tests/unit/utils/test_config.py @@ -2,19 +2,12 @@ import os import pytest -import yaml -from resin.chat_engine import ChatEngine -from resin.context_engine import ContextEngine -from resin.knowledge_base import KnowledgeBase from ._stub_classes import (BaseStubChunker, StubChunker, StubKB, BaseStubContextEngine, StubOtherChunker, StubContextBuilder, StubContextEngine, BaseStubKB) -DEFAULT_COFIG_PATH = 'config/config.yaml' - - def test_list_supported_classes(): supported_chunkers = BaseStubChunker.list_supported_types() assert len(supported_chunkers) == 2 @@ -337,46 +330,3 @@ def test_init_complex_missing_mandatory_dependency(): with pytest.raises(TypeError) as e: StubContextEngine() assert 'knowledge_base' in str(e.value) - - -@pytest.fixture(scope='module') -def temp_index_name(): - index_name_before = os.getenv("INDEX_NAME", None) - - os.environ["INDEX_NAME"] = "temp_index" - yield "temp_index" - - if index_name_before is None: - del os.environ["INDEX_NAME"] - else: - os.environ["INDEX_NAME"] = index_name_before - - -def test_default_config_matches_code_defaults(temp_index_name): - with open(DEFAULT_COFIG_PATH) as f: - default_config = yaml.safe_load(f) - chat_engine_config = default_config['chat_engine'] - - loaded_chat_engine = ChatEngine.from_config(chat_engine_config) - default_kb = KnowledgeBase(index_name=temp_index_name) - default_context_engine = ContextEngine(default_kb) - default_chat_engine = ChatEngine(default_context_engine) - - def assert_identical_components(loaded_component, default_component): - assert type(loaded_component) == type(default_component) # noqa: E721 - if not loaded_component.__module__.startswith("resin"): - return - - for key, value in default_component.__dict__.items(): - assert hasattr(loaded_component, key), ( - f"Missing attribute {key} in {type(loaded_component)}" - ) - if hasattr(value, '__dict__'): - assert_identical_components(getattr(loaded_component, key), value) - else: - assert getattr(loaded_component, key) == value, ( - f"Attribute {key} in {type(loaded_component)} is {value} in code " - f"but {getattr(loaded_component, key)} in config" - ) - - assert_identical_components(loaded_chat_engine, default_chat_engine) From 46158c76e1cb4dd104d7a0f7d97683ba3aa5d1ec Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 24 Oct 2023 15:59:04 +0300 Subject: [PATCH 6/8] fix linter --- tests/unit/utils/test_config.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/utils/test_config.py b/tests/unit/utils/test_config.py index b25d1b51..1d8f5f53 100644 --- a/tests/unit/utils/test_config.py +++ b/tests/unit/utils/test_config.py @@ -1,5 +1,4 @@ # noqa: F405 -import os import pytest From f4fede1388e8d6889a02651167a2a385ad56a2dd Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 24 Oct 2023 17:53:16 +0300 Subject: [PATCH 7/8] [llm] Remove `model_name` check against OpenAI'S model list This involves doing an API call on __init__(), which is cumbersome. Worst case it will fail on the first LLm call --- src/resin/llm/openai.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/resin/llm/openai.py b/src/resin/llm/openai.py index af7ca672..9e43fb6e 100644 --- a/src/resin/llm/openai.py +++ b/src/resin/llm/openai.py @@ -24,12 +24,10 @@ def __init__(self, ): super().__init__(model_name, model_params=model_params) - self.available_models = [k["id"] for k in openai.Model.list().data] - if model_name not in self.available_models: - raise ValueError( - f"Model {model_name} not found. " - f" Available models: {self.available_models}" - ) + + @property + def available_models(self): + return [k["id"] for k in openai.Model.list().data] @retry( wait=wait_random_exponential(min=1, max=10), From ab4d8265b5fb5a3b9a2c089242a5f2b3745fa094 Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 24 Oct 2023 18:09:27 +0300 Subject: [PATCH 8/8] [test] Added invalid_model_name test for OpenAILLM We removed this mechanism from the constructor --- tests/system/llm/test_openai.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/system/llm/test_openai.py b/tests/system/llm/test_openai.py index 1caad24c..ef5ad677 100644 --- a/tests/system/llm/test_openai.py +++ b/tests/system/llm/test_openai.py @@ -153,11 +153,6 @@ def test_max_tokens(openai_llm, messages): assert isinstance(response, ChatResponse) assert len(response.choices[0].message.content.split()) <= max_tokens - @staticmethod - def test_invalid_model_name(): - with pytest.raises(ValueError, match="Model invalid_model_name not found."): - OpenAILLM(model_name="invalid_model_name") - @staticmethod def test_missing_messages(openai_llm): with pytest.raises(InvalidRequestError):