chore: resync downstream content#25
Conversation
The distribution config file was missing the declaration of the Files API in the APIs list so it can be activated. Relates to: RHAIENG-897 Signed-off-by: Sébastien Han <seb@redhat.com>
We can now optionally use the remote::milvus provider. The provider would be activated if the MILVUS_ENDPOINT env variable is provider in the execution context. Relates to: RHAIENG-873 Signed-off-by: Sébastien Han <seb@redhat.com>
leseb
requested review from
Elbehery,
VaishnaviHire,
cdoern,
derekhiggins,
nathan-weinberg,
rhdedgar,
rhuss and
skamenan7
as code owners
So that we can fetch the right dependencies and build our distro image correctly. Relates to: RHAIENG-874 Signed-off-by: Sébastien Han <seb@redhat.com>
New files API and the localfs provider. Mostly used for RAG. Relates to: RHAIENG-896 Signed-off-by: Sébastien Han <seb@redhat.com>
![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)
distribution/run.yaml (1)
48-61: Remote Milvus gating and defaults — please verify behavior; consider safer typed defaults.Two checks:
- If MILVUS_ENDPOINT is unset,
${env.MILVUS_ENDPOINT:+milvus-remote}yields an empty provider_id. Confirm the config loader fully drops this provider block rather than creating a provider with an empty ID.- Several fields likely expect typed values. Empty-string defaults may violate schema validation (e.g., booleans/enums).
Suggested minimal hardening:
- Require uri when enabled; avoid empty default.
- Provide boolean and enum-safe defaults for secure and consistency_level.
Proposed diff:
- provider_id: ${env.MILVUS_ENDPOINT:+milvus-remote} provider_type: remote::milvus config: - uri: ${env.MILVUS_ENDPOINT:=} + uri: ${env.MILVUS_ENDPOINT} token: ${env.MILVUS_TOKEN:=} - secure: ${env.MILVUS_SECURE:=} - consistency_level: ${env.MILVUS_CONSISTENCY_LEVEL:=} + secure: ${env.MILVUS_SECURE:=false} + consistency_level: ${env.MILVUS_CONSISTENCY_LEVEL:=Bounded} ca_pem_path: ${env.MILVUS_CA_PEM_PATH:=} client_pem_path: ${env.MILVUS_CLIENT_PEM_PATH:=} client_key_path: ${env.MILVUS_CLIENT_KEY_PATH:=} kvstore: type: sqlite namespace: null db_path: /opt/app-root/src/.llama/distributions/rh/milvus_remote_registry.dbIf the loader does not drop empty provider blocks, one safe alternative is to keep provider_id constant and gate via a boolean flag your loader supports (e.g.,
enabled: ${env.MILVUS_ENDPOINT:+true}), or move this provider to an overlay file loaded only when MILVUS_ENDPOINT is set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
distribution/run.yaml(2 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-test-push (linux/amd64)
🔇 Additional comments (1)
distribution/run.yaml (1)
13-13: Files API surfaced — LGTM.Addition is consistent with the existing providers.files block later in the file. No issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
distribution/build.yaml (1)
11-11: Add pinned pymilvus to distribution/build.yaml additional_pip_packagesrun.yaml registers remote::milvus (distribution/run.yaml:49) and the Containerfile already installs pymilvus (distribution/Containerfile:30), but distribution/build.yaml’s additional_pip_packages does not — add a pinned pymilvus entry.
additional_pip_packages: - aiosqlite - sqlalchemy[asyncio] +- pymilvus>=2.4,<3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
distribution/build.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-test-push (linux/amd64)
So that the following metadata can be stored on Postgres: * Agent persistence * Agent responses * Metadata store Relates to: RHAIENG-731 Signed-off-by: Sébastien Han <seb@redhat.com>
Original LMEval provider issue: https://issues.redhat.com/browse/RHOAIENG-32731 Bump LMEval provider version from 0.2.1 to 0.2.3. Relates to: RHAIENG-696 Signed-off-by: Sébastien Han <seb@redhat.com>
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
When not running on kubernetes we can set TRUSTYAI_LMEVAL_USE_K8S=false to avoid loading the provider. Relates to: RHAIENG-1024 Signed-off-by: Sébastien Han <seb@redhat.com>
|
@coderabbitai ignore |
|
Note Reviews pausedUse the following commands to manage reviews:
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error detailsPre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
distribution/build.yaml (1)
39-40: Confirm need for both asyncpg and psycopg2-binaryCarrying both async and sync PG drivers is fine if both paths are used; otherwise, drop the unused one to slim the image.
distribution/run.yaml (1)
137-144: Files provider paths — ensure dirs exist in imageStorage dir and sqlite file directories must exist at runtime. The Containerfile change to mkdir these paths (see suggestion there) will prevent first-run failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
distribution/Containerfile(2 hunks)distribution/build.yaml(2 hunks)distribution/run.yaml(3 hunks)tests/smoke.sh(0 hunks)
💤 Files with no reviewable changes (1)
- tests/smoke.sh
⏰ 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-test-push (linux/amd64)
🔇 Additional comments (9)
distribution/build.yaml (4)
11-11: LGTM: remote Milvus provider added to vector_ioThis aligns with the new remote Milvus support in run.yaml. No concerns.
18-18: Pin lmeval module to 0.2.3 — good and consistentMatches the Containerfile pin. Keep provider and core stack versions in lockstep going forward.
33-34: Files API provider declared — OKInline localfs added to providers. This matches the new “files” API in run.yaml.
35-35: Python base mismatch between build spec (3.11) and Containerfile (3.12)Align container_image here with the 3.12 base used in the Containerfile to avoid ABI/runtime drift.
distribution/Containerfile (3)
10-10: LGTM: asyncpg installationRequired for async Postgres paths. No issues.
41-42: LGTM: lmeval pinned to 0.2.3 in a dedicated stepConsistent with build.yaml. Good.
7-8: Double SQLAlchemy install — keep or simplifyIf the plain sqlalchemy install is still required due to extras resolution quirks, keep it; otherwise, remove the first line to save one layer.
distribution/run.yaml (2)
13-13: LGTM: expose the new “files” APIAPI surface updated accordingly.
48-61: Remote Milvus provider — set explicit boolean default forsecure; verify conditional provider blockFile: distribution/run.yaml (lines 48–61)
- Set
secureto an explicit boolean default to avoid type/parsing issues:- secure: ${env.MILVUS_SECURE:=} + secure: ${env.MILVUS_SECURE:=false}
- Verify the templater behavior for
provider_id: ${env.MILVUS_ENDPOINT:+milvus-remote}— ensure the entire provider block is removed whenMILVUS_ENDPOINTis unset; if it is not pruned, gate the block or use a safe defaultprovider_idso server initialization cannot fail.
distribution/Containerfile
Outdated
| RUN pip install \ | ||
| llama_stack_provider_lmeval==0.2.3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Create directories used by the new Files API and DB paths
run.yaml writes under /opt/app-root/src/.llama/distributions/rh (and files/). Pre-create to avoid runtime failures on first boot.
Apply after the existing mkdir step:
RUN pip install --no-cache llama-stack==0.2.21
-RUN mkdir -p ${HOME}/.llama/providers.d ${HOME}/.cache
+RUN mkdir -p ${HOME}/.llama/providers.d ${HOME}/.cache \
+ && mkdir -p /opt/app-root/src/.llama/distributions/rh/filesCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In distribution/Containerfile around lines 41 to 42, the Dockerfile installs
llama_stack_provider_lmeval but does not pre-create directories used by the new
Files API and DB paths; add a step (immediately after the existing mkdir step)
to create /opt/app-root/src/.llama/distributions/rh and
/opt/app-root/src/.llama/files (or the exact files/ path used by run.yaml), set
appropriate ownership (app-root user) and permissions so the runtime process can
write there, and ensure the mkdir command uses -p to avoid errors.
| provider_type: remote::trustyai_lmeval | ||
| config: | ||
| use_k8s: True | ||
| use_k8s: ${env.TRUSTYAI_LMEVAL_USE_K8S:=true} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: env var name and default for use_k8s
PR text references LM_EVAL_USE_K8S=false, but config reads TRUSTYAI_LMEVAL_USE_K8S with default true. With kubernetes lib not installed in the image, default true can cause runtime errors.
Apply one of the following (preferred: align with PR doc and default to false):
- use_k8s: ${env.TRUSTYAI_LMEVAL_USE_K8S:=true}
+ use_k8s: ${env.LM_EVAL_USE_K8S:=false}Run to confirm naming and defaults across the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Containerfile base:"
rg -n '^FROM .*python-(3\d+)' -n distribution/Containerfile
echo
echo "build.yaml container_image:"
rg -n 'container_image:' -n distribution/build.yaml
echo
echo "Env var occurrences:"
rg -n 'LM_EVAL_USE_K8S|TRUSTYAI_LMEVAL_USE_K8S' -nLength of output: 403
Critical: env var name and default for use_k8s
distribution/run.yaml defaults TRUSTYAI_LMEVAL_USE_K8S to true (distribution/run.yaml:84); tests/smoke.sh sets TRUSTYAI_LMEVAL_USE_K8S=False (tests/smoke.sh:14). LM_EVAL_USE_K8S is not present. Default=true can cause runtime errors when k8s libs are not in the image. Apply one of the following (preferred: align with PR doc and default to false):
- use_k8s: ${env.TRUSTYAI_LMEVAL_USE_K8S:=true}
+ use_k8s: ${env.LM_EVAL_USE_K8S:=false}Files: distribution/run.yaml:84, tests/smoke.sh:14
📝 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.
| use_k8s: ${env.TRUSTYAI_LMEVAL_USE_K8S:=true} | |
| use_k8s: ${env.LM_EVAL_USE_K8S:=false} |
🤖 Prompt for AI Agents
In distribution/run.yaml around line 84, the environment variable and default
are wrong: change the key to use LM_EVAL_USE_K8S (matching the PR doc) and set
the default to false instead of true; ensure tests/smoke.sh (line ~14) and any
other refs use the same variable name and casing (or update the test to export
LM_EVAL_USE_K8S=False) so runtime defaults do not enable k8s by default.
✅ Actions performedReviews paused. |
We now use the "module" field to pull the dependencies. This also bumps trusty deps. Relates to: RHAIENG-498 Signed-off-by: Sébastien Han <seb@redhat.com>
leseb
changed the title
nathan-weinberg
left a comment
nathan-weinberg
left a comment
There was a problem hiding this comment.
LGTM and image smoke test is passing 👍
chore: bump LLS container to release 3.2.762
2 participants
358fe45 chore: add files API to the definition
b770d1a feat: add milvus remote provider
d576cb0 chore: add milvus remote to the build config
7e76164 feat: add files localfs provider
cbbc46c feat: add postgres sql dependencies
6c5211b chore: bump lmeval version
7674ef8 fix: allow trustyai lmeval to be disabled
c16eebb chore: convert build.yaml to use modules for external providers
commit 358fe45
Author: Sébastien Han seb@redhat.com
Date: Fri Sep 12 09:48:14 2025 +0200
commit b770d1a
Author: Sébastien Han seb@redhat.com
Date: Fri Sep 12 09:49:30 2025 +0200
commit d576cb0
Author: Sébastien Han seb@redhat.com
Date: Fri Sep 12 09:53:56 2025 +0200
commit 7e76164
Author: Sébastien Han seb@redhat.com
Date: Fri Sep 12 09:56:04 2025 +0200
commit cbbc46c
Author: Sébastien Han seb@redhat.com
Date: Fri Sep 12 09:58:21 2025 +0200
commit 6c5211b
Author: Sébastien Han seb@redhat.com
Date: Fri Sep 12 10:00:54 2025 +0200
commit 7674ef8
Author: Sébastien Han seb@redhat.com
Date: Fri Sep 12 10:24:08 2025 +0200
commit c16eebb
Author: Sébastien Han seb@redhat.com
Date: Fri Sep 12 10:49:26 2025 +0200
@coderabbitai ignore