Add a justfile for build & test automation - #33
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
|
| Layer / File(s) | Summary |
|---|---|
Justfile setup and build tasks justfile |
Adds global configuration, cleanup rules, and build-rust/build-python/build-all tasks. |
Justfile docs and test tasks justfile, docs/package.json, .gitignore |
Adds docs, test-python, test-rust, and test-all tasks, plus the docs package manifest and ignore rule they use. |
CI workflow integration with just .github/workflows/ci_python.yml, .github/workflows/ci_rust.yml, .github/workflows/fern-docs.yml |
CI installs just and switches test/doc execution to just test-python, just test-rust, and just docs. |
Documentation updates for just-based setup README.md, docs/getting-started/overview.mdx, examples/README.md |
Setup and usage instructions now describe just build-all, just build-python, and just build-rust instead of manual install commands. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Suggested labels: documentation
Important
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❌ Error | The title is descriptive, but it does not follow the required Conventional Commits format. | Rewrite it as a conventional commit, e.g. chore: add justfile for build and test automation. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Comment @coderabbitai help to get the list of available commands.
|
📖 Fern docs preview: https://nvidia-preview-pull-request-33.docs.buildwithfern.com/nemo/fabric |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/fern-docs.yml:
- Around line 57-58: The Fern CLI install step is using an ad-hoc global npm
install, which static analysis flags as outside lockfile management. Update the
workflow step that installs fern-api in the fern-docs job to use a
lockfile-backed package.json/package-lock.json workflow instead of npm install
-g, or otherwise route the install through a pinned, reproducible dependency
management step so the CLI version remains controlled by the repository.
In `@justfile`:
- Line 9: The shared no_uv global toggle should be replaced with a per-recipe
parameter so it is easier to override and more discoverable. Update the recipes
that branch on this flag, including build-python, and the other affected
recipes, to accept a no_uv default parameter instead of reading the global
variable. Keep the existing behavior by defaulting the parameter to false and
wiring the conditionals to the recipe-local value.
- Around line 33-60: The shebang-based recipes do not inherit the global strict
Bash flags, so `build-python`, `docs`, and `test-python` can continue after a
failed command and mask the real failure. Update each Bash recipe in the
justfile to enable strict mode at the start of the script so failures in `uv
sync`, `uv run`, or related commands stop execution immediately; use the recipe
names `build-python`, `docs`, and `test-python` to locate the affected blocks.
In `@README.md`:
- Around line 63-66: Add the missing blank line before the fenced code block in
the README section that says “Install `just` if not already installed.” so the
Markdown satisfies MD031; keep the surrounding text the same and adjust only the
spacing around the fenced block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 004a7159-20b2-4646-9259-66005b009738
📒 Files selected for processing (7)
.github/workflows/ci_python.yml.github/workflows/ci_rust.yml.github/workflows/fern-docs.ymlREADME.mddocs/getting-started/overview.mdxexamples/README.mdjustfile
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Build and publish docs
- GitHub Check: Test
⚠️ CI failures not shown inline (3)
GitHub Actions: Rust / Test: Add a justfile for build & test automation
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / 0_Test.txt: Add a justfile for build & test automation
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: Add a justfile for build & test automation
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 6.18s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (2)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/README.md
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
docs/getting-started/overview.mdxREADME.md
🪛 markdownlint-cli2 (0.22.1)
README.md
[warning] 64-64: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 zizmor (1.26.1)
.github/workflows/fern-docs.yml
[warning] 58-58: ad-hoc installation of packages (adhoc-packages): installs a package outside of a lockfile
(adhoc-packages)
🔇 Additional comments (14)
justfile (3)
12-25: LGTM!
28-30: LGTM!Also applies to: 63-64
42-42: LGTM!Also applies to: 67-67
.github/workflows/ci_python.yml (3)
46-50: LGTM!
65-94: 🎯 Functional CorrectnessVerify
just test-pythonparity with the manual venv/pytest flow.
uv venv --python 3.12 .venv(line 58) explicitly creates a project venv used by the dependency-free smokes (.venv/bin/python, lines 68-89). Line 94 now delegates tojust test-pythoninstead of the previousuv run pytest. Confirm the justfile'stest-pythonrecipe uses/activates this same.venv(or an equivalent) rather than creating a separate environment, and that it preserves whatever pytest args/markers the prior invocation passed.
54-59: 🩺 Stability & AvailabilityNo PyYAML install is missing here.
pyyamlis already included in thetestdependency group, anduv sync --group test --no-group dev --extra harbor --extra hermes --extra relaystill pulls it in fortests/smoke_hermes_config_mapping.py.> Likely an incorrect or invalid review comment..github/workflows/ci_rust.yml (2)
36-40: LGTM!
59-62: 🎯 Functional CorrectnessNo issue:
test-rustalready usescargo test --workspace --locked, so CI still enforcesCargo.lock.> Likely an incorrect or invalid review comment..github/workflows/fern-docs.yml (2)
34-38: LGTM!
39-63: 🎯 Functional CorrectnessVerify
just docsreproduces the prior generation + validation steps.The old flow separately ran
uv sync, a Rust reference generation script, thenfern check; this is now condensed into a singlejust docscall (line 63), whileSet up uv(lines 39-40) only installs theuvbinary with no visibleuv syncbeforejust docs. Confirm the justfile'sdocsrecipe performs the equivalentuv sync, Python docstring extraction, rustdoc-to-MDX generation, and Fern config validation that the removed steps did, so doc generation/validation coverage doesn't silently regress.README.md (2)
318-323: LGTM!
60-75: 🎯 Functional CorrectnessNo issue:
build-allalready installsfabricinto~/.cargo/bin
build-allchainsbuild-rustandbuild-python;build-rustrunscargo install --path crates/fabric-cli --locked --force, so the README’sexport PATH="$HOME/.cargo/bin:$PATH"matches the justfile.> Likely an incorrect or invalid review comment.docs/getting-started/overview.mdx (1)
67-82: 🎯 Functional CorrectnessSame
just build-allPATH assumption as README.md.This mirrors README.md's Quick Start change and depends on the same unverified assumption that
just build-allinstalls thefabricbinary into$HOME/.cargo/bin. If that assumption doesn't hold, thefabric plan/doctor/runcommands shown right after (lines 86-92) will fail with "command not found." See the verification script in the README.md review comment for the same justfile check.Source: Path instructions
examples/README.md (1)
22-29: 🎯 Functional CorrectnessNo issue:
just build-rustalready installsfabricviacargo install --path crates/fabric-cli --locked --force, soexport PATH="$HOME/.cargo/bin:$PATH"is enough for the barefabric validate/inspect/plancommands.> Likely an incorrect or invalid review comment.
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
justfile (1)
33-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing
set -euo pipefailin shebang recipes still applies.
build-python,docs, andtest-pythonuse#!/usr/bin/env bashshebangs, which don't inherit the globalset shell := ["bash", "-eu", "-o", "pipefail", "-c"]. A faileduv sync/uv pip installwon't halt the recipe, lettingdocsrun doc generation ortest-pythonrun pytest against a stale/broken environment.🐛 Proposed fix
build-python: #!/usr/bin/env bash + set -euo pipefail if [[ "{{ no_uv }}" == "true" ]]; then uv pip install --python .venv/bin/python --no-deps --reinstall --editable . else uv sync --no-default-groups --reinstall-package nemo-fabric fi docs: #!/usr/bin/env bash + set -euo pipefail if [[ "{{ no_uv }}" != "true" ]]; then uv sync --extra docs fiAlso applies to: 46-54, 56-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@justfile` around lines 33 - 40, The shebang-based recipes still bypass the global shell flags, so failures in build-python, docs, and test-python can be ignored. Update the bash recipes that use #!/usr/bin/env bash to explicitly enable strict mode at the top so uv sync/uv pip install failures stop execution before docs generation or pytest continues, and make the same change in the affected recipe blocks identified by build-python, docs, and test-python.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@justfile`:
- Around line 33-40: The shebang-based recipes still bypass the global shell
flags, so failures in build-python, docs, and test-python can be ignored. Update
the bash recipes that use #!/usr/bin/env bash to explicitly enable strict mode
at the top so uv sync/uv pip install failures stop execution before docs
generation or pytest continues, and make the same change in the affected recipe
blocks identified by build-python, docs, and test-python.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b26ca940-022b-480d-8e51-6e9219731317
📒 Files selected for processing (1)
justfile
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Build and publish docs
- GitHub Check: Test
⚠️ CI failures not shown inline (3)
GitHub Actions: Rust / Test: Add a justfile for build & test automation
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / 0_Test.txt: Add a justfile for build & test automation
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: Add a justfile for build & test automation
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.66s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🔇 Additional comments (3)
justfile (3)
33-33:no_uvglobal-toggle ergonomics concern still applies.Same discoverability issue as previously flagged:
no_uvis a global variable only settable viajust --set no_uv true <recipe>rather than a per-recipe default parameter, despite the new# --set [no_uv=true|false]comment hints added here.Also applies to: 46-46, 57-57
28-30: LGTM!
43-43: LGTM!
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
justfile (1)
48-57: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing
set -euo pipefailindocsrecipe; verifynpm cilockfile requirement.Same strict-mode gap flagged previously: this shebang recipe still lacks
set -euo pipefail, so a faileduv sync --extra docsornpm ciwon't stop the recipe.Separately,
npm ci --prefix docs --ignore-scripts(line 53) requires an existingdocs/package-lock.json. None is visible among the reviewed files — if it's missing, this step (and the CIjust docscall) will fail outright.🐛 Proposed fix for strict mode
docs: #!/usr/bin/env bash + set -euo pipefail if [[ "{{ no_uv }}" != "true" ]]; then uv sync --extra docs fiPlease run to confirm the lockfile exists:
#!/bin/bash fd -p 'docs/package-lock.json'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@justfile` around lines 48 - 57, The docs recipe in justfile is missing strict shell mode, so failures in steps like uv sync or npm ci may be ignored. Update the docs recipe to enable set -euo pipefail at the start of the shell block, using the docs recipe as the locator. Also verify that docs/package-lock.json exists for the npm ci --prefix docs --ignore-scripts step; if it is absent, add or commit the lockfile so the docs recipe can run reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@justfile`:
- Around line 48-57: The docs recipe in justfile is missing strict shell mode,
so failures in steps like uv sync or npm ci may be ignored. Update the docs
recipe to enable set -euo pipefail at the start of the shell block, using the
docs recipe as the locator. Also verify that docs/package-lock.json exists for
the npm ci --prefix docs --ignore-scripts step; if it is absent, add or commit
the lockfile so the docs recipe can run reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a4b51c40-b4f2-413c-a009-cd8c320aa5c5
⛔ Files ignored due to path filters (1)
docs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/fern-docs.yml.gitignoredocs/package.jsonjustfile
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Test
- GitHub Check: Build and publish docs
⚠️ CI failures not shown inline (3)
GitHub Actions: Rust / Test: Add a justfile for build & test automation
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / 0_Test.txt: Add a justfile for build & test automation
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: Add a justfile for build & test automation
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.77s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (1)
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
docs/package.json
🔇 Additional comments (6)
justfile (1)
12-27: Missingset -euo pipefailincleanshebang recipe.The global
set shell := [...]strict flags don't apply to#!/usr/bin/env bashshebang recipes, socleancan silently continue past a failedcargo clean..github/workflows/fern-docs.yml (3)
50-65: Ad-hoc npm install concern resolved.Moving
fern-apiintodocs/package.json+npm ciaddresses the previously flagged zizmor ad-hoc-install warning. This is contingent ondocs/package-lock.jsonexisting (see justfile comment).
34-38: LGTM!
76-76: LGTM!docs/package.json (1)
1-8: LGTM!.gitignore (1)
18-18: LGTM!
Signed-off-by: David Gardner <dagardner@nvidia.com>
2 participants
justjustno_uvavoids commands that would modify the user's local venvSummary by CodeRabbit
New Features
just-based automation workflow for building, testing, cleanup, and docs across Rust and Python.CI / Testing
just.just docs(including Fern generation vianpx).Documentation
just(v1.50.0+) and usejust build-*steps.Chores
docs/node_modulesand added docs-specificpackage.jsonconfiguration.