LCORE-599: bump-up Llama Stack version to 0.2.18 #452
Conversation
WalkthroughDependency pins for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🧹 Nitpick comments (1)
pyproject.toml (1)
28-29: Version pinning matches constants; lockfile and compatibility checks still needed
- ✅ The
llama-stackandllama-stack-clientpins in pyproject.toml (0.2.18) match theMAXIMAL_SUPPORTED_LLAMA_STACK_VERSIONin src/constants.py (0.2.18), and theMINIMAL_SUPPORTED_LLAMA_STACK_VERSIONremains 0.2.17 ≤ 0.2.18 as expected.- 🔄 Please regenerate/update any lockfiles (e.g., pdm.lock) and rebuild container images so CI and production environments pick up version 0.2.18 deterministically.
- 🔍 Sanity-check cross-version compatibility: ensure a 0.2.18 client still works with a 0.2.17 server. If you intend to drop support for 0.2.17, bump the minimal version in src/constants.py and update the release notes accordingly:
--- a/src/constants.py +++ b/src/constants.py @@ -MINIMAL_SUPPORTED_LLAMA_STACK_VERSION = "0.2.17" +MINIMAL_SUPPORTED_LLAMA_STACK_VERSION = "0.2.18"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.toml(1 hunks)src/constants.py(1 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: build-pr
🔇 Additional comments (1)
src/constants.py (1)
5-5: LGTM: MAXIMAL_SUPPORTED_LLAMA_STACK_VERSION aligned to 0.2.18.This matches the pins in pyproject.toml and keeps the declared support ceiling consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
run.yaml (1)
68-74: Avoid writing DPO artifacts to "."; choose a persistent, ignored path or env overrideUsing dpo_output_dir: "." will dump potentially large/ephemeral training outputs into the repo root, which can:
- pollute the working tree and risk accidental commits,
- fail in containerized/readonly workspaces,
- be wiped between runs if not volume-mounted.
Recommend a persistent path under .llama to match the rest of the config, or allow an env override with a sane default.
Apply one of these:
- dpo_output_dir: "." + # Prefer a persistent, repo-ignored location + dpo_output_dir: .llama/distributions/ollama/post_training/dpo_outor env-driven with fallback:
- dpo_output_dir: "." + dpo_output_dir: ${env.DPO_OUTPUT_DIR:-.llama/distributions/ollama/post_training/dpo_out}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
run.yaml(1 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: build-pr
🔇 Additional comments (2)
run.yaml (2)
68-74: Ensure directory creation and permissionsIf the provider does not auto-create dpo_output_dir, first runs will fail. Also, non-root containers may lack write permission to the chosen path.
Confirm the provider behavior in 0.2.18:
- Creates the directory if missing,
- Emits a clear error if unwritable.
If not auto-created, ensure the orchestration step creates the directory with correct ownership prior to starting the service.
68-74: I couldn’t find any schema definition for the newdpo_output_dirkey in our Pydantic models or JSON schemas. Since the codebase usessrc/models/config.py(withConfigurationas the root Pydantic model) to validaterun.yaml, adding an unexpected field will indeed raise a validation error on version 0.2.17.• In
src/constants.py,MINIMAL_SUPPORTED_LLAMA_STACK_VERSIONis set to"0.2.17".
• There is no reference todpo_output_dirin any Pydantic model undersrc/models/config.py.Please manually verify:
- Whether version 0.2.17’s schema will reject unknown fields (it does by default in Pydantic unless
extra = "ignore"is set).- If so, either bump
MINIMAL_SUPPORTED_LLAMA_STACK_VERSIONto"0.2.18"insrc/constants.pyand update the version-bound unit tests, or guard thedpo_output_dirfield inrun.yamlbehind a version check in your deployment tooling.
tisnik
force-pushed
the
lcore-599-bump-llama-stack-version-to-0.2.18
branch
from
f512267 to
3e44dde
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/e2e_tests.yaml (3)
4-11: Avoid pull_request_target + job-wide secrets: high risk of secret exposure to untrusted PRsThis workflow runs on pull_request_target and exposes OPENAI_API_KEY at the job scope. Combined with checking out the PR head, this lets untrusted code influence steps that have access to secrets. Mitigations like persist-credentials:false don’t prevent runtime exfiltration via docker, curl, or logs.
Recommended hardening:
- Use pull_request instead of pull_request_target for forked PRs, or gate secret-using steps to same-repo PRs only.
- Move OPENAI_API_KEY out of job env; pass it only to trusted, gated steps.
- Add least-privilege permissions.
Example patch:
- on: [push, pull_request_target] + on: [push, pull_request] +permissions: + contents: read jobs: e2e_tests: runs-on: ubuntu-latest - env: - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + # Do not set secrets at job scope. Gate usage at step level: + # Use: if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository)
228-233: Remove secret-dumping risk: don’t cat run.yaml to logsSince run.yaml currently embeds the API key, cat run.yaml risks leaking it into logs. Even with masking, avoid printing files that may contain secrets.
- - name: list files - run: | - ls - cat lightspeed-stack.yaml - cat run.yaml + - name: list files + run: | + ls + # Avoid printing config files that may contain secrets + sed -n '1,120p' lightspeed-stack.yaml | sed -e 's/api_key:.*/api_key: [redacted]/'
163-167: Prevent Secret Leakage in run.yaml Creation and LoggingThe workflow currently interpolates
${{ env.OPENAI_API_KEY }}directly intorun.yamland then prints that file, risking secret exposure.• Location 1:
.github/workflows/e2e_tests.yaml
– Step “create run.yaml” (around lines 46–90) writes:
yaml providers: inference: - provider_id: openai provider_type: remote::openai config: api_key: ${{ env.OPENAI_API_KEY }}
• Location 2: Same file, “list files” step (around lines 200–210) runscat run.yaml, logging its contents.Critical fixes required:
- Remove inline secret from
run.yamlcontent.- Inject the secret via environment only at runtime.
- Eliminate any
cat run.yamlcalls.Minimal diff suggestions:
- uses: 1arp/create-a-file-action@0.4.5 with: file: 'run.yaml' content: | version: '2' … providers: inference: - - provider_id: openai - provider_type: remote::openai - config: - api_key: ${{ env.OPENAI_API_KEY }} + - provider_id: openai + provider_type: remote::openai + # api_key is injected at runtime; do not write secrets here. - name: list files - run: | - ls - cat lightspeed-stack.yaml - cat run.yaml + run: | + ls + cat lightspeed-stack.yaml + # Do not cat run.yaml to avoid logging secrets - name: Run service manually - env: - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) + env: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
🧹 Nitpick comments (4)
.github/workflows/e2e_tests.yaml (4)
172-175: dpo_output_dir: "." may point to a non-writable/ephemeral location inside the containerUsing "." relies on the container’s working directory and may collide with bind mounts or lack write perms. Align with your other stores under /app-root/.llama to be explicit and consistent.
- dpo_output_dir: "." + dpo_output_dir: "/app-root/.llama/post_training"Action items:
- Ensure this path exists and is writable in the container image.
- If artifacts are needed post-job, mount it as a volume to the runner workspace or $RUNNER_TEMP.
245-249: Replace fixed sleep with a health/ready waitStatic sleep 20 is brittle. Prefer a bounded wait loop on container health or readiness to reduce flakes and speed up happy paths.
Example:
- - name: Wait for services - run: | - echo "Waiting for services to be healthy..." - sleep 20 # adjust depending on boot time + - name: Wait for services + run: | + echo "Waiting for services to be healthy..." + for i in {1..30}; do + if curl -sf http://localhost:8080/v1/models >/dev/null; then + echo "Service is up"; exit 0 + fi + sleep 2 + done + echo "Service did not become ready in time"; exit 1
13-21: Pin third-party actions by commit SHA to prevent supply-chain attacksactions/checkout@v4 and 1arp/create-a-file-action@0.4.5 are version tags. Pin to a specific commit SHA (with a comment referencing the tag) to avoid unexpected updates.
Example:
- - uses: actions/checkout@v4 + - uses: actions/checkout@3df4ab11feb006e8e1fc4b474e110515c8d3f0af # v4.1.7 - - uses: 1arp/create-a-file-action@0.4.5 + - uses: 1arp/create-a-file-action@<commit-sha> # 0.4.5Also applies to: 60-67, 94-101
26-46: Debug-only steps should be gated or removed for this dependency bumpThe verbose “Debug checkout …” and “Verify actual git checkout result” steps add noise/time. Gate them behind an input or env flag, or remove for mainline CI.
- - name: Debug checkout for umago/lightspeed-stack setup-metrics branch + - name: Debug checkout (optional) + if: ${{ env.CI_DEBUG == '1' }} ... - - name: Verify actual git checkout result + - name: Verify actual git checkout result (optional) + if: ${{ env.CI_DEBUG == '1' }}Also applies to: 47-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/e2e_tests.yaml(2 hunks)run.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- run.yaml
⏰ 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: build-pr
🔇 Additional comments (1)
.github/workflows/e2e_tests.yaml (1)
260-266: LGTM on trailing newline fixThe step remains the same; adding a trailing newline is fine.
1 participant
Description
LCORE-599: bump-up Llama Stack version to 0.2.18
Type of change
Related Tickets & Documents
Summary by CodeRabbit
Chores
dpo_output_dirwith default"."for the HuggingFace provider.Notes