Updted the template to use the LLAMA_STACK_2_0_FLASH_MODEL as the default model #77
Conversation
…ault model Signed-off-by: Eran Cohen <[email protected]>
WalkthroughThe changes update parameter names for Llama Stack models in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, eranco74 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 |
|
/lgtm |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
template.yaml (1)
303-306: Model entry duplicates provider_id in provider_model_id.
provider_model_idis expected to be the provider-specific model
identifier, not a fully qualified<provider>/<model>string.
With the current value it equalsmodel_id, defeating the separation
between “who serves” and “what is served”.If you keep full paths in
model_id, consider trimming
provider_model_idtogemini-2.0-flash(or whatever the provider
expects). Otherwise callers will have to strip the provider prefix
themselves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
template.yaml(3 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). (1)
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
🔇 Additional comments (2)
template.yaml (2)
78-86: Parameter values already embed the provider prefix – verify consistency.The new 2.5-Pro / 2.5-Flash / 2.0-Flash parameters carry the full
gemini/<model>path. Down-stream code now mixes two patterns:
${LLAMA_STACK_INFERENCE_PROVIDER}/${LLAMA_STACK_*_MODEL}(lines 146-147)${LLAMA_STACK_*_MODEL}alone (models list below)This asymmetry makes it very easy to produce duplicate prefixes
(gemini/gemini/...) or mismatches if the provider is changed.Confirm whether the intended contract is:
- “MODEL” parameters contain ONLY the model fragment, or
- They contain the full
<provider>/<model>string and should be used as-is.If #2 is correct, the expression on lines 146-147 must be simplified (see next comment).
308-310: 👍 Renaming to version-specific parameters improves clarity.Using explicit 2.5-Pro / 2.5-Flash identifiers removes ambiguity and
will age better than generic “default/flash” names.Also applies to: 313-315
| default_model: "${LLAMA_STACK_INFERENCE_PROVIDER}/${LLAMA_STACK_2_0_FLASH_MODEL}" | ||
| default_provider: ${LLAMA_STACK_INFERENCE_PROVIDER} |
There was a problem hiding this comment.
default_model builds an invalid path (gemini/gemini/...).
Because ${LLAMA_STACK_2_0_FLASH_MODEL} already starts with
gemini/, prepending ${LLAMA_STACK_INFERENCE_PROVIDER}/
duplicates the provider segment and will break routing to the model.
- default_model: "${LLAMA_STACK_INFERENCE_PROVIDER}/${LLAMA_STACK_2_0_FLASH_MODEL}"
+ # Param already includes provider – no need to prepend it
+ default_model: "${LLAMA_STACK_2_0_FLASH_MODEL}"Apply the same rule wherever the “MODEL” parameters are concatenated
with the provider.
📝 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.
| default_model: "${LLAMA_STACK_INFERENCE_PROVIDER}/${LLAMA_STACK_2_0_FLASH_MODEL}" | |
| default_provider: ${LLAMA_STACK_INFERENCE_PROVIDER} | |
| # Param already includes provider – no need to prepend it | |
| default_model: "${LLAMA_STACK_2_0_FLASH_MODEL}" | |
| default_provider: ${LLAMA_STACK_INFERENCE_PROVIDER} |
🤖 Prompt for AI Agents
In template.yaml around lines 146 to 147, the default_model value concatenates
the provider and model variables, causing duplication of the provider segment in
the path. To fix this, remove the provider prefix from default_model and use
only the model variable since it already includes the provider segment. Apply
this correction consistently wherever model parameters are concatenated with the
provider to avoid invalid paths.
There was a problem hiding this comment.
We know it's strange, we have a task for that
https://issues.redhat.com/browse/MGMT-21341
openshift-merge-bot
bot
merged commit 8315c15
into
rh-ecosystem-edge:main
3 participants
Summary by CodeRabbit