Litellm #1265
Litellm #1265
Conversation
2. removed stream from hyperparameters from prompt action
2. updated litellm
2. removed deprecated api
WalkthroughThe recent updates center around the enhancement of the Changes
Poem
Tip AI model upgrade
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| llm_type: str | ||
| hyperparameters: dict |
There was a problem hiding this comment.
Add description for new attributes in the class docstring.
The new attributes llm_type and hyperparameters are added to PromptActionConfigRequest, but there is no update in the class docstring explaining these new attributes.
class PromptActionConfigRequest(BaseModel):
+ """
+ Attributes:
+ llm_type: Description of llm_type
+ hyperparameters: Description of hyperparameters
+ """
llm_type: str
hyperparameters: dictCommittable suggestion was skipped due to low confidence.
| from kairon.actions.handlers.processor import ActionProcessor | ||
| from kairon.shared.actions.utils import ActionUtility | ||
| from kairon.shared.actions.exception import ActionFailure | ||
| from kairon.shared.utils import Utility | ||
| from unittest.mock import patch | ||
| from urllib.parse import urlencode |
There was a problem hiding this comment.
Correct import order to comply with PEP 8.
Move all imports to the top of the file to maintain clean and standard Python code structure.
+ from kairon.actions.handlers.processor import ActionProcessor
+ from kairon.shared.actions.utils import ActionUtility
+ from kairon.shared.actions.exception import ActionFailure
+ from unittest.mock import patch
+ from urllib.parse import urlencodeCommittable suggestion was skipped due to low confidence.
Tools
Ruff
42-42: Module level import not at top of file (E402)
43-43: Module level import not at top of file (E402)
44-44: Module level import not at top of file (E402)
45-45: Module level import not at top of file (E402)
46-46: Module level import not at top of file (E402)
| raise NotImplementedError("Provider not implemented") | ||
|
|
||
| async def perform_operation(self, op_type: Text, request_body: Dict): | ||
| async def perform_operation(self, op_type: Text, request_body: Dict, user: str, **kwargs): |
There was a problem hiding this comment.
Optimize dictionary usage and replace deprecated type annotations.
Optimize the dictionary key check and replace typing.Text with str.
- from typing import Dict, Text
+ from typing import Dict
+ from typing import str as Text
- if op_type not in supported_ops.keys():
+ if op_type not in supported_ops:Also applies to: 21-21
Tools
Ruff
18-18:
typing.Textis deprecated, usestr(UP019)Replace with
str
| from kairon.shared.utils import Utility | ||
| Utility.load_system_metadata() | ||
|
|
||
| from kairon.actions.definitions.email import ActionEmail | ||
| from kairon.actions.definitions.factory import ActionFactory |
There was a problem hiding this comment.
Reorder imports to adhere to PEP 8 standards.
The imports should be placed at the top of the file to comply with PEP 8 standards. Additionally, consider moving the Utility.load_system_metadata() call to an appropriate function or class method unless there is a specific reason for it to be executed at the module level.
+ from kairon.shared.utils import Utility
+ Utility.load_system_metadata()
import re
from unittest import mock
from googleapiclient.http import HttpRequest
from pipedrive.exceptions import UnauthorizedError, BadRequestError
from kairon.actions.definitions.email import ActionEmail
from kairon.actions.definitions.factory import ActionFactoryCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from kairon.shared.utils import Utility | |
| Utility.load_system_metadata() | |
| from kairon.actions.definitions.email import ActionEmail | |
| from kairon.actions.definitions.factory import ActionFactory | |
| from kairon.shared.utils import Utility | |
| Utility.load_system_metadata() | |
| import re | |
| from unittest import mock | |
| from googleapiclient.http import HttpRequest | |
| from pipedrive.exceptions import UnauthorizedError, BadRequestError | |
| from kairon.actions.definitions.email import ActionEmail | |
| from kairon.actions.definitions.factory import ActionFactory |
Tools
Ruff
11-11: Module level import not at top of file (E402)
12-12: Module level import not at top of file (E402)
| embedding = list(np.random.random(LLMProcessor.__embedding__)) | ||
| mock_embedding.side_effect = {'data': [{'embedding': embedding}]}, {'data': [{'embedding': embedding}]}, { | ||
| 'data': [{'embedding': embedding}]} | ||
| gpt3 = LLMProcessor(bot) | ||
| with mock.patch.dict(Utility.environment, {'llm': {"faq": "GPT3_FAQ_EMBED", 'api_key': secret}}): | ||
| aioresponses.add( | ||
| url=urljoin(Utility.environment['vector']['db'], f"/collections"), |
There was a problem hiding this comment.
Remove unnecessary f-string prefixes.
There are several f-string literals that do not contain placeholders, which are unnecessary and can be converted to regular strings.
- url=urljoin(Utility.environment['vector']['db'], f"/collections")
+ url=urljoin(Utility.environment['vector']['db'], "/collections")
- url=urljoin(Utility.environment['vector']['db'], f"/collections/{gpt3.bot}{gpt3.suffix}")
+ url=urljoin(Utility.environment['vector']['db'], "/collections/{gpt3.bot}{gpt3.suffix}")
- url=urljoin(Utility.environment['vector']['db'], f"/collections/{gpt3.bot}_user_details{gpt3.suffix}/points")
+ url=urljoin(Utility.environment['vector']['db'], "/collections/{gpt3.bot}_user_details{gpt3.suffix}/points")
- url=urljoin(Utility.environment['vector']['db'], f"/collections/{gpt3.bot}_country_details{gpt3.suffix}")
+ url=urljoin(Utility.environment['vector']['db'], "/collections/{gpt3.bot}_country_details{gpt3.suffix}")
- url=urljoin(Utility.environment['vector']['db'], f"/collections/{gpt3.bot}_country_details{gpt3.suffix}/points")
+ url=urljoin(Utility.environment['vector']['db'], "/collections/{gpt3.bot}_country_details{gpt3.suffix}/points")Also applies to: 170-170, 233-233, 238-238, 244-244
Tools
Ruff
129-129: f-string without any placeholders (F541)
Remove extraneous
fprefix
| from kairon.shared.multilingual.utils.translator import Translator | ||
| from kairon.shared.organization.processor import OrgProcessor | ||
| from kairon.shared.sso.clients.google import GoogleSSO | ||
| from kairon.shared.utils import Utility, MailUtility | ||
| from urllib.parse import urlencode | ||
| from deepdiff import DeepDiff |
There was a problem hiding this comment.
Move module-level imports to the top of the file.
The import statements should be placed at the top of the file to adhere to PEP 8 guidelines.
- from kairon.shared.multilingual.utils.translator import Translator
- from kairon.shared.organization.processor import OrgProcessor
- from kairon.shared.sso.clients.google import GoogleSSO
- from urllib.parse import urlencode
- from deepdiff import DeepDiff
+ # All imports
+ from kairon.shared.multilingual.utils.translator import Translator
+ from kairon.shared.organization.processor import OrgProcessor
+ from kairon.shared.sso.clients.google import GoogleSSO
+ from urllib.parse import urlencode
+ from deepdiff import DeepDiffCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from kairon.shared.multilingual.utils.translator import Translator | |
| from kairon.shared.organization.processor import OrgProcessor | |
| from kairon.shared.sso.clients.google import GoogleSSO | |
| from kairon.shared.utils import Utility, MailUtility | |
| from urllib.parse import urlencode | |
| from deepdiff import DeepDiff | |
| # All imports | |
| from kairon.shared.multilingual.utils.translator import Translator | |
| from kairon.shared.organization.processor import OrgProcessor | |
| from kairon.shared.sso.clients.google import GoogleSSO | |
| from urllib.parse import urlencode | |
| from deepdiff import DeepDiff |
Tools
Ruff
73-73: Module level import not at top of file (E402)
74-74: Module level import not at top of file (E402)
75-75: Module level import not at top of file (E402)
76-76: Module level import not at top of file (E402)
77-77: Module level import not at top of file (E402)
| from kairon.shared.utils import Utility | ||
|
|
||
| os.environ["system_file"] = "./tests/testing_data/system.yaml" |
There was a problem hiding this comment.
Consider using capitalized environment variable names.
The environment variable system_file should follow the convention of being all uppercase to distinguish it from regular variables:
- os.environ["system_file"] = "./tests/testing_data/system.yaml"
+ os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | |
| os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml" |
Tools
Ruff
9-9: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_file(SIM112)Replace
system_filewithSYSTEM_FILE
| @@ -5,6 +5,8 @@ | |||
| import re | |||
| from datetime import datetime | |||
| from typing import Any, List, Text, Dict | |||
| from ..utils import Utility | |||
| Utility.load_system_metadata() | |||
There was a problem hiding this comment.
Move the Utility.load_system_metadata() call to an appropriate location.
This function call should not be placed among import statements. It's best to move this to the main execution block or within an initialization function to maintain clean code structure.
- Utility.load_system_metadata()
+ # Moved to an initialization function or main execution blockCommittable suggestion was skipped due to low confidence.
kairon/shared/utils.py
Outdated
| def get_llms(): | ||
| return Utility.system_metadata["llm"].keys() | ||
|
|
||
| @staticmethod | ||
| def get_default_llm_hyperparameters(): | ||
| return Utility.get_llm_hyperparameters(DEFAULT_LLM) | ||
|
|
||
| @staticmethod | ||
| def get_llm_hyperparameters(llm_type): | ||
| hyperparameters = {} | ||
| if Utility.environment["llm"]["faq"] in {"GPT3_FAQ_EMBED"}: | ||
| for key, value in Utility.system_metadata["llm"]["gpt"].items(): | ||
| if llm_type in Utility.system_metadata["llm"].keys(): | ||
| for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items(): | ||
| hyperparameters[key] = value["default"] | ||
| return hyperparameters | ||
| raise AppException("Could not find any hyperparameters for configured LLM.") | ||
| raise AppException(f"Could not find any hyperparameters for {llm_type} LLM.") |
There was a problem hiding this comment.
Refactor the method to use dictionary comprehension.
The method get_llm_hyperparameters can be refactored to use dictionary comprehension for better readability and performance. Also, ensure that llm_type is checked directly in the dictionary to avoid KeyError.
- hyperparameters = {}
- if llm_type in Utility.system_metadata["llm"].keys():
- for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items():
- hyperparameters[key] = value["default"]
- return hyperparameters
- raise AppException(f"Could not find any hyperparameters for {llm_type} LLM.")
+ if llm_type not in Utility.system_metadata["llm"]:
+ raise AppException(f"Could not find any hyperparameters for {llm_type} LLM.")
+ return {key: value["default"] for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items()}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_llms(): | |
| return Utility.system_metadata["llm"].keys() | |
| @staticmethod | |
| def get_default_llm_hyperparameters(): | |
| return Utility.get_llm_hyperparameters(DEFAULT_LLM) | |
| @staticmethod | |
| def get_llm_hyperparameters(llm_type): | |
| hyperparameters = {} | |
| if Utility.environment["llm"]["faq"] in {"GPT3_FAQ_EMBED"}: | |
| for key, value in Utility.system_metadata["llm"]["gpt"].items(): | |
| if llm_type in Utility.system_metadata["llm"].keys(): | |
| for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items(): | |
| hyperparameters[key] = value["default"] | |
| return hyperparameters | |
| raise AppException("Could not find any hyperparameters for configured LLM.") | |
| raise AppException(f"Could not find any hyperparameters for {llm_type} LLM.") | |
| @staticmethod | |
| def get_llm_hyperparameters(llm_type): | |
| if llm_type not in Utility.system_metadata["llm"]: | |
| raise AppException(f"Could not find any hyperparameters for {llm_type} LLM.") | |
| return {key: value["default"] for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items()} |
Tools
Ruff
2064-2064: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
| @@ -2,7 +2,7 @@ | |||
| import responses | |||
| from kairon.shared.verification.email import QuickEmailVerification | |||
| from urllib.parse import urlencode | |||
| import mock | |||
| from unittest import mock | |||
There was a problem hiding this comment.
Change environment variable to uppercase.
For consistency and to adhere to best practices, use uppercase for environment variables. This change will also address the static analysis tool's recommendation.
- os.environ["system_file"] = "./tests/testing_data/system.yaml"
+ os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"Committable suggestion was skipped due to low confidence.
| @@ -73,7 +73,7 @@ def test_generate_questions(monkeypatch): | |||
|
|
|||
|
|
|||
| def test_generate_questions_empty_api_key(monkeypatch): | |||
| monkeypatch.setattr(openai.Completion, 'create', mock_create) | |||
| monkeypatch.setattr(Completions, 'create', mock_create) | |||
There was a problem hiding this comment.
Replace general exception handling in tests.
Using pytest.raises(Exception) is considered bad practice as it can mask other unexpected exceptions. Replace it with more specific exceptions to make the tests more robust and clear.
- with pytest.raises(Exception):
+ with pytest.raises(SpecificException):Please replace SpecificException with the actual exceptions that you expect to be raised.
Also applies to: 87-87, 97-97, 104-104
| @@ -14,7 +14,9 @@ | |||
| from kairon.shared.data.data_objects import LLMSettings | |||
| from kairon.shared.vector_embeddings.db.factory import VectorEmbeddingsDbFactory | |||
| from kairon.shared.vector_embeddings.db.qdrant import Qdrant | |||
|
|
|||
| import litellm | |||
There was a problem hiding this comment.
Change environment variable to uppercase and remove unused variable.
For consistency and to adhere to best practices, use uppercase for environment variables. Also, remove the unused variable to clean up the code.
- os.environ["system_file"] = "./tests/testing_data/system.yaml"
+ os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"
- secret = BotSecrets(secret_type=BotSecretType.gpt_key.value, value="key_value", bot="5f50fd0a56v098ca10d75d2g", user="user").save()Also applies to: 37-37
| @@ -60,7 +52,7 @@ async def request(self, request_method: str, http_url: str, request_body: Union[ | |||
| headers: dict = None, | |||
| return_json: bool = True, **kwargs): | |||
| max_retries = kwargs.get("max_retries", 1) | |||
| status_forcelist = kwargs.get("status_forcelist", [104, 502, 503, 504]) | |||
| status_forcelist = set(kwargs.get("status_forcelist", [104, 502, 503, 504])) | |||
There was a problem hiding this comment.
Set conversion should be direct.
The conversion of status_forcelist to a set should be done directly in the get method call to simplify the code.
- status_forcelist = set(kwargs.get("status_forcelist", [104, 502, 503, 504]))
+ status_forcelist = kwargs.get("status_forcelist", {104, 502, 503, 504})Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| status_forcelist = set(kwargs.get("status_forcelist", [104, 502, 503, 504])) | |
| status_forcelist = kwargs.get("status_forcelist", {104, 502, 503, 504}) |
| chunks.append(chunk) | ||
|
|
||
| return chunks | ||
| await self.session.close() |
There was a problem hiding this comment.
Ensure session is closed properly.
The session closure logic should handle exceptions to ensure that the session is always closed even if an error occurs.
- if not self.session.closed:
- await self.session.close()
+ try:
+ if not self.session.closed:
+ await self.session.close()
+ except Exception as e:
+ logger.error("Failed to close session: {}", str(e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await self.session.close() | |
| try: | |
| if not self.session.closed: | |
| await self.session.close() | |
| except Exception as e: | |
| logger.error("Failed to close session: {}", str(e)) |
| @@ -1,7 +1,7 @@ | |||
| import os | |||
| import re | |||
|
|
|||
| import mock | |||
| from unittest import mock | |||
There was a problem hiding this comment.
Use capitalized environment variables.
It's a best practice to use all uppercase letters for environment variables to distinguish them from regular variables.
- os.environ["system_file"] = "./tests/testing_data/system.yaml"
+ os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"Committable suggestion was skipped due to low confidence.
| @@ -16,7 +16,7 @@ | |||
| from kairon.shared.data.constant import EVENT_STATUS | |||
| from kairon.shared.data.model_processor import ModelProcessor | |||
|
|
|||
| from mock import patch | |||
| from unittest.mock import patch | |||
There was a problem hiding this comment.
Avoid redefinition of imports.
The patch import is redefined, which is unnecessary and can lead to confusion.
- from unittest.mock import patchCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from unittest.mock import patch |
Tools
Ruff
19-19: Module level import not at top of file (E402)
19-19: Redefinition of unused
patchfrom line 3 (F811)Remove definition:
patch
| try: | ||
| k_faq_action_config, bot_settings = self.retrieve_config() | ||
| user_question = k_faq_action_config.get('user_question') | ||
| user_msg = self.__get_user_msg(tracker, user_question) | ||
| llm_type = k_faq_action_config['llm_type'] |
There was a problem hiding this comment.
Remove unused variable.
The variable llm_type is declared but not used anywhere in the method.
- llm_type = k_faq_action_config['llm_type']Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| llm_type = k_faq_action_config['llm_type'] |
Tools
Ruff
69-69: Local variable
llm_typeis assigned to but never used (F841)Remove assignment to unused variable
llm_type
|
|
||
| global_safe = _safe_globals | ||
| global_safe['_getattr_'] = safer_getattr | ||
| global_safe['json'] = json | ||
|
|
||
|
|
||
| class PyScriptRunner(BaseActor): | ||
|
|
||
| def execute(self, source_code: Text, predefined_objects: Optional[Dict] = None, **kwargs): |
There was a problem hiding this comment.
Replace deprecated Text with str.
The use of Text is deprecated and should be replaced with str for type annotations.
- from typing import Text, Dict, Optional, Callable
+ from typing import str, Dict, Optional, Callable
- def execute(self, source_code: Text, predefined_objects: Optional[Dict] = None, **kwargs):
+ def execute(self, source_code: str, predefined_objects: Optional[Dict] = None, **kwargs):Also applies to: 38-38
Tools
Ruff
26-26:
typing.Textis deprecated, usestr(UP019)Replace with
str
| allow_module("datetime") | ||
| allow_module("time") |
There was a problem hiding this comment.
Security modules allowed.
Allowing specific modules (datetime, time) can be risky if not handled properly. Ensure that these modules do not expose sensitive operations or data.
- allow_module("datetime")
- allow_module("time")
+ # Review and restrict module permissions carefullyCommittable suggestion was skipped due to low confidence.
| @@ -8,7 +8,7 @@ | |||
| from kairon.shared.admin.constants import BotSecretType | |||
There was a problem hiding this comment.
Remove unused imports and replace deprecated Text.
Some imports are unused and should be removed to clean up the code. Also, replace deprecated Text with str.
- from kairon.shared.admin.constants.BotSecretType
- from kairon.shared.admin.processor.Sysadmin
- from kairon.shared.constants.GPT3ResourceTypes
- from typing import Text, Dict, List
+ from typing import str, Dict, ListAlso applies to: 9-9, 10-10, 18-18, 18-18, 18-18, 32-32
Tools
Ruff
8-8:
kairon.shared.admin.constants.BotSecretTypeimported but unused (F401)Remove unused import:
kairon.shared.admin.constants.BotSecretType
| @@ -81,6 +81,7 @@ def train_model_for_bot(bot: str): | |||
| raise AppException(e) | |||
There was a problem hiding this comment.
Improve exception handling and simplify conditional logic.
Using proper exception chaining and simplifying nested conditions can improve code clarity and error handling.
- raise AppException(e)
+ raise AppException(e) from e
- if agent_url:
- if token:
+ if agent_url and token:Also applies to: 111-112
Tools
Ruff
81-81: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
| from kairon.shared.data.training_data_generation_processor import TrainingDataGenerationProcessor | ||
| from kairon.shared.data.utils import DataUtility | ||
| from kairon.shared.importer.processor import DataImporterLogProcessor | ||
| from kairon.shared.llm.gpt3 import GPT3FAQEmbedding | ||
| from kairon.shared.metering.constants import MetricType | ||
| from kairon.shared.metering.data_object import Metering | ||
| from kairon.shared.models import StoryEventType, HttpContentType, CognitionDataType | ||
| from kairon.shared.multilingual.processor import MultilingualLogProcessor | ||
| from kairon.shared.test.data_objects import ModelTestingLogs | ||
| from kairon.shared.test.processor import ModelTestingLogProcessor | ||
| from kairon.shared.utils import Utility | ||
| from kairon.train import train_model_for_bot, start_training | ||
|
|
||
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | ||
| Utility.load_environment() | ||
| from deepdiff import DeepDiff | ||
| import litellm |
There was a problem hiding this comment.
Move module-level imports to the top of the file.
All imports should be placed at the top of the file to avoid hidden errors and to conform with Python's PEP 8 style guide.
- from kairon.shared.data.training_data_generation_processor import TrainingDataGenerationProcessor
- from kairon.shared.data.utils import DataUtility
- from kairon.shared.importer.processor import DataImporterLogProcessor
- from kairon.shared.metering.constants import MetricType
- from kairon.shared.metering.data_object import Metering
- from kairon.shared.models import StoryEventType, HttpContentType, CognitionDataType
- from kairon.shared.multilingual.processor import MultilingualLogProcessor
- from kairon.shared.test.data_objects import ModelTestingLogs
- from kairon.shared.test.processor import ModelTestingLogProcessor
- from kairon.train import train_model_for_bot, start_training
- from deepdiff import DeepDiff
- import litellm
+ # Move these imports to the top of the fileCommittable suggestion was skipped due to low confidence.
Tools
Ruff
80-80: Module level import not at top of file (E402)
81-81: Module level import not at top of file (E402)
82-82: Module level import not at top of file (E402)
83-83: Module level import not at top of file (E402)
84-84: Module level import not at top of file (E402)
85-85: Module level import not at top of file (E402)
86-86: Module level import not at top of file (E402)
87-87: Module level import not at top of file (E402)
88-88: Module level import not at top of file (E402)
89-89: Module level import not at top of file (E402)
90-90: Module level import not at top of file (E402)
91-91: Module level import not at top of file (E402)
2. removed deprecated api
2. removed stream from hyperparameters from prompt action
2. updated litellm
2. removed deprecated api
2. changed rasa domain.yml schemas to allow unicode Alphabets for slots and form name
# Conflicts: # kairon/shared/utils.py # tests/integration_test/action_service_test.py
There was a problem hiding this comment.
Actionable comments posted: 25
Outside diff range and nitpick comments (8)
kairon/shared/data/data_objects.py (1)
790-792: Class Definition: Ensure proper docstring for the class.Adding a docstring to the class
DemoRequestLogswill improve readability and maintainability.class DemoRequestLogs(Document): """ This class represents the logs for demo requests. """kairon/api/models.py (1)
1061-1062: Add description for new attributes in the class docstring.The new attributes
llm_typeandhyperparametersare added toPromptActionConfigRequest, but there is no update in the class docstring explaining these new attributes.class PromptActionConfigRequest(BaseModel): + """ + Attributes: + llm_type: Description of llm_type + hyperparameters: Description of hyperparameters + """ llm_type: str hyperparameters: dicttests/unit_test/utility_test.py (6)
Line range hint
852-869: Use context handler for opening files.To ensure the file is properly closed after its usage, use a context handler.
- Utility.email_conf["email"]["templates"]["add_member_invitation"] = ( - open("template/emails/memberAddAccept.html", "rb").read().decode() - ) + with open("template/emails/memberAddAccept.html", "rb") as file: + Utility.email_conf["email"]["templates"]["add_member_invitation"] = file.read().decode()Tools
Ruff
855-855: Use context handler for opening files
(SIM115)
Line range hint
893-917: Use context handler for opening files.To ensure the file is properly closed after its usage, use a context handler.
- Utility.email_conf["email"]["templates"]["add_member_confirmation"] = ( - open("template/emails/memberAddConfirmation.html", "rb").read().decode() - ) + with open("template/emails/memberAddConfirmation.html", "rb") as file: + Utility.email_conf["email"]["templates"]["add_member_confirmation"] = file.read().decode()
Line range hint
943-964: Use context handler for opening files.To ensure the file is properly closed after its usage, use a context handler.
- Utility.email_conf["email"]["templates"]["update_role"] = ( - open("template/emails/memberUpdateRole.html", "rb").read().decode() - ) + with open("template/emails/memberUpdateRole.html", "rb") as file: + Utility.email_conf["email"]["templates"]["update_role"] = file.read().decode()
Line range hint
979-1011: Use context handler for opening files.To ensure the file is properly closed after its usage, use a context handler.
- Utility.email_conf["email"]["templates"]["update_role"] = ( - open("template/emails/memberUpdateRole.html", "rb").read().decode() - ) + with open("template/emails/memberUpdateRole.html", "rb") as file: + Utility.email_conf["email"]["templates"]["update_role"] = file.read().decode()
Line range hint
1133-1136: Use context handler for opening files.To ensure the file is properly closed after its usage, use a context handler.
- Utility.email_conf["email"]["templates"]["password_generated"] = ( - open("template/emails/passwordGenerated.html", "rb").read().decode() - ) + with open("template/emails/passwordGenerated.html", "rb") as file: + Utility.email_conf["email"]["templates"]["password_generated"] = file.read().decode()Tools
Ruff
15-15:
numpyimported but unusedRemove unused import:
numpy(F401)
Line range hint
1168-1171: Use context handler for opening files.To ensure the file is properly closed after its usage, use a context handler.
- Utility.email_conf["email"]["templates"]["untrusted_login"] = ( - open("template/emails/untrustedLogin.html", "rb").read().decode() - ) + with open("template/emails/untrustedLogin.html", "rb") as file: + Utility.email_conf["email"]["templates"]["untrusted_login"] = file.read().decode()Tools
Ruff
15-15:
numpyimported but unusedRemove unused import:
numpy(F401)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- kairon/actions/definitions/email.py (3 hunks)
- kairon/api/models.py (5 hunks)
- kairon/shared/actions/data_objects.py (7 hunks)
- kairon/shared/data/constant.py (2 hunks)
- kairon/shared/data/data_objects.py (2 hunks)
- kairon/shared/data/processor.py (7 hunks)
- kairon/shared/utils.py (8 hunks)
- tests/integration_test/services_test.py (70 hunks)
- tests/testing_data/actions/actions.yml (2 hunks)
- tests/unit_test/action/action_test.py (8 hunks)
- tests/unit_test/data_processor/data_processor_test.py (46 hunks)
- tests/unit_test/multilingual/multilingual_processor_test.py (1 hunks)
- tests/unit_test/utility_test.py (28 hunks)
Files not summarized due to errors (3)
- tests/integration_test/services_test.py: Error: Message exceeds token limit
- tests/unit_test/data_processor/data_processor_test.py: Error: Message exceeds token limit
- tests/unit_test/utility_test.py: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (1)
- kairon/shared/data/constant.py
Additional context used
Ruff
kairon/shared/data/data_objects.py
42-42:
kairon.shared.models.CognitionMetadataTypeimported but unusedRemove unused import
(F401)
kairon/shared/utils.py
2064-2064: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
2079-2079: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/unit_test/utility_test.py
15-15:
numpyimported but unusedRemove unused import:
numpy(F401)
38-38:
kairon.shared.data.constant.DEFAULT_SYSTEM_PROMPTimported but unusedRemove unused import:
kairon.shared.data.constant.DEFAULT_SYSTEM_PROMPT(F401)
39-39:
kairon.shared.data.data_objects.LLMSettingsimported but unusedRemove unused import:
kairon.shared.data.data_objects.LLMSettings(F401)
855-855: Use context handler for opening files
(SIM115)
1204-1204: Use context handler for opening files
(SIM115)
2981-2981: Yoda condition detected
Rewrite as
ip == "58.0.127.89"(SIM300)
3236-3236: Use context handler for opening files
(SIM115)
tests/unit_test/action/action_test.py
41-41:
kairon.shared.actions.data_objects.SetSlotsFromResponseimported but unusedRemove unused import:
kairon.shared.actions.data_objects.SetSlotsFromResponse(F401)
2587-2588: Yoda condition detected
(SIM300)
2589-2589: Yoda condition detected
Rewrite as
actual['from_email']['value'] == '[email protected]'(SIM300)
2590-2590: Yoda condition detected
Rewrite as
actual['from_email']['parameter_type'] == 'value'(SIM300)
2617-2617: Yoda condition detected
Rewrite as
actual['from_email']['value'] == '[email protected]'(SIM300)
2618-2618: Yoda condition detected
Rewrite as
actual['from_email']['parameter_type'] == 'value'(SIM300)
kairon/shared/data/processor.py
5477-5477: Local variable
logsis assigned to but never usedRemove assignment to unused variable
logs(F841)
5490-5490:
typing.Textis deprecated, usestrReplace with
str(UP019)
5490-5490:
typing.Textis deprecated, usestrReplace with
str(UP019)
tests/unit_test/data_processor/data_processor_test.py
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
2632-2632: Unnecessary
strcall (rewrite as a literal)Replace with string literal
(UP018)
6528-6528: Avoid equality comparisons to
False; useif not actual_config.config['live_agent_enabled']:for false checksReplace with
not actual_config.config['live_agent_enabled'](E712)
Gitleaks
tests/unit_test/utility_test.py
2231-2231: Identified a Slack Bot token, which may compromise bot integrations and communication channel security.
(slack-bot-token)
Additional comments not posted (71)
kairon/actions/definitions/email.py (2)
61-61: Verify the correctness of__get_from_email_and_to_emailmethod.Ensure that the
__get_from_email_and_to_emailmethod correctly retrieves the email addresses based on the new structure.
104-120: Ensure email retrieval and validation logic is correct.The function retrieves and validates the
from_emailandto_emailbased on the new structure. Ensure that the logic correctly handles the differentparameter_typevalues and validates the email formats.tests/testing_data/actions/actions.yml (2)
182-185: Verify the newfrom_emailstructure.Ensure that the new
from_emailstructure withkey,value, andparameter_typeis correctly defined and used.
188-192: Verify the newto_emailstructure.Ensure that the new
to_emailstructure withkey,value, andparameter_typeis correctly defined and used.tests/unit_test/multilingual/multilingual_processor_test.py (2)
269-271: Verify the newfrom_emailstructure in the test setup.Ensure that the new
from_emailstructure withvalueandparameter_typeis correctly defined and used in the test setup.
271-271: Verify the newto_emailstructure in the test setup.Ensure that the new
to_emailstructure withvalueandparameter_typeis correctly defined and used in the test setup.kairon/shared/actions/data_objects.py (2)
413-415: Verify the newfrom_emailstructure inEmailActionConfig.Ensure that the new
from_emailstructure withEmbeddedDocumentField(CustomActionRequestParameters)is correctly defined and used in theEmailActionConfigclass.
415-415: Verify the newto_emailstructure inEmailActionConfig.Ensure that the new
to_emailstructure withEmbeddedDocumentField(CustomActionParameters)is correctly defined and used in theEmailActionConfigclass.kairon/api/models.py (5)
910-915: Ensure all allowed values are included in the validator.The
validate_parameter_typemethod currently allows onlyActionParameterType.valueandActionParameterType.slot. Verify if otherActionParameterTypevalues should be included.
917-925: Verify email validation logic.The
checkroot validator ensures that thevalueattribute is a list of emails whenparameter_typeisActionParameterType.value. Ensure that email validation is done properly.
1083-1087: Verify completeness of LLM types.The
validate_llm_typemethod checks ifllm_typeis valid usingUtility.get_llms(). Ensure thatUtility.get_llms()provides all valid LLM types.
1089-1092: Verify correctness of hyperparameter validation.The
validate_llm_hyperparametersmethod usesUtility.validate_llm_hyperparametersto validate the hyperparameters based onllm_type. Ensure thatUtility.validate_llm_hyperparameterscorrectly validates hyperparameters for all LLM types.
1098-1104: Verify completeness of default hyperparameters.The
checkroot validator ensures thathyperparametersare set correctly based onllm_type. Ensure thatUtility.get_llm_hyperparametersprovides all necessary default hyperparameters.kairon/shared/utils.py (2)
2058-2059: LGTM! Ensure DEFAULT_LLM is correctly defined.The code changes are approved.
However, ensure that
DEFAULT_LLMis correctly defined and used.
Line range hint
2550-2555: LGTM!The code changes are approved.
tests/unit_test/utility_test.py (1)
1188-1191: Use context handler for opening files.To ensure the file is properly closed after its usage, use a context handler.
- Utility.email_conf["email"]["templates"]["add_trusted_device"] = ( - open("template/emails/untrustedLogin.html", "rb").read().decode() - ) + with open("template/emails/untrustedLogin.html", "rb") as file: + Utility.email_conf["email"]["templates"]["add_trusted_device"] = file.read().decode()Likely invalid or redundant comment.
tests/unit_test/action/action_test.py (2)
8-9: Reorder imports to adhere to PEP 8 standards.The imports should be placed at the top of the file to comply with PEP 8 standards. Additionally, consider moving the
Utility.load_system_metadata()call to an appropriate function or class method unless there is a specific reason for it to be executed at the module level.+ from kairon.shared.utils import Utility + Utility.load_system_metadata() import re from unittest import mock from googleapiclient.http import HttpRequest from pipedrive.exceptions import UnauthorizedError, BadRequestError from kairon.actions.definitions.email import ActionEmail from kairon.actions.definitions.factory import ActionFactory
41-46: Correct import order to comply with PEP 8.Move all imports to the top of the file to maintain a clean and standard Python code structure.
+ from kairon.actions.handlers.processor import ActionProcessor + from kairon.shared.actions.utils import ActionUtility + from kairon.shared.actions.exception import ActionFailure + from unittest.mock import patch + from urllib.parse import urlencodeTools
Ruff
41-41:
kairon.shared.actions.data_objects.SetSlotsFromResponseimported but unusedRemove unused import:
kairon.shared.actions.data_objects.SetSlotsFromResponse(F401)
kairon/shared/data/processor.py (1)
6542-6544: Verify the structure offrom_emailandto_email.Ensure that the new structure for
from_emailandto_emailis correctly implemented and validated.# Verify the structure of `from_email` and `to_email` in the database and ensure it matches the expected format.tests/integration_test/services_test.py (25)
1-2: LGTM!The import statements are correctly added.
11-15: LGTM!The import statements are correctly added.
52-52: LGTM!The import statements are correctly added.
64-64: LGTM!The import statements are correctly added.
76-76: LGTM!The import statements are correctly added.
259-293: LGTM!The test function
test_book_a_demo_with_valid_datais well-structured and includes comprehensive assertions.
Line range hint
6421-6425: LGTM!The test function
test_enable_live_agentis well-structured and includes necessary assertions.
Line range hint
6426-6430: LGTM!The test function
test_get_live_agent_after_enabled_no_bot_settings_enabledis well-structured and includes necessary assertions.
Line range hint
1654-1658: LGTM!The test function
test_get_live_agent_after_enabledis well-structured and includes necessary assertions.
2391-2393: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3130-3131: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3189-3190: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3235-3236: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3294-3295: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3352-3354: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3419-3421: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3478-3480: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3537-3539: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3600-3602: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3662-3663: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3680-3742: LGTM!The test function
test_add_prompt_action_with_invalid_llm_typeis well-structured and includes comprehensive assertions.
3858-3860: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3921-3923: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
3974-3976: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.
4028-4030: LGTM!The additions of
llm_typeandhyperparametersfields to theactiondictionary are correct.tests/unit_test/data_processor/data_processor_test.py (27)
172-176: LGTM!The changes in this hunk look good to me.
200-204: LGTM!The changes in this hunk look good to me.
229-233: LGTM!The changes in this hunk look good to me.
259-263: LGTM!The changes in this hunk look good to me.
307-317: LGTM!The changes in this hunk look good to me.
326-328: LGTM!The changes in this hunk look good to me.
352-362: LGTM!The changes in this hunk look good to me.
371-373: LGTM!The changes in this hunk look good to me.
556-561: LGTM!The changes in this hunk look good to me.
581-595: LGTM!The changes in this hunk look good to me.
604-611: LGTM!The changes in this hunk look good to me.
621-630: LGTM!The changes in this hunk look good to me.
641-648: LGTM!The changes in this hunk look good to me.
659-666: LGTM!The changes in this hunk look good to me.
676-683: LGTM!The changes in this hunk look good to me.
693-700: LGTM!The changes in this hunk look good to me.
710-717: LGTM!The changes in this hunk look good to me.
727-734: LGTM!The changes in this hunk look good to me.
744-751: LGTM!The changes in this hunk look good to me.
761-768: LGTM!The changes in this hunk look good to me.
827-837: LGTM!The changes in this hunk look good to me.
Line range hint
858-863: LGTM!The changes in this hunk look good to me.
868-879: LGTM!The changes in this hunk look good to me.
912-918: LGTM!The changes in this hunk look good to me.
934-934: LGTM!The changes in this hunk look good to me.
947-953: LGTM!The changes in this hunk look good to me.
Line range hint
969-973: LGTM!The changes in this hunk look good to me.
kairon/shared/data/data_objects.py
Outdated
| first_name = StringField(required=True) | ||
| last_name = StringField(required=True) | ||
| email = StringField(required=True) | ||
| phone = StringField(default=None) | ||
| message = StringField(default=None) | ||
| recaptcha_response = StringField(default=None) | ||
| status = StringField(default=DEMO_REQUEST_STATUS.REQUEST_RECEIVED.value, | ||
| choices=[type.value for type in DEMO_REQUEST_STATUS]) |
There was a problem hiding this comment.
Field Definitions: Ensure proper validation for email and status.
The email field should be validated for proper email format, and the status field should ensure it only contains valid choices.
email = StringField(required=True, validation=email)
status = StringField(default=DEMO_REQUEST_STATUS.REQUEST_RECEIVED.value,
choices=[type.value for type in DEMO_REQUEST_STATUS], required=True)Committable suggestion was skipped due to low confidence.
kairon/shared/data/data_objects.py
Outdated
| def validate(self, clean=True): | ||
| from validators import email | ||
|
|
||
| if clean: | ||
| self.clean() | ||
|
|
||
| if Utility.check_empty_string(self.first_name): | ||
| raise ValidationError("first_name cannot be empty") | ||
| if Utility.check_empty_string(self.last_name): | ||
| raise ValidationError("last_name cannot be empty") | ||
| if isinstance(email(self.email), ValidationFailure): | ||
| raise ValidationError("Invalid email address") | ||
| if self.status not in [type.value for type in DEMO_REQUEST_STATUS]: | ||
| raise ValidationError("Invalid demo request status") | ||
|
|
There was a problem hiding this comment.
Validation Logic: Improve validation checks and error messages.
Enhance the validation logic to provide more specific error messages and ensure all fields are properly validated.
def validate(self, clean=True):
from validators import email
if clean:
self.clean()
if Utility.check_empty_string(self.first_name):
raise ValidationError("First name cannot be empty or blank spaces")
if Utility.check_empty_string(self.last_name):
raise ValidationError("Last name cannot be empty or blank spaces")
if isinstance(email(self.email), ValidationFailure):
raise ValidationError("Invalid email address")
if self.status not in [type.value for type in DEMO_REQUEST_STATUS]:
raise ValidationError(f"Invalid demo request status: {self.status}")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def validate(self, clean=True): | |
| from validators import email | |
| if clean: | |
| self.clean() | |
| if Utility.check_empty_string(self.first_name): | |
| raise ValidationError("first_name cannot be empty") | |
| if Utility.check_empty_string(self.last_name): | |
| raise ValidationError("last_name cannot be empty") | |
| if isinstance(email(self.email), ValidationFailure): | |
| raise ValidationError("Invalid email address") | |
| if self.status not in [type.value for type in DEMO_REQUEST_STATUS]: | |
| raise ValidationError("Invalid demo request status") | |
| def validate(self, clean=True): | |
| from validators import email | |
| if clean: | |
| self.clean() | |
| if Utility.check_empty_string(self.first_name): | |
| raise ValidationError("First name cannot be empty or blank spaces") | |
| if Utility.check_empty_string(self.last_name): | |
| raise ValidationError("Last name cannot be empty or blank spaces") | |
| if isinstance(email(self.email), ValidationFailure): | |
| raise ValidationError("Invalid email address") | |
| if self.status not in [type.value for type in DEMO_REQUEST_STATUS]: | |
| raise ValidationError(f"Invalid demo request status: {self.status}") |
| def get_llms(): | ||
| return Utility.system_metadata.get("llm", {}).keys() |
There was a problem hiding this comment.
Optimize dictionary access.
Consider using direct dictionary membership testing instead of calling .keys() for better performance.
- return Utility.system_metadata.get("llm", {}).keys()
+ return Utility.system_metadata.get("llm", {}).keys()Committable suggestion was skipped due to low confidence.
| @@ -37,14 +38,15 @@ | |||
| from kairon.shared.actions.data_objects import HttpActionRequestBody, HttpActionConfig, ActionServerLogs, SlotSetAction, \ | |||
| Actions, FormValidationAction, EmailActionConfig, GoogleSearchAction, JiraAction, ZendeskAction, \ | |||
| PipedriveLeadsAction, SetSlots, HubspotFormsAction, HttpActionResponse, CustomActionRequestParameters, \ | |||
| KaironTwoStageFallbackAction, SetSlotsFromResponse, PromptAction, PyscriptActionConfig, WebSearchAction | |||
| KaironTwoStageFallbackAction, SetSlotsFromResponse, PromptAction, PyscriptActionConfig, WebSearchAction, \ | |||
There was a problem hiding this comment.
Remove unused import.
The import kairon.shared.actions.data_objects.SetSlotsFromResponse is not used in the file and should be removed.
- from kairon.shared.actions.data_objects import SetSlotsFromResponseCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| KaironTwoStageFallbackAction, SetSlotsFromResponse, PromptAction, PyscriptActionConfig, WebSearchAction, \ |
Tools
Ruff
41-41:
kairon.shared.actions.data_objects.SetSlotsFromResponseimported but unusedRemove unused import:
kairon.shared.actions.data_objects.SetSlotsFromResponse(F401)
| assert '[email protected]' == actual['from_email'] | ||
| assert expected['to_email'] == actual['to_email'] | ||
| assert '[email protected]' == actual['from_email']['value'] | ||
| assert 'value' == actual['from_email']['parameter_type'] |
There was a problem hiding this comment.
Avoid Yoda conditions.
Rewrite the condition to follow standard comparison order.
- assert 'value' == actual['from_email']['parameter_type']
+ assert actual['from_email']['parameter_type'] == 'value'Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert 'value' == actual['from_email']['parameter_type'] | |
| assert actual['from_email']['parameter_type'] == 'value' |
Tools
Ruff
2618-2618: Yoda condition detected
Rewrite as
actual['from_email']['parameter_type'] == 'value'(SIM300)
|
|
||
| @responses.activate | ||
| def test_publish_auditlog(self): | ||
| bot = "secret" | ||
| bot = "publish_auditlog" | ||
| user = "secret_user" | ||
| config = { | ||
| "bot_user_oAuth_token": "xoxb-801939352912-801478018484-v3zq6MYNu62oSs8vammWOY8K", |
There was a problem hiding this comment.
Remove sensitive data.
Identified a Slack Bot token, which may compromise bot integrations and communication channel security.
- "bot_user_oAuth_token": "xoxb-801939352912-801478018484-v3zq6MYNu62oSs8vammWOY8K",
+ "bot_user_oAuth_token": "<REDACTED>",Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "bot_user_oAuth_token": "xoxb-801939352912-801478018484-v3zq6MYNu62oSs8vammWOY8K", | |
| "bot_user_oAuth_token": "<REDACTED>", |
Tools
Gitleaks
2231-2231: Identified a Slack Bot token, which may compromise bot integrations and communication channel security.
(slack-bot-token)
tests/unit_test/utility_test.py
Outdated
| "email": "[email protected]", | ||
| "phone": "+919876543210", | ||
| "message": "Thank You", | ||
| "recaptcha_response": "Svw2mPVxM0SkO4_2yxTcDQQ7iKNUDeDhGf4l6C2i" | ||
| } | ||
| Utility.email_conf["email"]["templates"]["custom_text_mail"] = ( | ||
| open("template/emails/custom_text_mail.html", "rb").read().decode() |
There was a problem hiding this comment.
Use context handler for opening files.
To ensure the file is properly closed after its usage, use a context handler.
- Utility.email_conf["email"]["templates"]["custom_text_mail"] = (
- open("template/emails/custom_text_mail.html", "rb").read().decode()
- )
+ with open("template/emails/custom_text_mail.html", "rb") as file:
+ Utility.email_conf["email"]["templates"]["custom_text_mail"] = file.read().decode()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| open("template/emails/custom_text_mail.html", "rb").read().decode() | |
| with open("template/emails/custom_text_mail.html", "rb") as file: | |
| Utility.email_conf["email"]["templates"]["custom_text_mail"] = file.read().decode() |
Tools
Ruff
1204-1204: Use context handler for opening files
(SIM115)
| from kairon.shared.utils import Utility, MailUtility | ||
|
|
||
| Utility.load_system_metadata() |
There was a problem hiding this comment.
Move module-level imports to the top of the file.
The function call Utility.load_system_metadata() should be moved below all imports to adhere to PEP 8 guidelines.
- Utility.load_system_metadata()
+ # All imports
+ from kairon.api.app.main import app
+ from kairon.events.definitions.multilingual import MultilingualEvent
+ # Function call after imports
+ Utility.load_system_metadata()Committable suggestion was skipped due to low confidence.
| assert model_training.first().exception in str("Training data does not exists!") | ||
|
|
||
| @patch.object(GPT3FAQEmbedding, "_GPT3FAQEmbedding__get_embedding", autospec=True) | ||
| @patch.object(litellm, "aembedding", autospec=True) |
There was a problem hiding this comment.
Replace unnecessary str call with a string literal.
In Python, it's more idiomatic to use a string literal directly rather than wrapping it in a str call.
- assert model_training.first().exception in str("Training data does not exists!")
+ assert model_training.first().exception in "Training data does not exists!"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert model_training.first().exception in str("Training data does not exists!") | |
| @patch.object(GPT3FAQEmbedding, "_GPT3FAQEmbedding__get_embedding", autospec=True) | |
| @patch.object(litellm, "aembedding", autospec=True) | |
| assert model_training.first().exception in "Training data does not exists!" | |
| @patch.object(litellm, "aembedding", autospec=True) |
Tools
Ruff
2632-2632: Unnecessary
strcall (rewrite as a literal)Replace with string literal
(UP018)
| from unittest.mock import patch | ||
| import numpy as np | ||
| import pandas as pd | ||
| import pytest |
There was a problem hiding this comment.
Move module-level imports to the top of the file.
Python PEP 8 style guide recommends that all module-level imports be at the top of the file to enhance readability and prevent hidden errors.
- from unittest.mock import patch
- import numpy as np
- import pandas as pd
- import pytest
+ # Move these imports to the top of the fileCommittable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- kairon/shared/data/processor.py (1 hunks)
- kairon/shared/utils.py (2 hunks)
- tests/integration_test/services_test.py (52 hunks)
- tests/unit_test/data_processor/data_processor_test.py (35 hunks)
Files skipped from review as they are similar to previous changes (2)
- kairon/shared/data/processor.py
- tests/integration_test/services_test.py
Additional context used
Ruff
kairon/shared/utils.py
2066-2066: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
2081-2081: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/unit_test/data_processor/data_processor_test.py
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
2632-2632: Unnecessary
strcall (rewrite as a literal)Replace with string literal
(UP018)
Additional comments not posted (25)
kairon/shared/utils.py (2)
2060-2061: LGTM!The function correctly retrieves default LLM hyperparameters.
2056-2057: Optimize dictionary access.Consider using direct dictionary membership testing instead of calling
.keys()for better performance.- return Utility.system_metadata.get("llm", {}).keys() + return Utility.system_metadata.get("llm", {}).keys()Likely invalid or redundant comment.
tests/unit_test/data_processor/data_processor_test.py (23)
Line range hint
1005-1005: LGTM!The test case for adding a prompt action with GPT feature disabled looks good.
Tools
Ruff
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
172-174: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_slotsare consistent and valid.
200-202: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_http_actionare consistent and valid.
229-231: LGTM!The hyperparameters in the request dictionary for
test_prompt_action_similarityare consistent and valid.
259-261: LGTM!The hyperparameters in the request dictionary for
test_prompt_action_invalid_top_resultsare consistent and valid.
307-315: LGTM!The assertion and the hyperparameters in the request dictionary for
test_add_prompt_action_with_empty_collection_for_bot_content_promptare consistent and valid.
326-326: LGTM!The assertion and the hyperparameters in the request dictionary for
test_add_prompt_action_with_bot_content_promptare consistent and valid.
352-360: LGTM!The assertion and the hyperparameters in the request dictionary for
test_add_prompt_action_with_bot_content_promptare consistent and valid.
371-371: LGTM!The assertion and the hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_query_promptare consistent and valid.
556-558: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_empty_llm_promptsare consistent and valid.
581-587: LGTM!The assertion and the hyperparameters in the request dictionary for
test_add_prompt_action_faq_action_with_default_valuesare consistent and valid.
604-606: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_temperature_hyperparameterare consistent and valid.
621-623: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_stop_hyperparameterare consistent and valid.
641-643: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_presence_penalty_hyperparameterare consistent and valid.
659-661: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_frequency_penalty_hyperparameterare consistent and valid.
676-678: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_max_tokens_hyperparameterare consistent and valid.
693-695: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_zero_max_tokens_hyperparameterare consistent and valid.
710-712: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_top_p_hyperparameterare consistent and valid.
727-729: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_n_hyperparameterare consistent and valid.
744-746: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_zero_n_hyperparameterare consistent and valid.
761-763: LGTM!The hyperparameters in the request dictionary for
test_add_prompt_action_with_invalid_logit_bias_hyperparameterare consistent and valid.
827-835: LGTM!The assertion and the hyperparameters in the request dictionary for
test_edit_prompt_action_faq_actionare consistent and valid.
858-859: LGTM!The assertion and the hyperparameters in the request dictionary for
test_edit_prompt_action_faq_action_againare consistent and valid.
| def get_llm_hyperparameters(llm_type): | ||
| hyperparameters = {} | ||
| if Utility.environment["llm"]["faq"] in {"GPT3_FAQ_EMBED"}: | ||
| for key, value in Utility.system_metadata["llm"]["gpt"].items(): | ||
| if llm_type in Utility.system_metadata["llm"].keys(): | ||
| for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items(): | ||
| hyperparameters[key] = value["default"] | ||
| return hyperparameters | ||
| raise AppException("Could not find any hyperparameters for configured LLM.") | ||
| raise AppException(f"Could not find any hyperparameters for {llm_type} LLM.") |
There was a problem hiding this comment.
Optimize dictionary access and improve exception message.
Consider using direct dictionary membership testing instead of calling .keys(). Additionally, improve the exception message for clarity.
- if llm_type in Utility.system_metadata["llm"].keys():
+ if llm_type in Utility.system_metadata["llm"]:
for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items():
hyperparameters[key] = value["default"]
return hyperparameters
raise AppException(f"Could not find any hyperparameters for LLM type: {llm_type}.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_llm_hyperparameters(llm_type): | |
| hyperparameters = {} | |
| if Utility.environment["llm"]["faq"] in {"GPT3_FAQ_EMBED"}: | |
| for key, value in Utility.system_metadata["llm"]["gpt"].items(): | |
| if llm_type in Utility.system_metadata["llm"].keys(): | |
| for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items(): | |
| hyperparameters[key] = value["default"] | |
| return hyperparameters | |
| raise AppException("Could not find any hyperparameters for configured LLM.") | |
| raise AppException(f"Could not find any hyperparameters for {llm_type} LLM.") | |
| def get_llm_hyperparameters(llm_type): | |
| hyperparameters = {} | |
| if llm_type in Utility.system_metadata["llm"]: | |
| for key, value in Utility.system_metadata["llm"][llm_type]['properties'].items(): | |
| hyperparameters[key] = value["default"] | |
| return hyperparameters | |
| raise AppException(f"Could not find any hyperparameters for LLM type: {llm_type}.") |
Tools
Ruff
2066-2066: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
| def validate_llm_hyperparameters(hyperparameters: dict, llm_type: str, exception_class): | ||
| from jsonschema_rs import JSONSchema, ValidationError as JValidationError | ||
| schema = Utility.system_metadata["llm"][llm_type] | ||
| try: | ||
| validator = JSONSchema(schema) | ||
| validator.validate(hyperparameters) | ||
| except JValidationError as e: | ||
| message = f"{e.instance_path}: {e.message}" | ||
| raise exception_class(message) |
There was a problem hiding this comment.
Improve exception handling for clarity.
Use the raise ... from ... pattern to provide more context in the traceback when re-raising exceptions.
- raise exception_class(message)
+ raise exception_class(message) from eCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def validate_llm_hyperparameters(hyperparameters: dict, llm_type: str, exception_class): | |
| from jsonschema_rs import JSONSchema, ValidationError as JValidationError | |
| schema = Utility.system_metadata["llm"][llm_type] | |
| try: | |
| validator = JSONSchema(schema) | |
| validator.validate(hyperparameters) | |
| except JValidationError as e: | |
| message = f"{e.instance_path}: {e.message}" | |
| raise exception_class(message) | |
| def validate_llm_hyperparameters(hyperparameters: dict, llm_type: str, exception_class): | |
| from jsonschema_rs import JSONSchema, ValidationError as JValidationError | |
| schema = Utility.system_metadata["llm"][llm_type] | |
| try: | |
| validator = JSONSchema(schema) | |
| validator.validate(hyperparameters) | |
| except JValidationError as e: | |
| message = f"{e.instance_path}: {e.message}" | |
| raise exception_class(message) from e |
Tools
Ruff
2081-2081: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
| @@ -55,7 +60,7 @@ | |||
| from kairon.shared.data.constant import UTTERANCE_TYPE, EVENT_STATUS, STORY_EVENT, ALLOWED_DOMAIN_FORMATS, \ | |||
| ALLOWED_CONFIG_FORMATS, ALLOWED_NLU_FORMATS, ALLOWED_STORIES_FORMATS, ALLOWED_RULES_FORMATS, REQUIREMENTS, \ | |||
| DEFAULT_NLU_FALLBACK_RULE, SLOT_TYPE, KAIRON_TWO_STAGE_FALLBACK, AuditlogActions, TOKEN_TYPE, GPT_LLM_FAQ, \ | |||
| DEFAULT_CONTEXT_PROMPT, DEFAULT_NLU_FALLBACK_RESPONSE, DEFAULT_SYSTEM_PROMPT | |||
| DEFAULT_CONTEXT_PROMPT, DEFAULT_NLU_FALLBACK_RESPONSE, DEFAULT_SYSTEM_PROMPT, DEFAULT_LLM | |||
There was a problem hiding this comment.
Move module-level imports to the top of the file.
To adhere to Python's best practices (PEP 8), ensure all imports are placed at the top of the file.
- from kairon.shared.data.constant import UTTERANCE_TYPE, EVENT_STATUS, STORY_EVENT, ALLOWED_DOMAIN_FORMATS, \
- ALLOWED_CONFIG_FORMATS, ALLOWED_NLU_FORMATS, ALLOWED_STORIES_FORMATS, ALLOWED_RULES_FORMATS, REQUIREMENTS, \
- DEFAULT_NLU_FALLBACK_RULE, SLOT_TYPE, KAIRON_TWO_STAGE_FALLBACK, AuditlogActions, TOKEN_TYPE, GPT_LLM_FAQ, \
- DEFAULT_CONTEXT_PROMPT, DEFAULT_NLU_FALLBACK_RESPONSE, DEFAULT_SYSTEM_PROMPT, DEFAULT_LLMCommittable suggestion was skipped due to low confidence.
| from deepdiff import DeepDiff | ||
| import litellm |
There was a problem hiding this comment.
Move module-level imports to the top of the file.
To adhere to Python's best practices (PEP 8), ensure all imports are placed at the top of the file.
- import litellmCommittable suggestion was skipped due to low confidence.
| @@ -8,8 +8,13 @@ | |||
| from datetime import datetime, timedelta, timezone | |||
| from io import BytesIO | |||
| from typing import List | |||
| from kairon.shared.utils import Utility | |||
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | |||
There was a problem hiding this comment.
Use capitalized environment variable SYSTEM_FILE instead of system_file.
Environment variables are conventionally capitalized to distinguish them from regular variables.
- os.environ["system_file"] = "./tests/testing_data/system.yaml"
+ os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | |
| os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml" |
Tools
Ruff
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
| @@ -67,8 +72,7 @@ | |||
| Utterances, BotSettings, ChatClientConfig, LookupTables, Forms, | |||
| SlotMapping, KeyVault, MultiflowStories, LLMSettings, | |||
| MultiflowStoryEvents, Synonyms, | |||
| Lookup, | |||
| DemoRequestLogs | |||
| Lookup | |||
There was a problem hiding this comment.
Move module-level imports to the top of the file.
To adhere to Python's best practices (PEP 8), ensure all imports are placed at the top of the file.
- from kairon.shared.data.data_objects import (TrainingExamples,
- Slots,
- Entities, EntitySynonyms, RegexFeatures,
- Utterances, BotSettings, ChatClientConfig, LookupTables, Forms,
- SlotMapping, KeyVault, MultiflowStories, LLMSettings,
- MultiflowStoryEvents, Synonyms,
- Lookup
- )Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
Refactor
llm_type.Tests
Chores