Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

secret mapping corrected #1654

Merged
merged 1 commit into from
Dec 13, 2024
Merged

secret mapping corrected #1654

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion kairon/nlu/classifiers/llm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def __init__(
data: Optional[Dict[Text, Any]] = None,
) -> None:
"""Construct a new intent classifiers using the OpenAI Completion framework."""
self.secret = {}
self.component_config = config
self._model_storage = model_storage
self._resource = resource
Expand Down Expand Up @@ -82,7 +83,7 @@ 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')
self.secret = llm_secret
elif os.environ.get("LLM_API_KEY"):
self.secret = {'api_key': os.environ.get("LLM_API_KEY")}
else:
Comment on lines +86 to 89
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Based on the search results, I can now generate the final response as the verification is complete. The results show that llm_secret from Sysadmin contains multiple fields including 'api_key', 'api_version', and 'api_base_url' (as seen in the test files), while the environment variable fallback only sets 'api_key'.

Standardize secret structure between Sysadmin and environment variable sources

The secret structure from Sysadmin includes additional fields ('api_version', 'api_base_url') that are missing in the environment variable fallback. This inconsistency could cause runtime errors when code attempts to access these fields.

  • kairon/shared/actions/utils.py: Uses get() safely for api_key
  • tests/unit_test/admin/admin_processor_test.py: Shows Sysadmin secrets contain 'api_key', 'api_version', 'api_base_url'
  • kairon/nlu/classifiers/llm.py: Environment fallback only sets 'api_key'

Consider:

  1. Adding the missing fields to the environment variable fallback structure
  2. Documenting the required secret structure
  3. Using safe dictionary access (.get()) for optional fields
🔗 Analysis chain

Ensure consistent secret structure between sources

The secret structure differs between the two sources:

  • llm_secret from Sysadmin is assigned directly
  • Environment variable fallback only sets {'api_key': value}

This could lead to issues if the code expects additional fields from llm_secret that won't be present when using the environment variable.

Let's check how llm_secret is structured:

Consider standardizing the secret structure by either:

  1. Documenting the expected secret structure
  2. Adding validation for required fields
  3. Ensuring both sources provide the same fields
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the structure of llm_secret in Sysadmin
rg -A 5 "get_llm_secret" --type py

Length of output: 4412

Expand Down
Loading