chore: clarify README.md#45
Conversation
leseb
requested review from
Elbehery,
VaishnaviHire,
cdoern,
derekhiggins,
nathan-weinberg,
rhdedgar,
rhuss and
skamenan7
as code owners
WalkthroughREADME.md now requires only pre-commit and replaces prior Containerfile generation and Podman build steps with a pre-commit-driven flow that creates a venv, installs dependencies, runs the distribution build (which invokes the llama CLI), and generates the Containerfile from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant PC as pre-commit
participant Venv as VirtualEnv
participant Build as distribution/build.py
participant Llama as llama CLI
participant Tmpl as distribution/Containerfile.in
participant CF as distribution/Containerfile (generated)
Dev->>PC: pre-commit run --all-files
PC->>Venv: create/activate venv
PC->>Venv: install dependencies (llama-stack, etc.)
PC->>Build: execute distribution build script
Build->>Llama: invoke llama CLI to generate deps
Build->>Tmpl: read template
Build->>CF: render/generate Containerfile
note over CF: Do not edit generated Containerfile\nEdit template and rerun pre-commit
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![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 (5)
README.md (5)
17-19: Add a language to the fenced code block to satisfy markdownlint (MD040).Use bash for the command snippet.
-``` +```bash pre-commit run --all-files--- `11-11`: **Provide quick install examples for pre-commit.** Small UX boost for first‑time contributors. ```diff - The `pre-commit` tool is [installed](https://pre-commit.com/#install) + - The `pre-commit` tool is [installed](https://pre-commit.com/#install) + For example: + - macOS: `brew install pre-commit` + - pipx: `pipx install pre-commit` + - pip: `python3 -m pip install --user pre-commit`
22-24: Clarify where dependencies come from (build.yaml) and that the venv is isolated.Keeps this consistent with how build.py sources config.
-- Install the dependencies (llama-stack etc) in a virtual environment -- Execute the build script `./distribution/build.py` +- Create an isolated virtual environment and install dependencies for the configured providers from `distribution/build.yaml` +- Run `./distribution/build.py`
25-27: Be explicit about inputs/outputs of the build script.Aligns with the repo convention that Containerfile is generated from build.yaml and the template.
The build script will: -- Execute the `llama` CLI to generate the dependencies -- Create a new `Containerfile` with the required dependencies +- Invoke the `llama` CLI to resolve provider dependencies defined in `distribution/build.yaml` +- Generate `distribution/Containerfile` from `distribution/Containerfile.in` with the required dependencies
31-31: Name the exact command to re-run.-The Containerfile is auto-generated from a template. To edit it, you can modify the template in `distribution/Containerfile.in` and run pre-commit again. +The Containerfile is auto-generated from a template. To edit it, modify `distribution/Containerfile.in` and run `pre-commit run --all-files` again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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:
README.md
🪛 markdownlint-cli2 (0.17.2)
README.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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)
README.md (1)
33-34: Good, explicit warning about not editing the generated Containerfile.This matches the repo’s practice that
distribution/Containerfileis auto-generated bydistribution/build.pyfrom config.
|
We shouldn't need to run the Build test if we edit the README :) |
|
@Mergifyio rebase |
|
|
@Mergifyio rebase |
Update the instructions to generate a new container file. Signed-off-by: Sébastien Han <seb@redhat.com>
✅ Branch has been successfully rebased |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (4)
22-24: Tiny wording tweak to avoid ambiguity about environmentsClarify this is an isolated pre-commit env (doesn’t touch system Python).
-- Install the dependencies (llama-stack etc) in a virtual environment +- Install dependencies in an isolated pre-commit–managed virtual environment (no changes to your system Python) - Execute the build script `./distribution/build.py`
25-26: Note where dependencies are sourced to prevent confusionReaders may try to edit the template for deps. Mention build.yaml as the source of provider/dependency inputs (per repository workflow).
The build script will: - Execute the `llama` CLI to generate the dependencies +- Read provider/dependency configuration from `distribution/build.yaml` - Create a new `Containerfile` with the required dependencies
31-31: Clarify when to edit build.yaml vs. the templateAvoids users changing the template when they only need provider/deps changes.
-The Containerfile is auto-generated from a template. To edit it, you can modify the template in `distribution/Containerfile.in` and run pre-commit again. +The Containerfile is auto-generated from a template. For dependency/provider changes, edit `distribution/build.yaml`; for base image/structure tweaks, edit `distribution/Containerfile.in`, then run pre-commit again.
17-19: Add a language to the fenced code block (markdownlint MD040)Specify a language to satisfy MD040 and improve rendering.
-``` +```bash pre-commit run --all-filesVerification: .pre-commit-config.yaml includes a local hook "pkg-gen" (entry: ./distribution/build.py) with always_run: true — the distribution build hook will run on README commits. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 41fedfae077be69cdf7c1940239886419f729fe8 and 2ecee2c011e7de486dda9b25049a04c9be968023. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `README.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (2)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: nathan-weinberg
PR: #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.</details> <details> <summary>📚 Learning: 2025-09-15T14:25:54.837Z</summary>Learnt from: nathan-weinberg
PR: #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:** - `README.md` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>README.md</summary> 17-17: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: build-test-push (linux/amd64) * GitHub Check: Summary </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>README.md (2)</summary><blockquote> `11-11`: **Prerequisite simplification looks good** Pre-commit as the only requirement is clear and reduces friction. --- `33-34`: **Strong warning is appropriate** Clear do-not-edit guidance on the generated file. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@nathan-weinberg Mergify must be sad now :-P |
4 participants
What does this PR do?
Update the instructions to generate a new container file.
Summary by CodeRabbit