ci: reorganize mesh-llm builds for 50% speedups using warm build caches - #209
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to speed up CI by removing an allegedly redundant release build step and making cargo fmt --check an early, blocking failure gate, alongside rustfmt-only cleanup in a few Rust sources.
Changes:
- Move
cargo fmt --checkearlier in Linux/macOS CI and make it blocking (remove|| true). - Remove the explicit
cargo build --release -p mesh-llm --bin mesh-llmstep from Linux/macOS CI jobs. - Apply rustfmt output to several Rust files previously masked by non-blocking formatting.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Reorders CI to fail fast on formatting and removes a dedicated release build step. |
mesh-llm/src/protocol/mod.rs |
rustfmt-only formatting in protocol tests. |
mesh-llm/src/mesh/mod.rs |
rustfmt-only formatting in config apply path. |
mesh-llm/src/lib.rs |
rustfmt-only formatting. |
mesh-llm/src/inference/launch.rs |
rustfmt-only formatting in warning text + tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Good catch on both threads — you were right that |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Final scoreboardPushed all four levers, validated against baseline run
PR runs now go green in half the time. Levers applied (commit-by-commit)
Cache warm-up gotchaBoth Lever 1 (cuda_arch change) and Lever 4 (rust-cache workspace fix) invalidated their respective cache namespaces. The first 1-2 runs after either commit were SLOWER than baseline because everything had to cold-populate. Run #3 hit steady state with sccache 99.81% hit rate on CUDA. If you ever change Carrack experiment summary
Carrack workspace cleaned up after measurement. What's NOT in this PR (deferred)
The PR is shippable as-is. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up: Debug profile lever — macOS down to 5m 34s (-49%)Previous scoreboard had macOS at 10m 51s as the new bottleneck. Investigated locally on Apple hardware (M4 Pro, isolated CARGO_HOME) following the same carrack methodology. Root cause foundThe macOS job (and linux) did two full Rust compiles per run:
Two separate profiles = separate Local experiments (M4 Pro, isolated CARGO_HOME, measured via
|
| Exp | Command | Wall | User |
|---|---|---|---|
| E1 | cold cargo build --release -p mesh-llm --bin mesh-llm |
1m 45s | 676s |
| E3 | warm release after touch lib.rs |
43s | 251s |
| E4 | warm debug test --no-run after E1 |
1m 01s | 272s |
| E5 | warm test --release --no-run after E1 |
1m 07s | 580s |
| E6a | cold debug build -p mesh-llm --bin mesh-llm |
52s | 219s |
| E6b | warm debug test after E6a | 40s | 154s |
| E7 | warm debug after touch lib.rs |
4.65s | 3s |
E5 rules out release-mode tests (dead end: 2x user CPU). E7 shows debug rebuild is ~10x faster than release rebuild on lib edits — because debug has incremental = true while release has incremental = false.
Fix (commit 95d951c)
Build both steps in dev/debug profile so they share one target/ subdir. Integration tests now exec target/debug/mesh-llm. mesh-llm is a thin orchestrator around llama-server (hot loop is inside llama-server C++, still built optimized), so debug vs release binary perf is negligible for smoke tests. CUDA/ROCm/Vulkan jobs keep release — they validate backend compilation for the release pipeline.
Gotcha: GitHub Actions cache scoping
First run after the commit showed a REGRESSION (+162s linux, +90s macos). Root cause: pull_request runs cache under refs/pull/209/merge but the first post-commit run was cold. Subsequent warm run now shows true numbers. (Same pattern as Lever 1/4 — documented for future readers.)
Warm-case results (run 24095554088)
| Job | Baseline (b158a1d) | New warm | Δ |
|---|---|---|---|
| macos | 10m 51s | 5m 34s | -5m 17s (-49%) |
| linux | 9m 09s | 6m 18s | -2m 51s (-31%) |
| Linux CUDA | 9m 38s | 10m 03s | +25s (noise) |
| Linux ROCm | 9m 30s | 9m 40s | +10s (noise) |
| Linux Vulkan | 6m 00s | 5m 58s | noise |
| Workflow wall clock | 10m 51s | 10m 03s | -48s (-7%) |
macOS step detail
| Step | Baseline | Warm | Δ |
|---|---|---|---|
| rust-cache restore | 27s | 13s | -14s |
| Build mesh-llm | 214s | 38s | -176s (-82%) |
| Unit tests | 169s | 99s | -70s (-41%) |
Linux step detail
| Step | Baseline | Warm | Δ |
|---|---|---|---|
| rust-cache restore | 21s | 18s | -3s |
| Build mesh-llm | 146s | 32s | -114s (-78%) |
| Unit tests | 115s | 113s | -2s (test execution dominates) |
Cumulative progress on PR #209
| Run | macos | linux | Linux CUDA | Wall clock |
|---|---|---|---|---|
| main baseline (2075bf7) | 18m 45s | 16m 23s | 21m 42s | 21m 42s |
| PR levers 1-4 (b158a1d) | 10m 51s | 9m 09s | 9m 38s | 10m 51s |
| PR + debug (95d951c) | 5m 34s | 6m 18s | 10m 03s | 10m 03s |
| vs main | -70% | -62% | -54% | -54% |
Next bottleneck
The workflow wall clock is now bound by Linux CUDA at 10m 03s. macOS is no longer the critical path. Attacking CUDA further would likely require changing scripts/build-linux.sh (Rust build profile inside the CUDA container) or splitting CUDA into parallel jobs — deferring to a follow-up since the main user-visible PRs already run well under 11 minutes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6a0e7ae to
24670cd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
783d67f to
28c8c72
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bortlesboat
left a comment
There was a problem hiding this comment.
The cache workspace fix and format-check changes are correct. One thing to check: the crate has tests in and that were previously covered by the workspace-wide . The new flag drops those.
|
@ndizazzo any risks - in the past I have been bitten by testing debug vs release but as long as release bin is tested at least once? |
fef3920 to
1ce4439
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@michaelneale Low risk, but not zero. PR CI intentionally uses debug for speed + incremental, and I think we're covered via the release workflow, where release-profile binaries are built and validated. As long as we still exercise the release binary in the release path at least once, I’m fairly comfortable with this change. I'm also thinking that our docker builds can run on main with a release path, so that's another angle we cover (once #220 gets finalized) |
|
@michaelneale making an executive decision to get this in. I was blocked on MLX I didn't want to conflict with that work, but we're losing the efficiency this brings. I'll pull changes into the MLX branch and update for compatibility. |
89091ca to
c06ab04
Compare
Moves the PR-side Linux CUDA job onto the new main-warmed cache flow. The job now restores the main-scope llama.cpp CUDA artifact cache with a key that matches warm-caches.yml, falls back to a full cold build when the key changes, and keeps the FA/cuda key dimensions aligned with the warmed artifact. This preserves cold-path validation for cache-input changes while letting ordinary PRs skip the expensive llama.cpp rebuild once main has already warmed the cache.
c06ab04 to
2853168
Compare
Summary
cargo test --releaseand thencargo build --release -p mesh-llm --bin mesh-llmback-to-back. The second step is redundant:cargo test --releasealready compilestarget/release/mesh-llmas part of building the workspace, and every downstream integration test (ci-smoke-test.sh,ci-split-test.sh,ci-moe-mesh-test.sh,ci-client-auto-test.sh, CLI smoke) already consumes that same binary.cargo fmt --checkto an early-fail gate, before unit tests and the llama.cpp build. Formatting issues now fail the job in seconds instead of ~15 minutes in. The|| truesuffix is also removed so the check actually blocks.|| true, in a separate style-only commit. Surfacing these is the point of making the fmt check blocking.Why the duplicate build is safe to remove
cargo test --releaseat the workspace root compiles every[[bin]]target of every workspace member in release mode (it has to, in order to build and link the test harnesses against the same crate). After that step,target/release/mesh-llmexists and is byte-for-byte the binary the subsequent tests were already using. The oldcargo build --release -p mesh-llm --bin mesh-llmstep was a full second-compile pass that accomplished nothing new.Rust caching — findings (no change in this PR)
The user asked whether there is an sccache-equivalent for Rust that we could adopt. Current state:
Swatinem/rust-cache@v2is wired up and caches the Rusttarget/dir + registry. This is the primary Rust cache today.mozilla-actions/sccache-action@v0.0.9is installed andSCCACHE_GHA_ENABLED: "true"is set, butRUSTC_WRAPPER: sccacheis not set, andCARGO_INCREMENTALis not pinned. So sccache currently only accelerates the llama.cpp C/C++ build via-DCMAKE_C_COMPILER_LAUNCHER=sccache. It does not cache any Rust compilation.Enabling sccache for Rust is possible (
RUSTC_WRAPPER: sccache+CARGO_INCREMENTAL: 0) but interacts non-trivially with Swatinem/rust-cache — they cache at different layers and combining them can either compound or fight each other depending on the workload. I did not change this in this PR because (a) it is risky and should be measured, and (b) this PR's goal is to eliminate an obvious 7 minute duplicate. Worth a follow-up PR that A/B tests the two caching strategies.Validation
Locally on this branch:
```
$ cargo fmt --manifest-path mesh-llm/Cargo.toml -- --check
(clean)
```
The four files touched in `83907bf` are pure whitespace/line-wrapping changes produced by `cargo fmt` and contain no behavioral changes.