chore(wren-ai-service): minor updates#1380
Conversation
WalkthroughThis pull request consolidates API key configuration across multiple files by removing several vendor-specific variables and replacing them with a single Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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 (
|
cyyeh
added
module/ai-service
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wren-ai-service/tools/config/.env.dev.example (1)
1-3: Consolidation of Vendor API KeysThe vendor keys section now uses a single
OPENAI_API_KEYvariable instead of multiple vendor-specific keys. This aligns with the PR objectives by simplifying key management and reducing redundancy. Please ensure that all service components referencing the old keys have been updated to useOPENAI_API_KEY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docker/.env.example(1 hunks)docker/config.example.yaml(0 hunks)wren-ai-service/src/pipelines/generation/utils/sql.py(1 hunks)wren-ai-service/tools/config/.env.dev.example(1 hunks)wren-ai-service/tools/config/config.example.yaml(0 hunks)wren-ai-service/tools/config/config.full.yaml(0 hunks)wren-ai-service/tools/dev/.env(1 hunks)wren-launcher/utils/docker.go(1 hunks)
💤 Files with no reviewable changes (3)
- wren-ai-service/tools/config/config.example.yaml
- wren-ai-service/tools/config/config.full.yaml
- docker/config.example.yaml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pytest
- GitHub Check: pytest
🔇 Additional comments (5)
wren-ai-service/tools/dev/.env (1)
16-16: Good version pin for stability.Pinning the UI version to a specific release (0.20.1) rather than using the 'latest' tag improves deployment stability and reproducibility.
wren-launcher/utils/docker.go (1)
48-50: API key consolidation looks good.This change simplifies configuration by using a single
OPENAI_API_KEYinstead of separate keys for different services. This aligns with the PR objectives and makes the API key management more straightforward.docker/.env.example (1)
19-19: API key consolidation simplifies configuration.Consolidating multiple vendor-specific API keys into a single
OPENAI_API_KEYsimplifies the environment setup, improving developer experience and reducing configuration errors.wren-ai-service/src/pipelines/generation/utils/sql.py (1)
50-50: Fixed SQL breakdown error handling.The condition was inverted to correctly handle error cases by returning early when an error is detected. This improves the flow control and addresses the SQL breakdown issue mentioned in the PR objectives.
wren-ai-service/tools/config/.env.dev.example (1)
4-10: Validation of Langfuse and Evaluation ConfigurationThe configuration entries for
LANGFUSE_SECRET_KEY,LANGFUSE_PUBLIC_KEY, andLANGFUSE_PROJECT_IDremain unchanged as expected. Confirm that these keys continue to be properly utilized in your application, and that no dependencies were inadvertently affected by the API key consolidation elsewhere.
|
Overall, LGTM!, but it seems having test cases failed. Can you check the failed cases? Thanks a lot! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deployment/kustomizations/examples/secret-wren_example.yaml (1)
8-9: Consolidate API Key Management & Verify Placeholder UseThe update correctly removes the two legacy API keys and introduces a single
OPENAI_API_KEYalong with an updated comment. Please ensure that the provided base64-encoded value is a placeholder (or dummy) value and not a real API key, so that sensitive credentials are not inadvertently exposed in version control. If this is for production, consider loading the secret from a secured external source instead.🧰 Tools
🪛 Gitleaks (8.21.2)
9-9: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ai-service-test.yaml(1 hunks)CONTRIBUTING.md(1 hunks)deployment/kustomizations/README.md(1 hunks)deployment/kustomizations/base/cm.yaml(0 hunks)deployment/kustomizations/base/deploy-wren-ai-service.yaml(1 hunks)deployment/kustomizations/examples/secret-wren_example.yaml(1 hunks)wren-ai-service/eval/__init__.py(1 hunks)wren-ai-service/eval/dspy_modules/prompt_optimizer.py(5 hunks)wren-ai-service/eval/evaluation.py(1 hunks)wren-ai-service/src/providers/llm/litellm.py(1 hunks)wren-ui/README.md(1 hunks)
💤 Files with no reviewable changes (1)
- deployment/kustomizations/base/cm.yaml
✅ Files skipped from review due to trivial changes (3)
- wren-ai-service/eval/evaluation.py
- wren-ai-service/eval/dspy_modules/prompt_optimizer.py
- wren-ai-service/src/providers/llm/litellm.py
🧰 Additional context used
🪛 Gitleaks (8.21.2)
deployment/kustomizations/examples/secret-wren_example.yaml
9-9: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pytest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
wren-ui/README.md (1)
85-85: API key variable has been updated to use unified convention.The change from requiring both
LLM_OPENAI_API_KEYandEMBEDDER_OPENAI_API_KEYto a singleOPENAI_API_KEYsimplifies the setup process. This is consistent with the PR's objective to consolidate API keys.wren-ai-service/eval/__init__.py (1)
12-12: Field alias updated to use the consolidated API key variable.The change from
LLM_OPENAI_API_KEYtoOPENAI_API_KEYas the alias for theopenai_api_keyfield ensures consistent API key usage throughout the application.This change aligns with the PR's goal of removing specific API keys in favor of a unified approach.
.github/workflows/ai-service-test.yaml (1)
57-57: CI workflow updated to use the consolidated API key.The workflow has been updated to use a single
OPENAI_API_KEYenvironment variable instead of the previous separate keys (LLM_OPENAI_API_KEYandEMBEDDER_OPENAI_API_KEY).This change ensures that the CI workflow remains functional after the API key consolidation.
CONTRIBUTING.md (1)
61-61: Contributing docs updated to reflect API key changes.The documentation has been updated to instruct contributors to fill in only
OPENAI_API_KEYinstead of bothLLM_OPENAI_API_KEYandEMBEDDER_OPENAI_API_KEY, maintaining consistency with the implementation changes.deployment/kustomizations/README.md (1)
32-33: Update Documentation to Reflect API Key ChangesThe documentation now correctly indicates that only the
OPENAI_API_KEYis required for thewren-ai-service-deploymentpod, improving clarity and aligning with the consolidated key management strategy.deployment/kustomizations/base/deploy-wren-ai-service.yaml (1)
27-31: Consolidate Deployment Environment VariablesThe deployment manifest now sources
OPENAI_API_KEYfrom thewrenai-secretssecret, replacing the previously usedLLM_OPENAI_API_KEYandEMBEDDER_OPENAI_API_KEY. This not only simplifies API key management, but it also ensures consistency across the deployment configurations.
2 participants
Summary by CodeRabbit
Chores
Documentation