added secret and bot_id in config #1644
Conversation
WalkthroughThe pull request introduces changes 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
📜 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)
🔇 Additional comments (2)
kairon/nlu/classifiers/llm.py (2)
85-87: Verify configuration consistency
Let's ensure the configuration values are used consistently across the codebase.
Also applies to: 94-96, 190-193
✅ Verification successful
Based on the search results, I can now provide a final response:
Configuration values are consistently managed
The hardcoded model names found in the codebase are primarily in test files (tests/) which is expected for test assertions. The actual implementation in kairon/nlu/classifiers/llm.py properly uses configuration values:
- Model names are retrieved from component config with defaults:
component_config.get("prediction_model", "gpt-3.5-turbo") - Other parameters like temperature and top_k are also properly configured:
component_config.get("temperature", 0.0) - Retry mechanism is consistently set to 3 attempts across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent use of configuration values
# Look for hardcoded model names that should use config
rg -B 2 -A 2 '"text-embedding-3-small"|"gpt-3.5-turbo"|"gpt-4o-mini"' --type py
# Check for hardcoded retry values
rg -B 2 -A 2 'max_retries\s*=\s*[0-9]+' --type py
# Look for other potential configuration inconsistencies
rg -B 2 -A 2 'component_config\.get' --type py
Length of output: 24683
85-87: Verify secret handling in logs
Let's ensure there's no accidental logging of API keys throughout the codebase.
✅ Verification successful
No security concerns found in secret handling
The codebase shows proper secret handling practices:
- The
encrypt_secretsmethod inActionUtilityis used to mask sensitive data - Only debug logging of the LLM response object is present, which contains the model output, not the API key
- The API key is passed directly to the LLM client library without being logged
- No direct logging of
self.secretor API keys was found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential API key logging or exposure
# Look for logging statements that might contain API keys
rg -i "log.*secret|log.*api.?key" --type py
# Check for print statements near API key handling
rg -B 3 -A 3 "print.*secret|print.*api.?key" --type py
# Look for debug logging that might expose secrets
rg -i "debug.*secret|debug.*api.?key" --type py
Length of output: 4224
Script:
#!/bin/bash
# Let's check the ActionUtility.encrypt_secrets implementation and its usage
# Check the implementation of encrypt_secrets
ast-grep --pattern 'def encrypt_secrets($$$)'
# Check for any logging of self.secret or api_key in the LLM classifier
rg -B 3 -A 3 "self\.secret|api_key" kairon/nlu/classifiers/llm.py
# Look for any direct logging of secrets in the codebase
rg -i "logger\.(debug|info|warning|error).*self\.secret" --type py
Length of output: 1455
Script:
#!/bin/bash
# Let's check the ActionUtility class implementation and any secret masking logic
# Search for ActionUtility class definition
ast-grep --pattern 'class ActionUtility {
$$$
}'
# Check for any secret masking or encryption implementation
rg -B 3 -A 3 "mask|encrypt|hide.*secret" --type py
# Look for any logging of response objects that might contain secrets
rg -B 3 -A 3 "logger\.debug.*response" --type py
Length of output: 87432
Script:
#!/bin/bash
# Let's check the ActionUtility class and any secret masking in the actual implementation
# Look for ActionUtility class implementation
rg -B 5 -A 10 "class ActionUtility" --type py
# Check for any secret masking or encryption implementation in the codebase
rg -B 3 -A 3 "def encrypt_secrets|def mask_secret" --type py
# Check for any logging of response objects in the LLM classifier
rg -B 3 -A 3 "logger\.(debug|info|warning|error).*response" kairon/nlu/classifiers/llm.py
Length of output: 1991
|
|
||
| index = next((index for (index, d) in enumerate(config['pipeline']) if d["name"] == "kairon.nlu.LLMClassifier"), None) | ||
| if index: | ||
| config[index]['bot_id']= bot |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for pipeline configuration
The current implementation silently continues if the LLMClassifier is not found in the pipeline. Consider adding validation and logging to handle edge cases.
- index = next((index for (index, d) in enumerate(config['pipeline']) if d["name"] == "kairon.nlu.LLMClassifier"), None)
- if index:
- config[index]['bot_id']= bot
+ try:
+ if not isinstance(config.get('pipeline'), list):
+ raise ValueError("Invalid pipeline configuration: expected a list")
+
+ index = next((index for (index, d) in enumerate(config['pipeline'])
+ if d.get("name") == "kairon.nlu.LLMClassifier"), None)
+ if index is not None:
+ config['pipeline'][index]['bot_id'] = bot
+ else:
+ logging.debug(f"LLMClassifier not found in pipeline configuration for bot {bot}")
+ except Exception as e:
+ logging.error(f"Error updating LLMClassifier configuration: {str(e)}")
+ raise AppException(f"Failed to configure LLMClassifier: {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.
| index = next((index for (index, d) in enumerate(config['pipeline']) if d["name"] == "kairon.nlu.LLMClassifier"), None) | |
| if index: | |
| config[index]['bot_id']= bot | |
| try: | |
| if not isinstance(config.get('pipeline'), list): | |
| raise ValueError("Invalid pipeline configuration: expected a list") | |
| index = next((index for (index, d) in enumerate(config['pipeline']) | |
| if d.get("name") == "kairon.nlu.LLMClassifier"), None) | |
| if index is not None: | |
| config['pipeline'][index]['bot_id'] = bot | |
| else: | |
| logging.debug(f"LLMClassifier not found in pipeline configuration for bot {bot}") | |
| except Exception as e: | |
| logging.error(f"Error updating LLMClassifier configuration: {str(e)}") | |
| raise AppException(f"Failed to configure LLMClassifier: {str(e)}") |
| embeddings = litellm.embedding( | ||
| model="text-embedding-3-small", input=text, api_key=self.api_key, max_retries=3 | ||
| model="text-embedding-3-small", input=text, max_retries=3, **self.secret | ||
| ) |
There was a problem hiding this comment.
Use configuration value for embedding model
The embedding model name is hardcoded as "text-embedding-3-small" but should use the value from component_config.
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.
| embeddings = litellm.embedding( | |
| model="text-embedding-3-small", input=text, api_key=self.api_key, max_retries=3 | |
| model="text-embedding-3-small", input=text, max_retries=3, **self.secret | |
| ) | |
| embeddings = litellm.embedding( | |
| model=self.component_config.get("embedding_model", "text-embedding-3-small"), | |
| input=text, | |
| max_retries=self.component_config.get("retry", 3), | |
| **self.secret | |
| ) |
| presence_penalty=0, | ||
| api_key=self.api_key, | ||
| max_retries=3 | ||
| max_retries=3, | ||
| **self.secret | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent retry configuration
The max_retries parameter should use the value from component_config to maintain consistency.
- max_retries=3,
+ 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.
| presence_penalty=0, | |
| api_key=self.api_key, | |
| max_retries=3 | |
| max_retries=3, | |
| **self.secret | |
| ) | |
| presence_penalty=0, | |
| max_retries=self.component_config.get("retry", 3), | |
| **self.secret | |
| ) |
| llm_secret = Sysadmin.get_llm_secret("openai", bot_id) | ||
| self.api_key = llm_secret.get('api_key') | ||
| 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")} | ||
| else: | ||
| raise KeyError( | ||
| f"either set bot_id'in LLMClassifier config or set LLM_API_KEY in environment variables" | ||
| ) |
There was a problem hiding this comment.
Improve security in error handling
The error message reveals sensitive configuration details. Consider using a more generic error message.
else:
raise KeyError(
- f"either set bot_id'in LLMClassifier config or set LLM_API_KEY in environment variables"
+ "API key configuration is missing. Please check the documentation for setup instructions."
)📝 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_secret = Sysadmin.get_llm_secret("openai", bot_id) | |
| self.api_key = llm_secret.get('api_key') | |
| 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")} | |
| else: | |
| raise KeyError( | |
| f"either set bot_id'in LLMClassifier config or set LLM_API_KEY in environment variables" | |
| ) | |
| llm_secret = Sysadmin.get_llm_secret("openai", bot_id) | |
| self.secret = llm_secret.get('api_key') | |
| elif os.environ.get("LLM_API_KEY"): | |
| self.secret = {'api_key': os.environ.get("LLM_API_KEY")} | |
| else: | |
| raise KeyError( | |
| "API key configuration is missing. Please check the documentation for setup instructions." | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
90-90: f-string without any placeholders
Remove extraneous f prefix
(F541)
Summary by CodeRabbit
New Features
Bug Fixes