From 8d2de77e6940e1ebcae634c9a9597b7eaaf9a22a Mon Sep 17 00:00:00 2001 From: Siraj R Aizlewood Date: Tue, 13 Feb 2024 23:00:55 +0400 Subject: [PATCH 1/8] Bug Fix Saving JSON/YAML of Layer Config Saving Layer Configs as JSON or YAML config files by representing the LayerConfig object as a dict and then turning into JSON/YAML wasn't working. See issue here: https://github.com/aurelio-labs/semantic-router/issues/144 The issue was that, by attempting this, we were attempting to serialize all objects included in the Layer, including Routes, and the LLMs that those Routes use. In the case of the above issue, the LLM was a Cohere one, which included a Client as one of its attributes, and this Client is not serializable. So, instead of attempting to represent the entire LLM object a dict, to then be converted into JSON/YAML, we only keep key information about the LLM: 'module': self.llm.__module__, 'class': self.llm.__class__.__name__, 'model': self.llm.name This is what's saved in route.py, and then sent to layer.py to be serialized (in LayerConfig.to_file()). Then, when it comes time to load from the file via LayerConfig.from_file, the LLM is re-initialized dynamically. --- semantic_router/layer.py | 39 +++++++++++++++++++++++++-------------- semantic_router/route.py | 12 +++++++++++- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/semantic_router/layer.py b/semantic_router/layer.py index 08afccfa..35f51405 100644 --- a/semantic_router/layer.py +++ b/semantic_router/layer.py @@ -13,7 +13,7 @@ from semantic_router.route import Route from semantic_router.schema import Encoder, EncoderType, RouteChoice from semantic_router.utils.logger import logger - +import importlib def is_valid(layer_config: str) -> bool: """Make sure the given string is json format and contains the 3 keys: ["encoder_name", "encoder_type", "routes"]""" @@ -77,7 +77,6 @@ def __init__( @classmethod def from_file(cls, path: str) -> "LayerConfig": - """Load the routes from a file in JSON or YAML format""" logger.info(f"Loading route config from {path}") _, ext = os.path.splitext(path) with open(path, "r") as f: @@ -86,21 +85,33 @@ def from_file(cls, path: str) -> "LayerConfig": elif ext in [".yaml", ".yml"]: layer = yaml.safe_load(f) else: - raise ValueError( - "Unsupported file type. Only .json and .yaml are supported" - ) + raise ValueError("Unsupported file type. Only .json and .yaml are supported") - route_config_str = json.dumps(layer) - if is_valid(route_config_str): - encoder_type = layer["encoder_type"] - encoder_name = layer["encoder_name"] - routes = [Route.from_dict(route) for route in layer["routes"]] - return cls( - encoder_type=encoder_type, encoder_name=encoder_name, routes=routes - ) - else: + if not is_valid(json.dumps(layer)): raise Exception("Invalid config JSON or YAML") + encoder_type = layer["encoder_type"] + encoder_name = layer["encoder_name"] + routes = [] + for route_data in layer["routes"]: + # Handle the 'llm' field specially if it exists + if 'llm' in route_data: + llm_data = route_data.pop('llm') # Remove 'llm' from route_data and handle it separately + # Use the module path directly from llm_data without modification + llm_module_path = llm_data['module'] + # Dynamically import the module and then the class from that module + llm_module = importlib.import_module(llm_module_path) + llm_class = getattr(llm_module, llm_data['class']) + # Instantiate the LLM class with the provided model name + llm = llm_class(name=llm_data['model']) + route_data['llm'] = llm # Reassign the instantiated llm object back to route_data + + # Dynamically create the Route object using the remaining route_data + route = Route(**route_data) + routes.append(route) + + return cls(encoder_type=encoder_type, encoder_name=encoder_name, routes=routes) + def to_dict(self) -> Dict[str, Any]: return { "encoder_type": self.encoder_type, diff --git a/semantic_router/route.py b/semantic_router/route.py index 1fe9983d..1b08547a 100644 --- a/semantic_router/route.py +++ b/semantic_router/route.py @@ -68,8 +68,18 @@ def __call__(self, query: Optional[str] = None) -> RouteChoice: func_call = None return RouteChoice(name=self.name, function_call=func_call) + # def to_dict(self) -> Dict[str, Any]: + # return self.dict() + def to_dict(self) -> Dict[str, Any]: - return self.dict() + data = self.dict() + if self.llm is not None: + data['llm'] = { + 'module': self.llm.__module__, + 'class': self.llm.__class__.__name__, + 'model': self.llm.name + } + return data @classmethod def from_dict(cls, data: Dict[str, Any]): From 596006471bae9cc23bb06271d8d1a5fdab48a3d8 Mon Sep 17 00:00:00 2001 From: Siraj R Aizlewood Date: Tue, 13 Feb 2024 23:09:17 +0400 Subject: [PATCH 2/8] Linting. --- semantic_router/layer.py | 25 +++++++++++++++++-------- semantic_router/route.py | 10 +++++----- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/semantic_router/layer.py b/semantic_router/layer.py index 35f51405..153d0607 100644 --- a/semantic_router/layer.py +++ b/semantic_router/layer.py @@ -15,6 +15,7 @@ from semantic_router.utils.logger import logger import importlib + def is_valid(layer_config: str) -> bool: """Make sure the given string is json format and contains the 3 keys: ["encoder_name", "encoder_type", "routes"]""" try: @@ -85,7 +86,9 @@ def from_file(cls, path: str) -> "LayerConfig": elif ext in [".yaml", ".yml"]: layer = yaml.safe_load(f) else: - raise ValueError("Unsupported file type. Only .json and .yaml are supported") + raise ValueError( + "Unsupported file type. Only .json and .yaml are supported" + ) if not is_valid(json.dumps(layer)): raise Exception("Invalid config JSON or YAML") @@ -95,22 +98,28 @@ def from_file(cls, path: str) -> "LayerConfig": routes = [] for route_data in layer["routes"]: # Handle the 'llm' field specially if it exists - if 'llm' in route_data: - llm_data = route_data.pop('llm') # Remove 'llm' from route_data and handle it separately + if "llm" in route_data: + llm_data = route_data.pop( + "llm" + ) # Remove 'llm' from route_data and handle it separately # Use the module path directly from llm_data without modification - llm_module_path = llm_data['module'] + llm_module_path = llm_data["module"] # Dynamically import the module and then the class from that module llm_module = importlib.import_module(llm_module_path) - llm_class = getattr(llm_module, llm_data['class']) + llm_class = getattr(llm_module, llm_data["class"]) # Instantiate the LLM class with the provided model name - llm = llm_class(name=llm_data['model']) - route_data['llm'] = llm # Reassign the instantiated llm object back to route_data + llm = llm_class(name=llm_data["model"]) + route_data[ + "llm" + ] = llm # Reassign the instantiated llm object back to route_data # Dynamically create the Route object using the remaining route_data route = Route(**route_data) routes.append(route) - return cls(encoder_type=encoder_type, encoder_name=encoder_name, routes=routes) + return cls( + encoder_type=encoder_type, encoder_name=encoder_name, routes=routes + ) def to_dict(self) -> Dict[str, Any]: return { diff --git a/semantic_router/route.py b/semantic_router/route.py index 1b08547a..a1e0e9b0 100644 --- a/semantic_router/route.py +++ b/semantic_router/route.py @@ -70,14 +70,14 @@ def __call__(self, query: Optional[str] = None) -> RouteChoice: # def to_dict(self) -> Dict[str, Any]: # return self.dict() - + def to_dict(self) -> Dict[str, Any]: data = self.dict() if self.llm is not None: - data['llm'] = { - 'module': self.llm.__module__, - 'class': self.llm.__class__.__name__, - 'model': self.llm.name + data["llm"] = { + "module": self.llm.__module__, + "class": self.llm.__class__.__name__, + "model": self.llm.name, } return data From b7721f5088b0d6fddd7f4ae89b193c4cc7737e9a Mon Sep 17 00:00:00 2001 From: Siraj R Aizlewood Date: Tue, 13 Feb 2024 23:41:11 +0400 Subject: [PATCH 3/8] Fix for case when no LLM in config dict. --- semantic_router/layer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/semantic_router/layer.py b/semantic_router/layer.py index 153d0607..46abe83d 100644 --- a/semantic_router/layer.py +++ b/semantic_router/layer.py @@ -98,7 +98,7 @@ def from_file(cls, path: str) -> "LayerConfig": routes = [] for route_data in layer["routes"]: # Handle the 'llm' field specially if it exists - if "llm" in route_data: + if "llm" in route_data and route_data["llm"] is not None: llm_data = route_data.pop( "llm" ) # Remove 'llm' from route_data and handle it separately From f7f0767653f8030d15891fbc4dc1b37cb3c42b88 Mon Sep 17 00:00:00 2001 From: Siraj R Aizlewood Date: Wed, 14 Feb 2024 00:02:57 +0400 Subject: [PATCH 4/8] New PyTests And some improvements where we make sure that temp files are definitely deleted after use. --- tests/unit/test_layer.py | 88 +++++++++++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 10 deletions(-) diff --git a/tests/unit/test_layer.py b/tests/unit/test_layer.py index 9669d594..ae0a6263 100644 --- a/tests/unit/test_layer.py +++ b/tests/unit/test_layer.py @@ -269,30 +269,98 @@ def test_failover_score_threshold(self, base_encoder): assert route_layer.score_threshold == 0.5 def test_json(self, openai_encoder, routes): - with tempfile.NamedTemporaryFile(suffix=".yaml", delete=False) as temp: + temp = tempfile.NamedTemporaryFile(suffix=".yaml", delete=False) + try: + temp_path = temp.name # Save the temporary file's path + temp.close() # Close the file to ensure it can be opened again on Windows os.environ["OPENAI_API_KEY"] = "test_api_key" route_layer = RouteLayer(encoder=openai_encoder, routes=routes) - route_layer.to_json(temp.name) - assert os.path.exists(temp.name) - route_layer_from_file = RouteLayer.from_json(temp.name) + route_layer.to_json(temp_path) + assert os.path.exists(temp_path) + route_layer_from_file = RouteLayer.from_json(temp_path) assert ( route_layer_from_file.index is not None and route_layer_from_file.categories is not None ) - os.remove(temp.name) + finally: + os.remove(temp_path) # Ensure the file is deleted even if the test fails def test_yaml(self, openai_encoder, routes): - with tempfile.NamedTemporaryFile(suffix=".yaml", delete=False) as temp: + temp = tempfile.NamedTemporaryFile(suffix=".yaml", delete=False) + try: + temp_path = temp.name # Save the temporary file's path + temp.close() # Close the file to ensure it can be opened again on Windows os.environ["OPENAI_API_KEY"] = "test_api_key" route_layer = RouteLayer(encoder=openai_encoder, routes=routes) - route_layer.to_yaml(temp.name) - assert os.path.exists(temp.name) - route_layer_from_file = RouteLayer.from_yaml(temp.name) + route_layer.to_yaml(temp_path) + assert os.path.exists(temp_path) + route_layer_from_file = RouteLayer.from_yaml(temp_path) assert ( route_layer_from_file.index is not None and route_layer_from_file.categories is not None ) - os.remove(temp.name) + finally: + os.remove(temp_path) # Ensure the file is deleted even if the test fails + + def test_from_file_json(openai_encoder, tmp_path): + # Create a temporary JSON file with layer configuration + config_path = tmp_path / "config.json" + config_path.write_text( + layer_json() + ) # Assuming layer_json() returns a valid JSON string + + # Load the LayerConfig from the temporary file + layer_config = LayerConfig.from_file(str(config_path)) + + # Assertions to verify the loaded configuration + assert layer_config.encoder_type == "cohere" + assert layer_config.encoder_name == "embed-english-v3.0" + assert len(layer_config.routes) == 2 + assert layer_config.routes[0].name == "politics" + + def test_from_file_yaml(openai_encoder, tmp_path): + # Create a temporary YAML file with layer configuration + config_path = tmp_path / "config.yaml" + config_path.write_text( + layer_yaml() + ) # Assuming layer_yaml() returns a valid YAML string + + # Load the LayerConfig from the temporary file + layer_config = LayerConfig.from_file(str(config_path)) + + # Assertions to verify the loaded configuration + assert layer_config.encoder_type == "cohere" + assert layer_config.encoder_name == "embed-english-v3.0" + assert len(layer_config.routes) == 2 + assert layer_config.routes[0].name == "politics" + + # def test_from_file_with_llm(openai_encoder, tmp_path): + # # Create a temporary JSON file with LLM information in one of the routes + # config_with_llm = layer_json_with_llm() # You need to define this function to include LLM data + # config_path = tmp_path / "config_with_llm.json" + # config_path.write_text(config_with_llm) + + # # Load the LayerConfig from the temporary file + # layer_config = LayerConfig.from_file(str(config_path)) + + # # Assertions to verify LLM handling + # assert isinstance(layer_config.routes[0].llm, BaseLLM) # Or the specific LLM class you expect + + def test_from_file_invalid_path(self): + with pytest.raises(FileNotFoundError) as excinfo: + LayerConfig.from_file("nonexistent_path.json") + assert "[Errno 2] No such file or directory: 'nonexistent_path.json'" in str( + excinfo.value + ) + + def test_from_file_unsupported_type(self, tmp_path): + # Create a temporary unsupported file + config_path = tmp_path / "config.unsupported" + config_path.write_text(layer_json()) + + with pytest.raises(ValueError) as excinfo: + LayerConfig.from_file(str(config_path)) + assert "Unsupported file type" in str(excinfo.value) def test_config(self, openai_encoder, routes): os.environ["OPENAI_API_KEY"] = "test_api_key" From b21ce8ab4c9163d02bfea0483a9be9d408a1ab29 Mon Sep 17 00:00:00 2001 From: Siraj R Aizlewood Date: Wed, 14 Feb 2024 12:25:27 +0400 Subject: [PATCH 5/8] New PyTest test_from_file_with_llm() --- tests/unit/test_layer.py | 54 ++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/tests/unit/test_layer.py b/tests/unit/test_layer.py index ae0a6263..cbf10f10 100644 --- a/tests/unit/test_layer.py +++ b/tests/unit/test_layer.py @@ -1,12 +1,14 @@ import os import tempfile -from unittest.mock import mock_open, patch +from unittest.mock import mock_open, patch, MagicMock, create_autospec import pytest from semantic_router.encoders import BaseEncoder, CohereEncoder, OpenAIEncoder from semantic_router.layer import LayerConfig, RouteLayer from semantic_router.route import Route +from semantic_router.llms.base import BaseLLM +from semantic_router.schema import Message def mock_encoder_call(utterances): @@ -334,18 +336,6 @@ def test_from_file_yaml(openai_encoder, tmp_path): assert len(layer_config.routes) == 2 assert layer_config.routes[0].name == "politics" - # def test_from_file_with_llm(openai_encoder, tmp_path): - # # Create a temporary JSON file with LLM information in one of the routes - # config_with_llm = layer_json_with_llm() # You need to define this function to include LLM data - # config_path = tmp_path / "config_with_llm.json" - # config_path.write_text(config_with_llm) - - # # Load the LayerConfig from the temporary file - # layer_config = LayerConfig.from_file(str(config_path)) - - # # Assertions to verify LLM handling - # assert isinstance(layer_config.routes[0].llm, BaseLLM) # Or the specific LLM class you expect - def test_from_file_invalid_path(self): with pytest.raises(FileNotFoundError) as excinfo: LayerConfig.from_file("nonexistent_path.json") @@ -362,6 +352,44 @@ def test_from_file_unsupported_type(self, tmp_path): LayerConfig.from_file(str(config_path)) assert "Unsupported file type" in str(excinfo.value) + def test_from_file_with_llm(self, tmp_path): + llm_config_json = """ + { + "encoder_type": "cohere", + "encoder_name": "embed-english-v3.0", + "routes": [ + { + "name": "llm_route", + "utterances": ["tell me a joke", "say something funny"], + "llm": { + "module": "semantic_router.llms.base", + "class": "BaseLLM", + "model": "fake-model-v1" + } + } + ] + }""" + + # Instead of mocking, directly instantiate BaseLLM with a name + fake_llm_instance = BaseLLM(name="fake-model-v1") + + config_path = tmp_path / "config_with_llm.json" + with open(config_path, "w") as file: + file.write(llm_config_json) + + # Load the LayerConfig from the temporary file + layer_config = LayerConfig.from_file(str(config_path)) + + # Assertions to verify the behavior + # Since we're not mocking importlib.import_module, we skip the assertion + # that checks if the module was imported. Instead, we focus on the result. + assert isinstance( + layer_config.routes[0].llm, BaseLLM + ), "LLM should be instantiated and associated with the route based on the config" + assert ( + layer_config.routes[0].llm.name == "fake-model-v1" + ), "LLM instance should have the 'name' attribute set correctly" + def test_config(self, openai_encoder, routes): os.environ["OPENAI_API_KEY"] = "test_api_key" route_layer = RouteLayer(encoder=openai_encoder, routes=routes) From 4ed7bf9dcd572fd427c6689969f2d9d300d8bc9e Mon Sep 17 00:00:00 2001 From: Siraj R Aizlewood Date: Wed, 14 Feb 2024 12:28:03 +0400 Subject: [PATCH 6/8] Linting. --- tests/unit/test_layer.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/unit/test_layer.py b/tests/unit/test_layer.py index cbf10f10..ba612d64 100644 --- a/tests/unit/test_layer.py +++ b/tests/unit/test_layer.py @@ -1,6 +1,6 @@ import os import tempfile -from unittest.mock import mock_open, patch, MagicMock, create_autospec +from unittest.mock import mock_open, patch import pytest @@ -8,7 +8,6 @@ from semantic_router.layer import LayerConfig, RouteLayer from semantic_router.route import Route from semantic_router.llms.base import BaseLLM -from semantic_router.schema import Message def mock_encoder_call(utterances): @@ -370,9 +369,6 @@ def test_from_file_with_llm(self, tmp_path): ] }""" - # Instead of mocking, directly instantiate BaseLLM with a name - fake_llm_instance = BaseLLM(name="fake-model-v1") - config_path = tmp_path / "config_with_llm.json" with open(config_path, "w") as file: file.write(llm_config_json) From 3ba1fb70b8015ddac84d4f81b90cd8cfeb2dfd16 Mon Sep 17 00:00:00 2001 From: Siraj R Aizlewood Date: Wed, 14 Feb 2024 13:41:23 +0400 Subject: [PATCH 7/8] New PyTest test_from_file_invalid_config() --- tests/unit/test_layer.py | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_layer.py b/tests/unit/test_layer.py index ba612d64..d256aa21 100644 --- a/tests/unit/test_layer.py +++ b/tests/unit/test_layer.py @@ -351,6 +351,30 @@ def test_from_file_unsupported_type(self, tmp_path): LayerConfig.from_file(str(config_path)) assert "Unsupported file type" in str(excinfo.value) + def test_from_file_invalid_config(self, tmp_path): + # Define an invalid configuration JSON + invalid_config_json = """ + { + "encoder_type": "cohere", + "encoder_name": "embed-english-v3.0", + "routes": "This should be a list, not a string" + }""" + + # Write the invalid configuration to a temporary JSON file + config_path = tmp_path / "invalid_config.json" + with open(config_path, "w") as file: + file.write(invalid_config_json) + + # Patch the is_valid function to return False for this test + with patch("semantic_router.layer.is_valid", return_value=False): + # Attempt to load the LayerConfig from the temporary file + # and assert that it raises an exception due to invalid configuration + with pytest.raises(Exception) as excinfo: + LayerConfig.from_file(str(config_path)) + assert "Invalid config JSON or YAML" in str( + excinfo.value + ), "Loading an invalid configuration should raise an exception." + def test_from_file_with_llm(self, tmp_path): llm_config_json = """ { @@ -376,9 +400,7 @@ def test_from_file_with_llm(self, tmp_path): # Load the LayerConfig from the temporary file layer_config = LayerConfig.from_file(str(config_path)) - # Assertions to verify the behavior - # Since we're not mocking importlib.import_module, we skip the assertion - # that checks if the module was imported. Instead, we focus on the result. + # Using BaseLLM because trying to create a useable Mock LLM is a nightmare. assert isinstance( layer_config.routes[0].llm, BaseLLM ), "LLM should be instantiated and associated with the route based on the config" From c1a078324dd5d5742cc167b0ca55887bd2a9e26c Mon Sep 17 00:00:00 2001 From: James Briggs <35938317+jamescalam@users.noreply.github.com> Date: Wed, 14 Feb 2024 14:48:21 +0400 Subject: [PATCH 8/8] add importlib back --- semantic_router/layer.py | 1 + 1 file changed, 1 insertion(+) diff --git a/semantic_router/layer.py b/semantic_router/layer.py index a08ca18b..1e8a8130 100644 --- a/semantic_router/layer.py +++ b/semantic_router/layer.py @@ -6,6 +6,7 @@ import numpy as np import yaml from tqdm.auto import tqdm +import importlib from semantic_router.encoders import BaseEncoder, OpenAIEncoder from semantic_router.llms import BaseLLM, OpenAILLM