Llm bot id classifier corrected #1646
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
kairon/nlu/classifiers/llm.py (1)
90-90: Remove unnecessary f-string prefixThe error message doesn't contain any placeholders.
- raise KeyError( - f"either set bot_id'in LLMClassifier config or set LLM_API_KEY in environment variables" - ) + raise KeyError( + "either set bot_id in LLMClassifier config or set LLM_API_KEY in environment variables" + )🧰 Tools
🪛 Ruff (0.8.2)
90-90: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kairon/nlu/classifiers/llm.py(2 hunks)kairon/train.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
kairon/nlu/classifiers/llm.py
90-90: f-string without any placeholders
Remove extraneous f prefix
(F541)
| index = next((index for (index, d) in enumerate(config['pipeline']) if d["name"] == "kairon.nlu.LLMClassifier"), None) | ||
| if index: | ||
| config['pipeline'][index]['bot_id']= bot |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for LLMClassifier configuration
While the logic is correct, consider adding error handling to make the code more robust:
- index = next((index for (index, d) in enumerate(config['pipeline']) if d["name"] == "kairon.nlu.LLMClassifier"), None)
- if index:
- config['pipeline'][index]['bot_id']= bot
+ if not config.get('pipeline'):
+ raise ValueError("Pipeline configuration is missing")
+
+ index = next((index for (index, d) in enumerate(config['pipeline']) if d["name"] == "kairon.nlu.LLMClassifier"), None)
+ if index is not None:
+ config['pipeline'][index]['bot_id'] = bot
+ else:
+ logging.debug("LLMClassifier not found in pipeline configuration")📝 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.
| index = next((index for (index, d) in enumerate(config['pipeline']) if d["name"] == "kairon.nlu.LLMClassifier"), None) | |
| if index: | |
| config['pipeline'][index]['bot_id']= bot | |
| if not config.get('pipeline'): | |
| raise ValueError("Pipeline configuration is missing") | |
| index = next((index for (index, d) in enumerate(config['pipeline']) if d["name"] == "kairon.nlu.LLMClassifier"), None) | |
| if index is not None: | |
| config['pipeline'][index]['bot_id'] = bot | |
| else: | |
| logging.debug("LLMClassifier not found in pipeline configuration") |
| model="text-embedding-3-small", input=text, max_retries=3, **self.secret | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use configuration for embedding model
The embedding model name is hardcoded despite being available in the component configuration.
embeddings = litellm.embedding(
- model="text-embedding-3-small", input=text, max_retries=3, **self.secret
+ model=self.component_config.get("embedding_model", "text-embedding-3-small"),
+ input=text,
+ max_retries=self.component_config.get("retry", 3),
+ **self.secret
)📝 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.
| model="text-embedding-3-small", input=text, max_retries=3, **self.secret | |
| ) | |
| model=self.component_config.get("embedding_model", "text-embedding-3-small"), | |
| input=text, | |
| max_retries=self.component_config.get("retry", 3), | |
| **self.secret | |
| ) |
| max_retries=3, | ||
| **self.secret |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in predict method
The error handling could be more specific and informative.
max_retries=3,
**self.secret
)
logger.debug(response)
responses = json.loads(response.choices[0]["message"]["content"])
intent = responses["intent"] if "intent" in responses.keys() else "nlu_fallback"
explanation = responses["explanation"] if "explanation" in responses.keys() else None
entities = responses["entities"]if "entities" in responses.keys() else []
- except Exception as e:
- logger.error(e)
+ except json.JSONDecodeError as e:
+ logger.error(f"Failed to parse LLM response: {e}")
+ except (KeyError, IndexError) as e:
+ logger.error(f"Unexpected response format from LLM: {e}")
+ except Exception as e:
+ logger.error(f"Error during prediction: {str(e)}")Committable suggestion skipped: line range outside the PR's diff.
| self.secret = llm_secret.get('api_key') | ||
| elif os.environ.get("LLM_API_KEY"): | ||
| self.api_key = os.environ.get("LLM_API_KEY") | ||
| self.secret = {'api_key': os.environ.get("LLM_API_KEY")} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for API key and type hints
The API key handling could be more robust with proper validation and type hints.
+ secret: Dict[str, str]
+
def load_api_key(self, bot_id: Text):
if bot_id:
from kairon.shared.admin.processor import Sysadmin
llm_secret = Sysadmin.get_llm_secret("openai", bot_id)
- self.secret = llm_secret.get('api_key')
+ api_key = llm_secret.get('api_key')
+ if not api_key:
+ raise ValueError("API key not found in llm_secret")
+ self.secret = {'api_key': api_key}
elif os.environ.get("LLM_API_KEY"):
self.secret = {'api_key': os.environ.get("LLM_API_KEY")}
else:
raise KeyError(
"either set bot_id in LLMClassifier config or set LLM_API_KEY in environment variables"
)📝 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.
| self.secret = llm_secret.get('api_key') | |
| elif os.environ.get("LLM_API_KEY"): | |
| self.api_key = os.environ.get("LLM_API_KEY") | |
| self.secret = {'api_key': os.environ.get("LLM_API_KEY")} | |
| secret: Dict[str, str] | |
| def load_api_key(self, bot_id: Text): | |
| if bot_id: | |
| from kairon.shared.admin.processor import Sysadmin | |
| llm_secret = Sysadmin.get_llm_secret("openai", bot_id) | |
| api_key = llm_secret.get('api_key') | |
| if not api_key: | |
| raise ValueError("API key not found in llm_secret") | |
| self.secret = {'api_key': api_key} | |
| elif os.environ.get("LLM_API_KEY"): | |
| self.secret = {'api_key': os.environ.get("LLM_API_KEY")} | |
| else: | |
| raise KeyError( | |
| "either set bot_id in LLMClassifier config or set LLM_API_KEY in environment variables" | |
| ) |
Summary by CodeRabbit
New Features
Bug Fixes