-
Notifications
You must be signed in to change notification settings - Fork 21
Update local llama stack config #240
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.
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
base: main
Are you sure you want to change the base?
Update local llama stack config #240
Conversation
With the latest images this doesn't work anymore, make the configuration more like what we use in the SaaS template
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughConfiguration file updated to migrate database connection strings from hardcoded localhost values to environment variables, update telemetry service configuration with new sink settings and paths, reorganize model definitions with new host/port/db/user/password fields, and change MCP endpoint URI from service hostname to localhost. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/llama_stack_client_config.yaml (2)
29-35: Verify all required environment variables are properly configured in the runtime environment.The configuration depends on multiple environment variables (
ASSISTED_CHAT_POSTGRES_HOST,ASSISTED_CHAT_POSTGRES_PORT,ASSISTED_CHAT_POSTGRES_NAME,ASSISTED_CHAT_POSTGRES_USER,ASSISTED_CHAT_POSTGRES_PASSWORD,STORAGE_MOUNT_PATH). Ensure these are:
- Documented in setup/deployment guides
- Set in local development, CI/CD, and production environments
- Not accidentally leaked in logs or configs
Consider adding a validation step (e.g., startup check) to catch missing environment variables early.
Also applies to: 38-44, 70-76
34-34: Confirm SSL is intentionally disabled for local development.All PostgreSQL connections have
ssl_mode: disableandca_cert_path: /etc/tls/ca-bundle.pem. While acceptable for local development, confirm this is not used in staging/production environments. Consider:
- Adding environment-specific configs or conditional SSL settings
- Documenting that SSL must be enabled for production deployments
- Defaulting to
ssl_mode: requirefor safetyAlso applies to: 43-43, 75-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/llama_stack_client_config.yaml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-chat-test-image-saas-main-on-pull-request
🔇 Additional comments (4)
config/llama_stack_client_config.yaml (4)
19-19: Verify API key placeholder is appropriate for Vertex integration.The api_key is set to a placeholder string with a comment indicating Vertex is used instead of Gemini directly. Confirm this placeholder prevents errors during Llama stack initialization and that the actual Vertex authentication is handled elsewhere (e.g., via GOOGLE_APPLICATION_CREDENTIALS or similar).
67-67: Verify STORAGE_MOUNT_PATH environment variable is consistently used.The metadata_store db_path references
${STORAGE_MOUNT_PATH}which also appears on line 51 (telemetry sqlite_db_path). Ensure this path is writable and persistent in your local/production environment, and document its purpose and expected location.
77-77: Clarify why the models list is now empty.The models configuration has been changed from populated entries to an empty list (
models: []). Confirm this is intentional and does not represent an incomplete migration. If models are defined elsewhere (e.g., environment variables, separate config file), document that reference.
87-87: Verify MCP endpoint localhost change works in your local setup.The MCP endpoint URI changed from
http://assisted-service-mcp:8000/mcp(service hostname) tohttp://localhost:8000/mcp(localhost). This assumes:
- The MCP service is accessible on localhost:8000 in local development
- No cross-container networking is required (or port forwarding is configured)
Confirm this works in your containerized local environment and document the setup if there are special networking requirements.
|
@carbonin: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| password: ${env.ASSISTED_CHAT_POSTGRES_PASSWORD} | ||
| ssl_mode: disable | ||
| ca_cert_path: /etc/tls/ca-bundle.pem | ||
| models: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luis5tb is that OK? You mentioned you're still using Gemini without vertex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that means it requires to be added later on. My question here is that it seems a changed not mentioned in the PR. It it was because it was failing, perhaps just need to move from gemini/gemini-XXX to models/gemini-XXX
| provider_type: remote::gemini | ||
| config: | ||
| api_key: ${env.GEMINI_API_KEY:=} | ||
| api_key: dummy-to-stop-llama-stack-from-complaining-even-though-we-use-vertex-and-not-gemini-directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
With the latest images this doesn't work anymore, make the configuration more like what we use in the SaaS template
Summary by CodeRabbit
Chores