feat: fully switch to external provider module method#37
feat: fully switch to external provider module method#37leseb merged 1 commit intoopendatahub-io:mainfrom
Conversation
cdoern
requested review from
Elbehery,
VaishnaviHire,
derekhiggins,
leseb,
nathan-weinberg,
rhdedgar,
rhuss and
skamenan7
as code owners
WalkthroughRemoved top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-15T14:25:54.837ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
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 |
|
docker is down :( |
![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 (2)
distribution/run.yaml (2)
79-79: Declare lmeval module on the eval provider (avoid hidden coupling).llama_stack_provider_lmeval is declared on an inline::meta-reference in distribution/run.yaml while provider_id: trustyai_lmeval has no module; declare the module with the provider to make the dependency explicit (distribution/build.yaml already does this).
- provider_id: trustyai_lmeval provider_type: remote::trustyai_lmeval + module: llama_stack_provider_lmeval==${env.LMEVAL_PROVIDER_VERSION:=0.2.4} config:
71-71: Parameterize FMS provider version (avoid hard pins across files).rg found the same hard pin in three places: distribution/run.yaml:71, distribution/build.yaml:15, distribution/Containerfile:44 — centralize via a single env var (same default) so CI/CD can bump once.
Apply to run.yaml (still valid):
- module: llama_stack_provider_trustyai_fms==0.2.2 + module: llama_stack_provider_trustyai_fms==${env.FMS_PROVIDER_VERSION:=0.2.2}Also update:
- distribution/build.yaml: replace the hard pin with the same ${env.FMS_PROVIDER_VERSION:=0.2.2} form.
- distribution/Containerfile: add a build ARG (e.g. ARG FMS_PROVIDER_VERSION=0.2.2) and use pip install llama_stack_provider_trustyai_fms==${FMS_PROVIDER_VERSION}.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
distribution/build.yaml(0 hunks)distribution/providers.d/remote/eval/trustyai_lmeval.yaml(0 hunks)distribution/providers.d/remote/safety/trustyai_fms.yaml(0 hunks)distribution/run.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- distribution/providers.d/remote/safety/trustyai_fms.yaml
- distribution/build.yaml
- distribution/providers.d/remote/eval/trustyai_lmeval.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T14:25:54.837Z
Learnt from: nathan-weinberg
PR: opendatahub-io/llama-stack-distribution#33
File: distribution/Containerfile:17-21
Timestamp: 2025-09-15T14:25:54.837Z
Learning: In the opendatahub-io/llama-stack-distribution repository, the distribution/Containerfile is auto-generated by distribution/build.py based on configuration in build.yaml. When providers are added to build.yaml, the build script automatically regenerates the Containerfile with the required dependencies. Changes to the Containerfile should not be flagged as manual edits if they correspond to legitimate changes in the build configuration.
Applied to files:
distribution/run.yaml
nathan-weinberg
left a comment
nathan-weinberg
left a comment
There was a problem hiding this comment.
You'll need to make some changes to the Containerfile.in as well, see this failure from CI
> [12/12] COPY distribution/providers.d/ /opt/app-root/src/.llama/providers.d/:
------
Containerfile:50
--------------------
48 | RUN mkdir -p ${HOME}/.llama/providers.d ${HOME}/.cache
49 | COPY distribution/run.yaml ${APP_ROOT}/run.yaml
50 | >>> COPY distribution/providers.d/ ${HOME}/.llama/providers.d/
51 |
52 | ENTRYPOINT ["python", "-m", "llama_stack.core.server.server", "/opt/app-root/run.yaml"]
--------------------
ERROR: failed to build: failed to solve: failed to compute cache key: failed to calculate checksum of ref tob88amabijate2le42cdxuvi::m7nnnfel8kzryzflgos090pz9: "/distribution/providers.d": not found
cdoern
force-pushed
the
modules
branch
2 times, most recently
from
424c24c to
1f300a5
Compare
delete `providers.d` and depend on `module` for build and run. also delete `external_provider_dir` from both build and run yaml. this forces `build.py` to only install uv pip install llama_stack_provider_lmeval==0.2.4 uv pip install llama_stack_provider_trustyai_fms==0.2.2 rather than also grabbing dependencies like `kubernetes` Signed-off-by: Charlie Doern <cdoern@redhat.com>
leseb
left a comment
leseb
left a comment
There was a problem hiding this comment.
Downstream still lacks a similar update to the run.yaml let's do that before we merge anything here. Thanks!
nathan-weinberg
added
the
do-not-merge
nathan-weinberg
left a comment
nathan-weinberg
left a comment
There was a problem hiding this comment.
Approving from my side since everything I brought up is addressed
nathan-weinberg
removed
the
do-not-merge
chore: bump wheel release to use 0.4.1 and fix config file path
4 participants
What does this PR do?
delete
providers.dand depend onmodulefor build and run. also deleteexternal_provider_dirfrom both build and run yaml.this forces
build.pyto only installuv pip install llama_stack_provider_lmeval==0.2.4
uv pip install llama_stack_provider_trustyai_fms==0.2.2
rather than also grabbing dependencies like
kubernetesSummary by CodeRabbit
Refactor
Chores
Removals