feature: add benchmark 'tune' option - #948
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a ChangesGPU Benchmark Tune Command
Native MTP speculative decoding and mmap/mlock runtime options
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as mesh-llm CLI
participant Runner as tune_runner
participant Resolver as tune_resolver
participant Bench as benchmark::run_benchmark_plans
participant Apply as tune_apply
CLI->>Runner: run_benchmark_tune_command
Runner->>Resolver: resolve targets
Runner->>Runner: build_tune_plan per target
Runner->>Bench: run_benchmark_plans (if requested)
Bench-->>Runner: TuneBenchmarkTargetReport
Runner->>Apply: apply_prepared_tune_plans (apply mode)
Runner->>CLI: emit_tune_output
sequenceDiagram
participant Backend as StageOpenAiBackend
participant Runtime as skippy-runtime
participant Verifier as NativeMtpVerifier
Backend->>Runtime: decode_sampled_mtp / decode_step_sampled_mtp
Runtime-->>Backend: predicted token + NativeMtpDraft(tokens)
Backend->>Verifier: observe_taken_draft_span(draft_tokens, target_tokens)
Verifier-->>Backend: NativeMtpSpanVerification (accepted_count, decision)
Backend->>Backend: commit accepted tokens, adopt next draft
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs (1)
97-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate against the resolved max tokens
draft_min_tokensis checked against the rawdraft_max_tokensbefore the native-MTP default of3is applied. Whennative_mtp_enabledanddraft_max_tokensis unset,draft_min_tokens > 3can still resolve intonative_mtp_min_tokens > native_mtp_max_tokensdownstream. Move the guard to use the resolved max (or compute it once and reuse it).🤖 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 `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs` around lines 97 - 114, The speculative config validation in `speculative.rs` is comparing `draft_min_tokens` against the raw `draft_max_tokens`, which misses the native-MTP default path. Update the guard in the resolver logic so it uses the resolved max token value (including the native-MTP default of 3 when `native_mtp_enabled` applies), or compute the resolved max once and reuse it for validation and downstream setup. Keep the check near the existing `normalize_pairing_fault`, `explicit`, and `draft_min_tokens`/`draft_max_tokens` validation flow.crates/mesh-llm-host-runtime/src/mesh/mod.rs (1)
9247-9297: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile continues to grow well past the 2,000-line limit.
This file is already over 9,000 lines; adding more functions/fields here compounds the size problem. Per coding guidelines, new logic (including proto (de)serialization helpers like
stage_load_to_proto/stage_load_from_proto) should be split into an owning module instead of growing this file further.As per coding guidelines, "Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."
Also applies to: 9416-9468
🤖 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 `@crates/mesh-llm-host-runtime/src/mesh/mod.rs` around lines 9247 - 9297, The issue is that new proto serialization helpers are being added to an already oversized mesh module, pushing the file further past the 2,000-line guideline. Move stage_load_to_proto and the related stage_load_from_proto logic, along with any closely related StageLoadRequest conversion helpers, into a dedicated owning module and keep the mesh/mod.rs wrapper minimal by delegating to that module. Ensure the new module owns the skippy_stage_proto conversion code so this file stops accumulating more responsibility.Source: Coding guidelines
crates/mesh-llm-commands/src/gpus/tune/benchmark.rs (1)
1-2000: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFile has reached the 2,000-line guideline limit — split the embedded test module out.
This file's
#[cfg(test)] mod benchmark_testsblock alone spans roughly 826 lines (1174-1999), pushing the whole file to ~2000 lines. This PR already establishes the convention of separate*_tests.rsfiles for this exact module (e.g.apply_collision_tests.rs,apply_write_tests.rs). Movingmod benchmark_testsinto a siblingbenchmark_tests.rs(included via#[path] mod benchmark_tests;orinclude!, consistent with the existing pattern) keeps this file under the limit and matches the established layout.As per coding guidelines, "Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs` around lines 1 - 2000, The benchmark module file is over the 2,000-line limit because the embedded #[cfg(test)] mod benchmark_tests block dominates the file. Move benchmark_tests, including all fixtures and unit tests, into a sibling benchmark_tests.rs using the same split-module pattern already used by apply_collision_tests.rs and apply_write_tests.rs, and leave benchmark.rs with only the production code plus the module declaration that points to the new test file.Source: Coding guidelines
🧹 Nitpick comments (13)
crates/skippy-server/src/frontend/speculative.rs (1)
52-78: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBound the n-gram search
propose_ngram_tokensscanscontext_tokenswith a cubic worst case, andembedded_generation.rscalls it directly in the decode loop whenrequest.ngram_max > 0. Limiting the search window or indexing recent n-grams would reduce the hot-path cost on long generations.🤖 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 `@crates/skippy-server/src/frontend/speculative.rs` around lines 52 - 78, The hot-path n-gram lookup in propose_ngram_tokens is unbounded and can become too expensive during decode. Update propose_ngram_tokens to only search within a bounded recent window of history, or switch it to a more efficient indexed/reused n-gram lookup, and make sure embedded_generation.rs continues calling the optimized path when request.ngram_max is enabled.crates/mesh-llm-host-runtime/src/inference/skippy/mod.rs (1)
453-458: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsolidate the duplicated
EmbeddedOpenAiArgsconstruction.The same ~30-line field block (now including
ngram_min/ngram_max/native-MTP draft fields) is copy-pasted across 4 functions. This PR had to touch all 4 in lockstep to add these fields, and the wiring-plan doc lists more speculative-decoding fields to come — each will repeat this risk of silent divergence if one site is missed.Consider extracting a shared helper (e.g.,
fn build_embedded_openai_args(embedded_args: ResolvedEmbeddedOpenAiArgs, config: StageConfig, runtime: ..., telemetry: Telemetry, hook_policy: ..., prediction_returns: Option<...>) -> EmbeddedOpenAiArgs) that the 4 call sites populate with only their differing parameters.Also applies to: 554-559, 731-736, 859-864
🤖 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 `@crates/mesh-llm-host-runtime/src/inference/skippy/mod.rs` around lines 453 - 458, The repeated EmbeddedOpenAiArgs field wiring across the Skippy inference setup is duplicated in multiple call sites, making future speculative-decoding additions easy to miss. Extract the shared construction into a helper in the mod.rs flow (for example around the EmbeddedOpenAiArgs build path used by the four functions) and have each call site pass only its differing inputs while the helper centralizes the common fields like ngram_min, ngram_max, and native_mtp_*.crates/skippy-model-package/src/preflight.rs (1)
1739-1739: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared constant for the
"mtp"strategy id.The renamed strategy id
"mtp"is now hardcoded as a literal in this crate's generation logic and tests, and per the PR's rename cohort also inmesh-llm-config/src/validate.rs. A shared constant (e.g. inmesh-llm-configor a common crate) would prevent future drift between the generator and the validator if the id changes again.🤖 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 `@crates/skippy-model-package/src/preflight.rs` at line 1739, The renamed strategy id is still hardcoded as the literal "mtp" in the generation logic and tests, which risks drifting from the validator. Introduce a shared constant for the strategy id in a common place, then update the relevant code in preflight generation and the related validation path in mesh-llm-config/src/validate.rs to reference that constant instead of repeating the string literal.crates/mesh-llm-commands/src/gpus/tune_apply.rs (1)
133-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate rendering/ref helpers vs. benchmark.rs.
appended_model_ref(lines 133-138),render_kv_cache_type(185-191), andrender_bool_or_auto(207-213) are re-implemented nearly verbatim incrates/mesh-llm-commands/src/gpus/tune/benchmark.rsastrial_model_ref(benchmark.rs:582-591),render_cache_type(benchmark.rs:1156-1162), andrender_bool_or_auto(benchmark.rs:1148-1154). Extracting these into a shared module (e.g.tune/render.rs) would remove the duplication and prevent the two copies from silently drifting.Also applies to: 185-213
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_apply.rs` around lines 133 - 138, The helpers in tune_apply.rs are duplicated in benchmark.rs, so extract the shared rendering logic into a common module and reuse it from both places. Move appended_model_ref, render_kv_cache_type, and render_bool_or_auto into a shared tune/render helper, then update tune_apply.rs and benchmark.rs to call the shared functions instead of keeping separate copies. Keep the existing behavior and signatures aligned so trial_model_ref/render_cache_type/render_bool_or_auto map cleanly to the shared implementations.crates/mesh-llm-commands/src/gpus/tune/benchmark.rs (1)
71-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFlatten the candidate-generation loop
Use a small cross-product helper or chained iterators instead of six nestedforloops here;benchmark_candidatesis already deep enough to hurt readability and maintainability.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs` around lines 71 - 126, Flatten benchmark_candidates by replacing the six nested loops with a small cross-product helper or chained iterator combinators so the candidate construction reads declaratively instead of imperatively. Keep the existing filtering behavior for ubatch > batch and preserve the same TuneBenchmarkCandidate fields and input sources (contexts, batches, ubatches, mmap_values, mlock_values, speculative_values) while refactoring the loop structure only.Source: Coding guidelines
crates/mesh-llm-commands/src/gpus/tune/output_render.rs (1)
3-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting these large rendering functions into helpers.
render_tune_human_outputandwrite_benchmark_sectioneach inline the full per-target/per-trial rendering logic. Extracting arender_target_sectionandrender_benchmark_trial_linehelper would keep both functions well under any line-count/complexity limits as more fields get added later.As per coding guidelines, "Do not add Rust methods or functions over the configured Clippy line-count limit. Split long logic into semantically named helpers before it reaches the configured
too_many_linesthreshold."Also applies to: 167-254
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/output_render.rs` around lines 3 - 101, The rendering logic in render_tune_human_output and write_benchmark_section is too large and should be split into smaller helpers before hitting the Clippy line-count threshold. Extract semantically named helpers such as render_target_section for the per-target block and render_benchmark_trial_line for per-trial output, then have render_tune_human_output and write_benchmark_section delegate to them while preserving the same formatting and behavior.Source: Coding guidelines
crates/mesh-llm-commands/src/gpus/tune_runner.rs (1)
498-515: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOffload benchmark tuning from the async path.
std::thread::scope(...).join()still blocks the caller for the full benchmark run, sodispatch_benchmark_commandkeeps a Tokio worker occupied here. Move this totokio::task::spawn_blockingwith owned inputs and await it at the async boundary; otherwise the extra thread only adds overhead.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_runner.rs` around lines 498 - 515, The benchmark tuning path is still blocking the async runtime because maybe_run_benchmark_reports and run_benchmark_plans_on_plain_thread use std::thread::scope().join() inside the call chain from dispatch_benchmark_command. Replace this with tokio::task::spawn_blocking at the async boundary so the benchmark work runs on a blocking thread pool, pass only owned inputs into the blocking closure, and await the task result before returning TuneBenchmarkTargetReport values.crates/mesh-llm-commands/src/gpus/tune/matrix.rs (1)
1-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ALLis hand-maintained and can silently drift from the enum.Unlike the exhaustive
matchinspec()(which the compiler forces to stay complete),ALLis a manually listed array. If a newTuneFieldvariant is added later without remembering to add it here, the compiler won't catch it — it'll just be silently missing from any iteration relying onALL.Consider deriving
strum::EnumIteronTuneFieldand buildingALL(or replacing its call sites) fromTuneField::iter().collect(), so the compiler/iterator generation stays in sync with variant additions automatically.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/matrix.rs` around lines 1 - 19, The TuneField::ALL array is manually maintained and can drift when new variants are added. Update TuneField in matrix.rs to use an enum-driven source of truth, such as deriving strum::EnumIter and building ALL from TuneField::iter(), or replace ALL call sites with iteration over TuneField::iter() so additions to TuneField stay automatically in sync.crates/model-artifact/src/gguf.rs (1)
561-606: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared GGUF header/KV-skip parsing.
The magic/version validation and KV-pair skip loop here duplicate logic already present in
scan_gguf_compact_meta(and likelyscan_gguf_tensor_byte_profile). Extracting a shared helper (e.g.open_gguf_header+skip_all_kv_pairs) would reduce triplicated parsing logic and the risk of these copies drifting apart on future format changes.♻️ Sketch of a shared helper
+fn open_gguf_header(path: &Path) -> Option<(std::fs::File, u64, u64)> { + let mut f = std::fs::File::open(path).ok()?; + let mut magic = [0u8; 4]; + f.read_exact(&mut magic).ok()?; + if &magic != b"GGUF" { + return None; + } + let version = read_u32(&mut f).ok()?; + if version < 2 { + return None; + } + let n_tensors = read_gguf_header_count(&mut f, MAX_GGUF_TENSOR_COUNT, "tensor count").ok()?; + let n_kv = read_gguf_header_count(&mut f, MAX_GGUF_HEADER_KV_COUNT, "KV count").ok()?; + Some((f, n_tensors, n_kv)) +} + +fn skip_all_kv_pairs(f: &mut std::fs::File, n_kv: u64) -> Option<()> { + for _ in 0..n_kv { + let _key = read_gguf_string(f).ok()?; + let vtype = GgufType::from_u32(read_u32(f).ok()?)?; + skip_gguf_value(f, vtype).ok()?; + } + Some(()) +}🤖 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 `@crates/model-artifact/src/gguf.rs` around lines 561 - 606, The GGUF header validation and KV-skipping logic in scan_gguf_tensor_names_any is duplicated in scan_gguf_compact_meta and likely scan_gguf_tensor_byte_profile. Refactor the shared magic/version checks and KV-pair skip loop into common helpers such as open_gguf_header and skip_all_kv_pairs, then update scan_gguf_tensor_names_any to use them while keeping its tensor-name scan behavior unchanged.crates/mesh-llm-commands/src/gpus.rs (1)
11-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider real
moddeclarations instead of flattening 17 files withinclude!.Building
tunebyinclude!-ing many separate files into one flat scope discards Rust's module system benefits: per-file privacy, no accidental name collisions across files, and normal IDE/tooling navigation. Given the stack plan adds several moretune/*.rsfiles into this exact same block across later layers in this PR, the risk of subtle symbol collisions (private helpers with the same name in two files) grows with the file count. A standardpub mod tune { mod types; mod matrix; ... }tree with targetedpub(crate) usere-exports would give the same organization with proper module boundaries.🤖 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 `@crates/mesh-llm-commands/src/gpus.rs` around lines 11 - 49, The `tune` module in `gpus.rs` is flattening many files with `include!`, which removes normal Rust module boundaries and increases the risk of symbol collisions. Replace the `include!`-based aggregation with real `mod` declarations for the `tune` submodules (for example `types`, `matrix`, `metadata`, etc.), then add only the necessary `pub(crate) use` re-exports so existing callers still work. Keep the `tune`, `tune_apply`, `tune_hardware`, `tune_resolver`, and `tune_runner` symbols as the entry points while restoring per-file privacy and cleaner navigation.crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs (2)
108-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
unreachable!()arms rely on an invariant not enforced by the type system.Both
resolve_requested_gpu(lines 127-131) andmissing_configured_device(lines 191-195)unreachable!()onTuneDeviceSelectionSource::SurveyDefault/CpuSystemRamFallback, relying ondevice_request.rsnever constructing aConfiguredTuneDeviceRequestwith those variants. This holds today, but nothing stops a future caller/test from constructing one and triggering a runtime panic. Consider narrowingConfiguredTuneDeviceRequest::sourceto a dedicated enum that only contains the three "configured" variants, making the invalid state unrepresentable.Also applies to: 187-195
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs` around lines 108 - 132, The `unreachable!()` cases in `resolve_requested_gpu` and `missing_configured_device` depend on `ConfiguredTuneDeviceRequest::source` never being `SurveyDefault` or `CpuSystemRamFallback`, but that invariant is not enforced by the type system. Update `ConfiguredTuneDeviceRequest` (and any constructors/usages in the tune hardware flow) so `source` uses a narrower enum that only allows the valid configured-device variants, and adjust `resolve_requested_gpu`/`missing_configured_device` to handle only those cases instead of relying on runtime panics.
62-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDefault GPU selection picks the first survey entry, not necessarily the best one.
When no device is configured, the code selects
survey.gpus.iter().find(|gpu| gpu.backend_device.is_some())— effectively an arbitrary "first in list" choice rather than, e.g., the GPU with the most allocatable VRAM. For multi-GPU hosts without explicit config, this can silently target a weaker/smaller card for tuning.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs` around lines 62 - 69, The default GPU selection in evaluate_hardware currently uses the first survey GPU with a backend device, which can pick a suboptimal card. Update the selection logic in evaluate_hardware so it chooses the best available GPU for tuning instead of the first match, using a clear ranking criterion such as highest allocatable VRAM (or equivalent capacity metric) among the GPUs returned by survey.gpus. Keep the existing EvaluatedTuneDevice and to_gpu_target flow, but change the iterator logic around survey.gpus.iter().find(...) to select the preferred GPU deterministically.crates/mesh-llm-commands/src/gpus/tune/recommendation.rs (1)
108-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the partial-layer search into a named helper.
plan_fitmixes device-target dispatch, full-offload check, CPU-fit check, and the partial-GPU-layer search loop in one function. Splitting theTuneDeviceTarget::Gpupartial-offload search (Lines 152-178) into a dedicated helper (e.g.find_partial_gpu_layers_fit) would keepplan_fit's control flow flatter and easier to follow, per repo guidance to prefer small, named decision helpers over nested branching.As per coding guidelines, "Do not add Rust code over the configured cognitive-complexity limit. Prefer small, named decision helpers and clear control-flow phases instead of nested branching."
Also applies to: 180-222
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/recommendation.rs` around lines 108 - 138, The `plan_fit` function is doing too much at once by combining target dispatch, fit checks, and the partial GPU-layer search logic. Extract the `TuneDeviceTarget::Gpu` partial-offload branch from `plan_fit` into a dedicated helper such as `find_partial_gpu_layers_fit`, and have `plan_fit` delegate to it so the overall control flow stays flat and easier to read. Keep the helper focused on the partial-layer search and return the same `PlannedFit`-compatible result shape used by `plan_fit`.Source: Coding guidelines
🤖 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 `@crates/mesh-llm-commands/Cargo.toml`:
- Line 39: The benchmark tune path in mesh_llm is using reqwest::blocking from
within a Tokio-driven command flow, which can panic on async runtimes. Update
the synchronous benchmark runner path (the one reached from the async dispatch
tree) to avoid blocking reqwest on Tokio by either moving the HTTP work behind
spawn_blocking or a dedicated thread, or by converting that call path to async
reqwest end-to-end. Use the benchmark runner entrypoint and the command dispatch
flow in mesh_llm to locate the affected code.
In `@crates/mesh-llm-commands/src/gpus.rs`:
- Around line 315-375: The test fixture helpers are duplicated between the GPU
tests and the tune_resolver tests, so extract CacheFixtureGuard,
write_local_gguf_file, random_suffix, write_hf_cache_gguf, and
mesh_config_with_models into a shared #[cfg(test)] support module. Update both
gpus.rs test code and tune_resolver/tests.rs to use the shared helpers instead
of keeping separate copies, preserving the existing function and type names so
the call sites stay easy to locate.
In `@crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs`:
- Around line 108-132: The fallback in resolve_requested_gpu is discarding the
backend-device failure by returning only the original pinned-GPU diagnostic.
Update the ModelHardwareDevice/DefaultsHardwareDevice branch so
resolve_backend_device contributes its own diagnostic instead of using
map_err(|_| diagnostic), and keep the richer error context visible to callers.
Also replace the string-based “not pinnable” / “available pinnable GPU IDs”
check with a structured match if possible, using resolve_requested_pinned_gpu
and resolve_backend_device as the key paths to update.
In `@crates/mesh-llm-commands/src/gpus/tune_runner.rs`:
- Around line 38-41: Remove the leftover symbol-anchor calls in
tune_runner::run_tune: recommendation_symbol_anchor(), resolver_symbol_anchor(),
and dispatch_symbol_anchor() are no-ops with discarded results and should not
remain in production flow. Delete these calls and keep the actual tune logic
that already uses resolve_configured_tune_targets, evaluate_tune_hardware, and
build_tune_plan later in the same function.
- Around line 29-203: The function run_tune_request_with_writer repeats the same
RunnerOutputContext construction in several branches, making it too long and
hard to maintain. Extract a small helper around the shared emit_runner_output
call that takes the varying blockers slice plus the common inputs from
TuneRunnerArgs, config, apply_mode, prepared, target_failures, and
benchmark_reports. Replace each duplicated struct literal in
run_tune_request_with_writer with that helper so the control flow stays the same
but the repeated context-building logic is centralized.
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs`:
- Around line 24-36: `run_benchmark_plans` only checks
`throughput_tolerance_pct` with `debug_assert!`, so invalid values can slip
through in release builds. Add a real validation path in `run_benchmark_plans`
(or the request parsing/constructor) that rejects non-finite or negative
tolerance values and returns a propagated error instead of proceeding. Update
the caller flow and any related benchmark selection logic such as
`throughput_threshold` in `benchmark_selection.rs` so malformed input cannot
reach `run_target_benchmarks` or produce a silent “no recommendation.”
- Around line 684-702: `create_trial_dir` is using the process current working
directory to place trial artifacts under a misleading `target/gpu-tune` path,
which makes the CLI fragile and pollutes the user’s folder. Update
`create_trial_dir` to build the directory under a dedicated temp/cache location
instead of `std::env::current_dir()`, while keeping the existing model-specific
and timestamped subdirectory naming via `sanitize_path_component` and the trial
index.
- Around line 1000-1027: The benchmark tests are using hardcoded /tmp fixtures,
which causes discover_sibling_draft_models to scan the real parent directory and
pick up unrelated .gguf files. Update the affected tests to create their model
files inside isolated tempfile::tempdir()-backed directories, following the
pattern used by other tests in this layer, so resolved_path only sees the
intended sibling draft models. Use discover_sibling_draft_models and
resolved_path to locate the affected setup.
In `@crates/mesh-llm-commands/src/gpus/tune/output_values.rs`:
- Around line 71-81: `render_benchmark_candidate` currently formats only ctx,
batch, ubatch, mmap, mlock, and spec, so trials that differ in cache type can
look identical in human output. Update this helper to include `cache_type_k` and
`cache_type_v` in the rendered string, using the `TuneBenchmarkCandidate` fields
directly and keeping the formatting consistent with the existing summary used by
`render_tune_human_output`.
In `@crates/mesh-llm-config/src/validate.rs`:
- Around line 812-817: The speculative strategy validation in
validate_speculative currently hard-rejects native-mtp-n1, breaking existing
configs without a migration path. Update the validation flow to accept
native-mtp-n1 as a legacy alias for mtp and emit a deprecation/alias diagnostic
instead of a generic enum failure, similar to other legacy-field handling in
this file such as gpu_id → hardware.device. Keep the canonical allowlist
centered on mtp, but route the old value through an alias warning for at least
one release cycle.
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs`:
- Around line 300-309: The MTP sidecar gate in
direct_gguf_supports_native_mtp/contains_mtp_marker is currently based on the
draft filename instead of the GGUF contents, so a renamed file can incorrectly
pass. Update the MTP check to inspect the draft GGUF metadata/tensors using the
existing scan_gguf_compact_meta and scan_gguf_tensor_names_any logic, and remove
or stop using the filename-based contains_mtp_marker path for strategy = "mtp".
Also tighten the related test in speculative.rs so it validates real native MTP
markers in the GGUF rather than relying on a tempfile name prefix and a minimal
GGUF header.
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rs`:
- Around line 375-380: The fallback defaults in direct_single_stage_defaults and
embedded_stage_defaults are hardcoding native MTP on, which then propagates
through the skippy resolver into mod.rs. Update these helpers to derive
native_mtp_enabled and native_mtp_max_tokens from the resolved config instead of
setting fixed values, so the no-embedded_openai path reflects the actual model
defaults. Use the existing translation/resolution flow in translation.rs to
source the values consistently before they are forwarded to the fallback config.
In `@crates/skippy-prompt/src/prompt_cli/binary_repl.rs`:
- Around line 30-31: The BinaryReplArgs path is ignoring the CLI load controls
because both StageModel::open calls in BinaryReplArgs still hardcode mmap and
mlock defaults. Update BinaryReplArgs and the binary REPL wiring in
binary_repl.rs so mmap/mlock are either explicitly exposed and passed through to
StageModel::open, or clearly kept as fixed defaults in one place; use the
existing StageModel::open call sites and BinaryReplArgs struct to locate the
change.
In `@crates/skippy-server/src/frontend/local_generation.rs`:
- Around line 531-582: The local generation path in local_generation.rs is
dropping the native-MTP window lower bound and hard-coding the draft origin.
Update the request handling around decode_frame_sampled_mtp and
native_mtp.observe_target_token so the with_window-derived native_mtp_min_tokens
is threaded into the sampled MTP decode call, and choose NativeMtpDraftOrigin
based on whether this is the first draft (use InitialSerial for the first-draft
case instead of always SerialAfterGap). Keep the changes localized to the
request/native_mtp_enabled branch and the native_mtp_decision construction.
In
`@third_party/llama.cpp/patches/0012-Add-external-MTP-draft-sidecar-attachment.patch`:
- Around line 6-9: The staged-runtime ABI changed because this patch adds a new
exported ABI function and a new field on skippy_model, so update
SKIPPY_ABI_VERSION_PATCH in skippy/common.h accordingly. Then make sure the
Rust-side mirror of the ABI version is kept in sync wherever that constant is
duplicated or generated, so both sides agree on the same version.
---
Outside diff comments:
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs`:
- Around line 1-2000: The benchmark module file is over the 2,000-line limit
because the embedded #[cfg(test)] mod benchmark_tests block dominates the file.
Move benchmark_tests, including all fixtures and unit tests, into a sibling
benchmark_tests.rs using the same split-module pattern already used by
apply_collision_tests.rs and apply_write_tests.rs, and leave benchmark.rs with
only the production code plus the module declaration that points to the new test
file.
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs`:
- Around line 97-114: The speculative config validation in `speculative.rs` is
comparing `draft_min_tokens` against the raw `draft_max_tokens`, which misses
the native-MTP default path. Update the guard in the resolver logic so it uses
the resolved max token value (including the native-MTP default of 3 when
`native_mtp_enabled` applies), or compute the resolved max once and reuse it for
validation and downstream setup. Keep the check near the existing
`normalize_pairing_fault`, `explicit`, and `draft_min_tokens`/`draft_max_tokens`
validation flow.
In `@crates/mesh-llm-host-runtime/src/mesh/mod.rs`:
- Around line 9247-9297: The issue is that new proto serialization helpers are
being added to an already oversized mesh module, pushing the file further past
the 2,000-line guideline. Move stage_load_to_proto and the related
stage_load_from_proto logic, along with any closely related StageLoadRequest
conversion helpers, into a dedicated owning module and keep the mesh/mod.rs
wrapper minimal by delegating to that module. Ensure the new module owns the
skippy_stage_proto conversion code so this file stops accumulating more
responsibility.
---
Nitpick comments:
In `@crates/mesh-llm-commands/src/gpus.rs`:
- Around line 11-49: The `tune` module in `gpus.rs` is flattening many files
with `include!`, which removes normal Rust module boundaries and increases the
risk of symbol collisions. Replace the `include!`-based aggregation with real
`mod` declarations for the `tune` submodules (for example `types`, `matrix`,
`metadata`, etc.), then add only the necessary `pub(crate) use` re-exports so
existing callers still work. Keep the `tune`, `tune_apply`, `tune_hardware`,
`tune_resolver`, and `tune_runner` symbols as the entry points while restoring
per-file privacy and cleaner navigation.
In `@crates/mesh-llm-commands/src/gpus/tune_apply.rs`:
- Around line 133-138: The helpers in tune_apply.rs are duplicated in
benchmark.rs, so extract the shared rendering logic into a common module and
reuse it from both places. Move appended_model_ref, render_kv_cache_type, and
render_bool_or_auto into a shared tune/render helper, then update tune_apply.rs
and benchmark.rs to call the shared functions instead of keeping separate
copies. Keep the existing behavior and signatures aligned so
trial_model_ref/render_cache_type/render_bool_or_auto map cleanly to the shared
implementations.
In `@crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs`:
- Around line 108-132: The `unreachable!()` cases in `resolve_requested_gpu` and
`missing_configured_device` depend on `ConfiguredTuneDeviceRequest::source`
never being `SurveyDefault` or `CpuSystemRamFallback`, but that invariant is not
enforced by the type system. Update `ConfiguredTuneDeviceRequest` (and any
constructors/usages in the tune hardware flow) so `source` uses a narrower enum
that only allows the valid configured-device variants, and adjust
`resolve_requested_gpu`/`missing_configured_device` to handle only those cases
instead of relying on runtime panics.
- Around line 62-69: The default GPU selection in evaluate_hardware currently
uses the first survey GPU with a backend device, which can pick a suboptimal
card. Update the selection logic in evaluate_hardware so it chooses the best
available GPU for tuning instead of the first match, using a clear ranking
criterion such as highest allocatable VRAM (or equivalent capacity metric) among
the GPUs returned by survey.gpus. Keep the existing EvaluatedTuneDevice and
to_gpu_target flow, but change the iterator logic around
survey.gpus.iter().find(...) to select the preferred GPU deterministically.
In `@crates/mesh-llm-commands/src/gpus/tune_runner.rs`:
- Around line 498-515: The benchmark tuning path is still blocking the async
runtime because maybe_run_benchmark_reports and
run_benchmark_plans_on_plain_thread use std::thread::scope().join() inside the
call chain from dispatch_benchmark_command. Replace this with
tokio::task::spawn_blocking at the async boundary so the benchmark work runs on
a blocking thread pool, pass only owned inputs into the blocking closure, and
await the task result before returning TuneBenchmarkTargetReport values.
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs`:
- Around line 71-126: Flatten benchmark_candidates by replacing the six nested
loops with a small cross-product helper or chained iterator combinators so the
candidate construction reads declaratively instead of imperatively. Keep the
existing filtering behavior for ubatch > batch and preserve the same
TuneBenchmarkCandidate fields and input sources (contexts, batches, ubatches,
mmap_values, mlock_values, speculative_values) while refactoring the loop
structure only.
In `@crates/mesh-llm-commands/src/gpus/tune/matrix.rs`:
- Around line 1-19: The TuneField::ALL array is manually maintained and can
drift when new variants are added. Update TuneField in matrix.rs to use an
enum-driven source of truth, such as deriving strum::EnumIter and building ALL
from TuneField::iter(), or replace ALL call sites with iteration over
TuneField::iter() so additions to TuneField stay automatically in sync.
In `@crates/mesh-llm-commands/src/gpus/tune/output_render.rs`:
- Around line 3-101: The rendering logic in render_tune_human_output and
write_benchmark_section is too large and should be split into smaller helpers
before hitting the Clippy line-count threshold. Extract semantically named
helpers such as render_target_section for the per-target block and
render_benchmark_trial_line for per-trial output, then have
render_tune_human_output and write_benchmark_section delegate to them while
preserving the same formatting and behavior.
In `@crates/mesh-llm-commands/src/gpus/tune/recommendation.rs`:
- Around line 108-138: The `plan_fit` function is doing too much at once by
combining target dispatch, fit checks, and the partial GPU-layer search logic.
Extract the `TuneDeviceTarget::Gpu` partial-offload branch from `plan_fit` into
a dedicated helper such as `find_partial_gpu_layers_fit`, and have `plan_fit`
delegate to it so the overall control flow stays flat and easier to read. Keep
the helper focused on the partial-layer search and return the same
`PlannedFit`-compatible result shape used by `plan_fit`.
In `@crates/mesh-llm-host-runtime/src/inference/skippy/mod.rs`:
- Around line 453-458: The repeated EmbeddedOpenAiArgs field wiring across the
Skippy inference setup is duplicated in multiple call sites, making future
speculative-decoding additions easy to miss. Extract the shared construction
into a helper in the mod.rs flow (for example around the EmbeddedOpenAiArgs
build path used by the four functions) and have each call site pass only its
differing inputs while the helper centralizes the common fields like ngram_min,
ngram_max, and native_mtp_*.
In `@crates/model-artifact/src/gguf.rs`:
- Around line 561-606: The GGUF header validation and KV-skipping logic in
scan_gguf_tensor_names_any is duplicated in scan_gguf_compact_meta and likely
scan_gguf_tensor_byte_profile. Refactor the shared magic/version checks and
KV-pair skip loop into common helpers such as open_gguf_header and
skip_all_kv_pairs, then update scan_gguf_tensor_names_any to use them while
keeping its tensor-name scan behavior unchanged.
In `@crates/skippy-model-package/src/preflight.rs`:
- Line 1739: The renamed strategy id is still hardcoded as the literal "mtp" in
the generation logic and tests, which risks drifting from the validator.
Introduce a shared constant for the strategy id in a common place, then update
the relevant code in preflight generation and the related validation path in
mesh-llm-config/src/validate.rs to reference that constant instead of repeating
the string literal.
In `@crates/skippy-server/src/frontend/speculative.rs`:
- Around line 52-78: The hot-path n-gram lookup in propose_ngram_tokens is
unbounded and can become too expensive during decode. Update
propose_ngram_tokens to only search within a bounded recent window of history,
or switch it to a more efficient indexed/reused n-gram lookup, and make sure
embedded_generation.rs continues calling the optimized path when
request.ngram_max is enabled.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a8b8d6c-4bbf-4b55-9d7f-041583bf0380
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (118)
.agents/skills/benchmark-tune/SKILL.md.agents/skills/benchmark-tune/agents/openai.yamlcrates/mesh-llm-cli/src/benchmark.rscrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-commands/Cargo.tomlcrates/mesh-llm-commands/src/benchmark.rscrates/mesh-llm-commands/src/gpus.rscrates/mesh-llm-commands/src/gpus/tune/apply_collision_tests.rscrates/mesh-llm-commands/src/gpus/tune/apply_test_support.rscrates/mesh-llm-commands/src/gpus/tune/apply_write_tests.rscrates/mesh-llm-commands/src/gpus/tune/benchmark.rscrates/mesh-llm-commands/src/gpus/tune/benchmark_progress.rscrates/mesh-llm-commands/src/gpus/tune/benchmark_selection.rscrates/mesh-llm-commands/src/gpus/tune/matrix.rscrates/mesh-llm-commands/src/gpus/tune/metadata.rscrates/mesh-llm-commands/src/gpus/tune/metadata_tests.rscrates/mesh-llm-commands/src/gpus/tune/output_emit.rscrates/mesh-llm-commands/src/gpus/tune/output_launch.rscrates/mesh-llm-commands/src/gpus/tune/output_render.rscrates/mesh-llm-commands/src/gpus/tune/output_report.rscrates/mesh-llm-commands/src/gpus/tune/output_tests.rscrates/mesh-llm-commands/src/gpus/tune/output_types.rscrates/mesh-llm-commands/src/gpus/tune/output_values.rscrates/mesh-llm-commands/src/gpus/tune/planning.rscrates/mesh-llm-commands/src/gpus/tune/recommendation.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_defaults_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_existing.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_failure_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_reports.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_writes.rscrates/mesh-llm-commands/src/gpus/tune/tests.rscrates/mesh-llm-commands/src/gpus/tune/types.rscrates/mesh-llm-commands/src/gpus/tune_apply.rscrates/mesh-llm-commands/src/gpus/tune_hardware.rscrates/mesh-llm-commands/src/gpus/tune_hardware/device_request.rscrates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rscrates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/helpers.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/mlock_reporting.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/selection.rscrates/mesh-llm-commands/src/gpus/tune_hardware/types.rscrates/mesh-llm-commands/src/gpus/tune_resolver.rscrates/mesh-llm-commands/src/gpus/tune_resolver/tests.rscrates/mesh-llm-commands/src/gpus/tune_resolver/types.rscrates/mesh-llm-commands/src/gpus/tune_runner.rscrates/mesh-llm-commands/src/gpus/tune_runner_tests.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-config/src/validate_gpu_tune_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/deployment.rscrates/mesh-llm-host-runtime/src/inference/skippy/family_policy.rscrates/mesh-llm-host-runtime/src/inference/skippy/materialization.rscrates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/test_support.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/types.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/types.rscrates/mesh-llm-host-runtime/src/mesh/mod.rscrates/mesh-llm-host-runtime/src/mesh/tests.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-system/src/benchmark.rscrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/tests/cli_errors.rscrates/model-artifact/src/gguf.rscrates/skippy-bench/src/distributed.rscrates/skippy-bench/src/local_split.rscrates/skippy-bench/src/token_lengths.rscrates/skippy-bench/src/verify_span_local.rscrates/skippy-correctness/src/report.rscrates/skippy-correctness/src/runner.rscrates/skippy-correctness/tests/parity_models/mod.rscrates/skippy-ffi/src/lib.rscrates/skippy-model-package/src/main.rscrates/skippy-model-package/src/preflight.rscrates/skippy-prompt/src/prompt_cli/binary_repl.rscrates/skippy-prompt/src/prompt_cli/stage_config.rscrates/skippy-protocol/proto/stage.protocrates/skippy-protocol/src/lib.rscrates/skippy-runtime/src/lib.rscrates/skippy-server/src/binary_transport.rscrates/skippy-server/src/binary_transport/forwarding.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/binary_transport/tests.rscrates/skippy-server/src/cli.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/embedded_execution.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/native_mtp/decode.rscrates/skippy-server/src/frontend/native_mtp/draft.rscrates/skippy-server/src/frontend/native_mtp/mod.rscrates/skippy-server/src/frontend/native_mtp/stats.rscrates/skippy-server/src/frontend/native_mtp/verifier.rscrates/skippy-server/src/frontend/prefix_cache.rscrates/skippy-server/src/frontend/speculative.rscrates/skippy-server/src/frontend/tests.rscrates/skippy-server/src/kv_integration/activation.rscrates/skippy-server/src/kv_integration/config.rscrates/skippy-server/src/kv_integration/identity.rscrates/skippy-server/src/runtime_state.rsdocs/CLI.mddocs/USAGE.mddocs/design/GLM_NATIVE_MTP_SKIPPY_ARCHITECTURE.mddocs/design/TESTING.mddocs/skippy/CONFIGURATION.mddocs/specs/layer-package-repos.mddocs/specs/speculative-decoding-wiring-plan.mdscripts/build-llama.shthird_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patchthird_party/llama.cpp/patches/0011-Wire-mmap-and-mlock-runtime-load-options.patchthird_party/llama.cpp/patches/0012-Add-external-MTP-draft-sidecar-attachment.patch
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs (1)
111-138: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the pinned/backend fallback dispatch into a named helper.
This block nests a match, an error guard with two
||conditions, and a closure that mutates a cloned diagnostic — flagged as high complexity. As per coding guidelines, "Do not add Rust code over the configured cognitive-complexity limit... Prefer small, named decision helpers and clear control-flow phases instead of nested branching." Extracting the guard-and-combine logic (e.g., intoresolve_pinned_with_backend_fallback) would reduce nesting and make the control flow phases explicit.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs` around lines 111 - 138, The control flow in resolve_requested_gpu is too complex because it mixes pinned-device resolution, backend fallback dispatch, and error combination in one match. Extract the guard-and-fallback logic into a small named helper such as resolve_pinned_with_backend_fallback, and keep resolve_requested_gpu focused on choosing between LegacyGpuId and the hardware-device path. Move the diagnostic-cloning and backend error append behavior into that helper so the two-phase decision is explicit and the nested match/closure are removed.Source: Coding guidelines
🧹 Nitpick comments (3)
crates/mesh-llm-commands/src/gpus/tune/matrix.rs (1)
6-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider merging the two
match selfblocks.
config_pathandsupportare each derived from a separate exhaustive match onself. Combining them into a single match returning(ConfigPath, TuneFieldSupport)per variant would shorten the function and reduce duplicated pattern matching, aligning with the guideline to prefer smaller, clearer control-flow phases as functions approach complexity limits.As per coding guidelines, "Do not add Rust code over the configured cognitive-complexity limit. Prefer small, named decision helpers and clear control-flow phases instead of nested branching."
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/matrix.rs` around lines 6 - 70, The spec method in TuneFieldSpec::spec repeats an exhaustive match on self for config_path and support, making the function longer and harder to maintain. Refactor it into a single match on self that returns both values together (for example as a pair) and then build TuneFieldSpec from that result, preserving the existing behavior for every TuneField variant. This keeps the control flow in one clear phase and reduces duplicated pattern matching in matrix.rs.Source: Coding guidelines
crates/mesh-llm-system/src/util.rs (1)
1-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the new utility functions.
contains_mtp_marker,contains_mtp_marker_str, andvalidate_draft_min_maxare new pure functions that gate MTP detection and speculative-decoding validation. They're cheap to test and would catch regressions in matching/validation logic.🤖 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 `@crates/mesh-llm-system/src/util.rs` around lines 1 - 34, Add unit tests for the new pure helpers in util.rs: contains_mtp_marker, contains_mtp_marker_str, and validate_draft_min_max. Cover positive and negative cases for MTP matching patterns and path/file-name handling, plus both valid and invalid draft_min_tokens/draft_max_tokens combinations. Place the tests near these functions or in the existing util test module so regressions in the marker-detection and validation logic are caught.crates/skippy-prompt/src/prompt_cli/args.rs (1)
241-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify
--mmap's value-taking syntax.Unlike
--mlockand otherboolfields in this struct (which are bare flags via clap's defaultSetTrueaction),--mmap: Option<bool>requires an explicit value (--mmap true/--mmap false) since it isn't a literalbool. This tri-state design (auto/true/false) looks intentional, but a shorthelpstring would prevent users from assuming--mmapis a bare flag like its neighbors.📝 Proposed help text
- #[arg(long)] + #[arg(long, help = "Override mmap for model loading (true/false); omit to use the runtime default")] pub mmap: Option<bool>,🤖 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 `@crates/skippy-prompt/src/prompt_cli/args.rs` around lines 241 - 244, The `Args` struct’s `mmap` field uses `Option<bool>`, so `--mmap` is not a bare flag like `mlock` and needs an explicit value; add a short `help`/doc string on the `mmap` field in `args.rs` to make the `true`/`false` syntax clear and distinguish it from the neighboring `bool` flags. Keep the tri-state intent obvious by referencing the `mmap` and `mlock` fields in the wording so users understand `--mmap true/false` is required.
🤖 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 `@crates/mesh-llm-system/src/util.rs`:
- Around line 3-10: The `contains_mtp_marker` doc comment and implementation are
inconsistent: it claims to check a path or string, but only `file_name()` is
inspected. Update `contains_mtp_marker` in `util.rs` to either examine the full
path text (so markers in parent directories are detected) or narrow the doc
comment to say it only checks the filename; keep the behavior and documentation
aligned.
---
Outside diff comments:
In `@crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs`:
- Around line 111-138: The control flow in resolve_requested_gpu is too complex
because it mixes pinned-device resolution, backend fallback dispatch, and error
combination in one match. Extract the guard-and-fallback logic into a small
named helper such as resolve_pinned_with_backend_fallback, and keep
resolve_requested_gpu focused on choosing between LegacyGpuId and the
hardware-device path. Move the diagnostic-cloning and backend error append
behavior into that helper so the two-phase decision is explicit and the nested
match/closure are removed.
---
Nitpick comments:
In `@crates/mesh-llm-commands/src/gpus/tune/matrix.rs`:
- Around line 6-70: The spec method in TuneFieldSpec::spec repeats an exhaustive
match on self for config_path and support, making the function longer and harder
to maintain. Refactor it into a single match on self that returns both values
together (for example as a pair) and then build TuneFieldSpec from that result,
preserving the existing behavior for every TuneField variant. This keeps the
control flow in one clear phase and reduces duplicated pattern matching in
matrix.rs.
In `@crates/mesh-llm-system/src/util.rs`:
- Around line 1-34: Add unit tests for the new pure helpers in util.rs:
contains_mtp_marker, contains_mtp_marker_str, and validate_draft_min_max. Cover
positive and negative cases for MTP matching patterns and path/file-name
handling, plus both valid and invalid draft_min_tokens/draft_max_tokens
combinations. Place the tests near these functions or in the existing util test
module so regressions in the marker-detection and validation logic are caught.
In `@crates/skippy-prompt/src/prompt_cli/args.rs`:
- Around line 241-244: The `Args` struct’s `mmap` field uses `Option<bool>`, so
`--mmap` is not a bare flag like `mlock` and needs an explicit value; add a
short `help`/doc string on the `mmap` field in `args.rs` to make the
`true`/`false` syntax clear and distinguish it from the neighboring `bool`
flags. Keep the tri-state intent obvious by referencing the `mmap` and `mlock`
fields in the wording so users understand `--mmap true/false` is required.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57d2bd53-f3db-4c08-b6b1-01b8f9da0fd4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Cargo.tomlcrates/mesh-llm-commands/Cargo.tomlcrates/mesh-llm-commands/src/gpus/tune/benchmark.rscrates/mesh-llm-commands/src/gpus/tune/matrix.rscrates/mesh-llm-commands/src/gpus/tune/output_values.rscrates/mesh-llm-commands/src/gpus/tune/recommendation.rscrates/mesh-llm-commands/src/gpus/tune/types.rscrates/mesh-llm-commands/src/gpus/tune_hardware.rscrates/mesh-llm-commands/src/gpus/tune_hardware/device_request.rscrates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rscrates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rscrates/mesh-llm-commands/src/gpus/tune_hardware/types.rscrates/mesh-llm-commands/src/gpus/tune_resolver.rscrates/mesh-llm-commands/src/gpus/tune_runner.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-system/src/lib.rscrates/mesh-llm-system/src/util.rscrates/skippy-prompt/src/prompt_cli/args.rscrates/skippy-prompt/src/prompt_cli/binary_repl.rscrates/skippy-prompt/src/prompt_cli/launch.rscrates/skippy-server/src/frontend/local_generation.rs
💤 Files with no reviewable changes (3)
- crates/mesh-llm-commands/src/gpus/tune_hardware.rs
- crates/mesh-llm-commands/src/gpus/tune_resolver.rs
- crates/mesh-llm-commands/src/gpus/tune_runner.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/skippy-prompt/src/prompt_cli/binary_repl.rs
- crates/mesh-llm-config/src/validate.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/device_request.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rs
- crates/skippy-server/src/frontend/local_generation.rs
- crates/mesh-llm-commands/src/gpus/tune/types.rs
- crates/mesh-llm-commands/Cargo.toml
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs
- crates/mesh-llm-commands/src/gpus/tune/output_values.rs
- crates/mesh-llm-commands/src/gpus/tune/benchmark.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/mesh-llm-config/src/validate.rs`:
- Around line 826-852: `push_speculative_alias_diagnostic` only emits a warning,
while `validate_speculative` still accepts `native-mtp-n1`, so the legacy alias
can reach the resolver and fail later. Update the normalization path around
`validate_speculative`/`push_speculative_alias_diagnostic` to convert
`native-mtp-n1` to `mtp` before speculative resolution, or otherwise ensure the
config value on the `speculative.strategy` path is rewritten so the resolver
never sees the legacy alias.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9619f75-787f-4be4-963f-f325aca7c3a7
📒 Files selected for processing (19)
crates/mesh-llm-cli/src/benchmark.rscrates/mesh-llm-commands/src/benchmark.rscrates/mesh-llm-commands/src/gpus.rscrates/mesh-llm-commands/src/gpus/tune/benchmark.rscrates/mesh-llm-commands/src/gpus/tune_apply.rscrates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rscrates/mesh-llm-commands/src/gpus/tune_runner.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rscrates/mesh-llm-host-runtime/src/mesh/mod.rscrates/mesh-llm-host-runtime/src/mesh/stage_proto.rscrates/model-artifact/src/gguf.rscrates/skippy-ffi/src/lib.rscrates/skippy-server/src/frontend/local_generation.rsthird_party/llama.cpp/patches/0012-Add-external-MTP-draft-sidecar-attachment.patchwebsite/src/docs/pages/CLI.md
💤 Files with no reviewable changes (1)
- crates/mesh-llm-commands/src/gpus.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rs
- crates/skippy-server/src/frontend/local_generation.rs
- crates/mesh-llm-commands/src/benchmark.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/mod.rs
- crates/mesh-llm-commands/src/gpus/tune_apply.rs
- crates/model-artifact/src/gguf.rs
- crates/mesh-llm-cli/src/benchmark.rs
- third_party/llama.cpp/patches/0012-Add-external-MTP-draft-sidecar-attachment.patch
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rs
- crates/mesh-llm-commands/src/gpus/tune_runner.rs
- crates/mesh-llm-commands/src/gpus/tune/benchmark.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@third_party/llama.cpp/patches/0013-Add-non-frame-native-MTP-decode-ABI.patch`:
- Line 86: The MTP ABI wrapper currently passes the caller’s max_draft_tokens
straight through to skippy_mtp_propose_next, which can exceed the fixed
skippy_native_mtp_draft.token_ids capacity. Clamp max_draft_tokens to the native
draft buffer size at this boundary in the wrapper that returns from
skippy_mtp_propose_next, so external callers cannot request more draft tokens
than the ABI can safely store.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 52a3e341-f595-4417-98c4-9ba018234731
📒 Files selected for processing (5)
crates/skippy-ffi/src/lib.rscrates/skippy-runtime/src/lib.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/runtime_state.rsthird_party/llama.cpp/patches/0013-Add-non-frame-native-MTP-decode-ABI.patch
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/skippy-server/src/frontend/local_generation.rs
- crates/skippy-runtime/src/lib.rs
- crates/skippy-server/src/runtime_state.rs
ndizazzo
force-pushed
the
feature/gpu-tune-cli
branch
from
July 3, 2026 20:02
dac86ff to
cf7da5b
Compare
ndizazzo
force-pushed
the
feature/gpu-tune-cli
branch
from
July 3, 2026 23:28
cf7da5b to
b0b67d2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-commands/src/gpus/tune/benchmark.rs (1)
1-2262: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit this Rust source file below the 2,000-line limit.
This new file is 2,262 lines. Please split it by responsibility, for example candidate generation, trial process orchestration, trial config rendering, and tests. As per coding guidelines, "Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs` around lines 1 - 2262, The benchmark.rs module is over the 2,000-line limit and should be split by responsibility into smaller owning modules. Move candidate generation helpers like benchmark_candidates and benchmark_speculative_values into one module, trial orchestration like run_trial_inner, wait_for_trial_ready, and TrialChild into another, and config rendering helpers like trial_config and apply_speculative_overrides into a third. Keep run_benchmark_plans as the public entry point and update imports/re-exports so the existing benchmark_tests and helper symbols still resolve cleanly after the module split.Source: Coding guidelines
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_runner.rs`:
- Around line 75-94: The benchmark launch in tune_runner is happening before the
global safety guard, so expensive benchmark trials can start even when the
command will abort. Update the control flow in the tune_runner logic to check
global_safety_errors before calling maybe_run_benchmark_reports, and if blockers
exist, emit the runner output immediately using RunnerOutputContext with an
empty benchmark_reports collection. Keep the existing prepared, target_failures,
and output formatting behavior unchanged otherwise.
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs`:
- Around line 751-756: The trial directory name in the benchmark setup is only
using epoch seconds plus the trial index, which can collide across concurrent
runs. Update the directory naming logic in the benchmark/tune code that builds
`dir` to add stronger uniqueness, such as nanoseconds from `SystemTime`, the
current process ID, or a random suffix, while keeping the existing index
component so trial folders remain traceable.
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs`:
- Around line 193-208: The speculative config validation in the resolver is
incorrectly rejecting benchmark-tuned fields, causing startup failure for
MTP/Draft trials. Update the checks in the speculative resolver logic around
pick_owned and unsupported_speculative_field so that draft_acceptance_threshold
and draft_split_probability from benchmark-generated trial configs are allowed
through instead of being treated as unsupported, while preserving rejection only
for truly unsupported user-provided speculative fields.
---
Outside diff comments:
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs`:
- Around line 1-2262: The benchmark.rs module is over the 2,000-line limit and
should be split by responsibility into smaller owning modules. Move candidate
generation helpers like benchmark_candidates and benchmark_speculative_values
into one module, trial orchestration like run_trial_inner, wait_for_trial_ready,
and TrialChild into another, and config rendering helpers like trial_config and
apply_speculative_overrides into a third. Keep run_benchmark_plans as the public
entry point and update imports/re-exports so the existing benchmark_tests and
helper symbols still resolve cleanly after the module split.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d497b3b1-b7cf-423a-836c-695c96cf14b0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (130)
.agents/skills/benchmark-tune/SKILL.md.agents/skills/benchmark-tune/agents/openai.yamlCargo.tomlcrates/llama-spec-bench/src/main.rscrates/mesh-llm-cli/src/benchmark.rscrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-commands/Cargo.tomlcrates/mesh-llm-commands/src/benchmark.rscrates/mesh-llm-commands/src/gpus.rscrates/mesh-llm-commands/src/gpus/tune/apply_collision_tests.rscrates/mesh-llm-commands/src/gpus/tune/apply_test_support.rscrates/mesh-llm-commands/src/gpus/tune/apply_write_tests.rscrates/mesh-llm-commands/src/gpus/tune/benchmark.rscrates/mesh-llm-commands/src/gpus/tune/benchmark_progress.rscrates/mesh-llm-commands/src/gpus/tune/benchmark_selection.rscrates/mesh-llm-commands/src/gpus/tune/matrix.rscrates/mesh-llm-commands/src/gpus/tune/metadata.rscrates/mesh-llm-commands/src/gpus/tune/metadata_tests.rscrates/mesh-llm-commands/src/gpus/tune/output_emit.rscrates/mesh-llm-commands/src/gpus/tune/output_launch.rscrates/mesh-llm-commands/src/gpus/tune/output_render.rscrates/mesh-llm-commands/src/gpus/tune/output_report.rscrates/mesh-llm-commands/src/gpus/tune/output_tests.rscrates/mesh-llm-commands/src/gpus/tune/output_types.rscrates/mesh-llm-commands/src/gpus/tune/output_values.rscrates/mesh-llm-commands/src/gpus/tune/planning.rscrates/mesh-llm-commands/src/gpus/tune/recommendation.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_defaults_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_existing.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_failure_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_reports.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_writes.rscrates/mesh-llm-commands/src/gpus/tune/tests.rscrates/mesh-llm-commands/src/gpus/tune/types.rscrates/mesh-llm-commands/src/gpus/tune_apply.rscrates/mesh-llm-commands/src/gpus/tune_hardware.rscrates/mesh-llm-commands/src/gpus/tune_hardware/device_request.rscrates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rscrates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/helpers.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/mlock_reporting.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/selection.rscrates/mesh-llm-commands/src/gpus/tune_hardware/types.rscrates/mesh-llm-commands/src/gpus/tune_resolver.rscrates/mesh-llm-commands/src/gpus/tune_resolver/tests.rscrates/mesh-llm-commands/src/gpus/tune_resolver/types.rscrates/mesh-llm-commands/src/gpus/tune_runner.rscrates/mesh-llm-commands/src/gpus/tune_runner_tests.rscrates/mesh-llm-config/src/model.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-config/src/validate_gpu_tune_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/deployment.rscrates/mesh-llm-host-runtime/src/inference/skippy/family_policy.rscrates/mesh-llm-host-runtime/src/inference/skippy/materialization.rscrates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/test_support.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/types.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/types.rscrates/mesh-llm-host-runtime/src/mesh/mod.rscrates/mesh-llm-host-runtime/src/mesh/stage_proto.rscrates/mesh-llm-host-runtime/src/mesh/tests.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-system/src/benchmark.rscrates/mesh-llm-system/src/lib.rscrates/mesh-llm-system/src/util.rscrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/src/lib.rscrates/mesh-llm/tests/cli_errors.rscrates/model-artifact/src/gguf.rscrates/skippy-bench/src/distributed.rscrates/skippy-bench/src/local_split.rscrates/skippy-bench/src/token_lengths.rscrates/skippy-bench/src/verify_span_local.rscrates/skippy-correctness/src/report.rscrates/skippy-correctness/src/runner.rscrates/skippy-correctness/tests/parity_models/mod.rscrates/skippy-ffi/src/lib.rscrates/skippy-model-package/src/main.rscrates/skippy-model-package/src/preflight.rscrates/skippy-prompt/src/prompt_cli/args.rscrates/skippy-prompt/src/prompt_cli/binary_repl.rscrates/skippy-prompt/src/prompt_cli/draft.rscrates/skippy-prompt/src/prompt_cli/launch.rscrates/skippy-prompt/src/prompt_cli/stage_config.rscrates/skippy-protocol/proto/stage.protocrates/skippy-protocol/src/lib.rscrates/skippy-runtime/src/lib.rscrates/skippy-server/src/binary_transport.rscrates/skippy-server/src/binary_transport/forwarding.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/binary_transport/tests.rscrates/skippy-server/src/cli.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/embedded_execution.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/native_mtp/decode.rscrates/skippy-server/src/frontend/native_mtp/draft.rscrates/skippy-server/src/frontend/native_mtp/mod.rscrates/skippy-server/src/frontend/native_mtp/stats.rscrates/skippy-server/src/frontend/native_mtp/verifier.rscrates/skippy-server/src/frontend/prefix_cache.rscrates/skippy-server/src/frontend/speculative.rscrates/skippy-server/src/frontend/tests.rscrates/skippy-server/src/kv_integration/activation.rscrates/skippy-server/src/kv_integration/config.rscrates/skippy-server/src/kv_integration/identity.rscrates/skippy-server/src/runtime_state.rsdocs/CLI.mddocs/USAGE.mddocs/design/GLM_NATIVE_MTP_SKIPPY_ARCHITECTURE.mddocs/design/TESTING.mddocs/skippy/CONFIGURATION.mddocs/specs/layer-package-repos.mddocs/specs/speculative-decoding-wiring-plan.mdscripts/build-llama.shthird_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patchthird_party/llama.cpp/patches/0012-Wire-mmap-and-mlock-runtime-load-options.patchthird_party/llama.cpp/patches/0013-Add-external-MTP-draft-sidecar-attachment.patchthird_party/llama.cpp/patches/0014-Add-non-frame-native-MTP-decode-ABI.patchwebsite/src/docs/pages/CLI.md
💤 Files with no reviewable changes (17)
- crates/skippy-server/src/kv_integration/activation.rs
- docs/specs/layer-package-repos.md
- crates/skippy-server/src/kv_integration/config.rs
- scripts/build-llama.sh
- website/src/docs/pages/CLI.md
- docs/specs/speculative-decoding-wiring-plan.md
- docs/design/TESTING.md
- crates/skippy-server/src/kv_integration/identity.rs
- docs/skippy/CONFIGURATION.md
- third_party/llama.cpp/patches/0014-Add-non-frame-native-MTP-decode-ABI.patch
- docs/CLI.md
- third_party/llama.cpp/patches/0012-Wire-mmap-and-mlock-runtime-load-options.patch
- docs/design/GLM_NATIVE_MTP_SKIPPY_ARCHITECTURE.md
- third_party/llama.cpp/patches/0013-Add-external-MTP-draft-sidecar-attachment.patch
- crates/skippy-server/src/runtime_state.rs
- docs/USAGE.md
- third_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patch
✅ Files skipped from review due to trivial changes (7)
- crates/skippy-server/src/cli.rs
- .agents/skills/benchmark-tune/agents/openai.yaml
- crates/mesh-llm-system/src/lib.rs
- crates/skippy-server/src/binary_transport/forwarding.rs
- crates/skippy-prompt/src/prompt_cli/binary_repl.rs
- crates/mesh-llm-system/src/benchmark.rs
- crates/skippy-server/src/binary_transport/tests.rs
🚧 Files skipped from review as they are similar to previous changes (97)
- crates/mesh-llm-commands/src/gpus/tune_hardware/tests/mlock_reporting.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_failure_tests.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/family_policy.rs
- crates/mesh-llm-commands/src/gpus/tune/benchmark_progress.rs
- crates/skippy-prompt/src/prompt_cli/launch.rs
- crates/mesh-llm-config/src/validate_gpu_tune_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/output_emit.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/stage/types.rs
- crates/skippy-protocol/proto/stage.proto
- crates/mesh-llm-commands/src/gpus/tune/tests.rs
- crates/mesh-llm-commands/src/gpus/tune_resolver/types.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.rs
- crates/mesh-llm/src/commands/mod.rs
- crates/mesh-llm-commands/src/gpus/tune/apply_collision_tests.rs
- Cargo.toml
- crates/mesh-llm-commands/src/gpus/tune/apply_write_tests.rs
- crates/skippy-prompt/src/prompt_cli/args.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_reports.rs
- crates/mesh-llm-commands/src/gpus/tune_resolver/tests.rs
- crates/skippy-server/src/frontend/speculative.rs
- crates/mesh-llm-commands/src/gpus/tune/apply_test_support.rs
- crates/skippy-server/src/binary_transport/options.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/stage/tests.rs
- crates/mesh-llm/tests/cli_errors.rs
- crates/skippy-server/src/frontend/embedded_execution.rs
- crates/skippy-server/src/frontend/generation_flow.rs
- crates/llama-spec-bench/src/main.rs
- crates/skippy-bench/src/local_split.rs
- crates/mesh-llm-host-runtime/src/mesh/tests.rs
- crates/skippy-correctness/tests/parity_models/mod.rs
- crates/mesh-llm-commands/src/gpus/tune/output_tests.rs
- crates/skippy-bench/src/token_lengths.rs
- crates/skippy-prompt/src/prompt_cli/stage_config.rs
- crates/mesh-llm-system/src/util.rs
- crates/skippy-server/src/frontend/native_mtp/mod.rs
- crates/skippy-server/src/frontend/native_mtp/decode.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/deployment.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/device_request.rs
- crates/skippy-prompt/src/prompt_cli/draft.rs
- crates/mesh-llm-commands/src/benchmark.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/materialization.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_existing.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/types.rs
- crates/mesh-llm-commands/src/gpus/tune/output_types.rs
- crates/skippy-bench/src/distributed.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/tests/helpers.rs
- crates/mesh-llm-commands/src/gpus/tune/output_report.rs
- crates/mesh-llm-host-runtime/src/runtime/local.rs
- crates/mesh-llm-commands/src/gpus/tune/output_launch.rs
- crates/skippy-protocol/src/lib.rs
- crates/mesh-llm-commands/src/gpus/tune/metadata_tests.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/tests/selection.rs
- crates/mesh-llm-cli/src/benchmark.rs
- crates/skippy-server/src/frontend/native_mtp/stats.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_writes.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/test_support.rs
- crates/mesh-llm-commands/src/gpus/tune_resolver.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs
- crates/skippy-model-package/src/main.rs
- crates/mesh-llm-commands/Cargo.toml
- crates/mesh-llm-commands/src/gpus/tune/matrix.rs
- crates/mesh-llm-cli/src/parser.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/types.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation.rs
- crates/skippy-server/src/frontend/prefix_cache.rs
- crates/mesh-llm-host-runtime/src/mesh/mod.rs
- crates/mesh-llm-commands/src/gpus/tune/output_render.rs
- crates/skippy-model-package/src/preflight.rs
- crates/mesh-llm-commands/src/gpus/tune/types.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_defaults_tests.rs
- crates/skippy-server/src/frontend/native_mtp/draft.rs
- crates/mesh-llm-commands/src/gpus/tune/planning.rs
- crates/mesh-llm-commands/src/gpus/tune_apply.rs
- crates/mesh-llm-commands/src/gpus.rs
- crates/skippy-server/src/binary_transport.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rs
- crates/skippy-server/src/frontend/local_generation.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rs
- crates/model-artifact/src/gguf.rs
- crates/mesh-llm-commands/src/gpus/tune/output_values.rs
- crates/mesh-llm-commands/src/gpus/tune/benchmark_selection.rs
- crates/skippy-server/src/frontend/tests.rs
- crates/mesh-llm-commands/src/gpus/tune/metadata.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rs
- crates/skippy-bench/src/verify_span_local.rs
- crates/skippy-server/src/frontend.rs
- crates/skippy-server/src/frontend/embedded_generation.rs
- crates/skippy-ffi/src/lib.rs
- crates/skippy-runtime/src/lib.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/mod.rs
- crates/skippy-server/src/frontend/native_mtp/verifier.rs
- crates/skippy-correctness/src/runner.rs
- crates/mesh-llm-host-runtime/src/mesh/stage_proto.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/mesh-llm-config/src/model.rs (1)
542-670: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftField duplication between
SpeculativeConfigandSpeculativeConfigRawis a sync-drift risk.Every field is duplicated across the two structs. Adding a new field later and forgetting to mirror it into
SpeculativeConfigRaw(which hasdeny_unknown_fields) will surface as a confusing "unknown field" TOML parse error for end users instead of a compile error. Consider deriving both via a shared field-list macro, or moving to#[serde(alias = "draft_model_path")]directly onSpeculativeConfig::draft_modelcombined with a post-deserialize hook (e.g. via#[serde(deserialize_with)]on just that field) if the "both keys set" rejection can be expressed there, to reduce the duplicated surface.🤖 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 `@crates/mesh-llm-config/src/model.rs` around lines 542 - 670, The issue is that `SpeculativeConfig` and `SpeculativeConfigRaw` duplicate the same field list, creating a sync-drift risk that can turn new fields into confusing `unknown field` parse errors. Refactor the deserialization path in `SpeculativeConfig`/`SpeculativeConfigRaw` so the field set is defined in one place, either by sharing a macro-generated struct definition or by using `serde(alias = "draft_model_path")` on `draft_model` with a custom deserialization hook to preserve the legacy-key conflict check. Keep the legacy handling and the “both keys set” rejection in `Deserialize for SpeculativeConfig`, but eliminate the duplicated per-field declarations.
🤖 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 `@crates/mesh-llm-config/src/validate.rs`:
- Around line 184-213: The `draft_model_path` deprecation in `validate.rs`
currently points users to `draft_model`, but `validate_model_identifier` still
rejects bare local paths unless `legacy_path_used` is set, creating a dead end.
Update the validation flow so `draft_model` can accept path-shaped values or,
alternatively, make the alias warning in `alias_diagnostic` conditional in
`validate`/`validate_model_identifier` so `draft_model_path` is only deprecated
for identifier-like values and local-path draft models keep working.
- Around line 190-212: The model path construction in the diagnostics uses a
bracketed string inside ConfigPath::from_fields, which produces the wrong
rendered path format. Update the path building in the validation loop over
config.models so the model prefix is created with push_index(index) or an
equivalent helper, and use that same indexed prefix for both the
draft_model_path and draft_model paths in alias_diagnostic.
---
Nitpick comments:
In `@crates/mesh-llm-config/src/model.rs`:
- Around line 542-670: The issue is that `SpeculativeConfig` and
`SpeculativeConfigRaw` duplicate the same field list, creating a sync-drift risk
that can turn new fields into confusing `unknown field` parse errors. Refactor
the deserialization path in `SpeculativeConfig`/`SpeculativeConfigRaw` so the
field set is defined in one place, either by sharing a macro-generated struct
definition or by using `serde(alias = "draft_model_path")` on `draft_model` with
a custom deserialization hook to preserve the legacy-key conflict check. Keep
the legacy handling and the “both keys set” rejection in `Deserialize for
SpeculativeConfig`, but eliminate the duplicated per-field declarations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e21f99b-8bb1-43bb-9ad1-ffbf5b8817ea
📒 Files selected for processing (18)
crates/mesh-llm-commands/src/gpus/tune/benchmark.rscrates/mesh-llm-commands/src/gpus/tune/output_types.rscrates/mesh-llm-commands/src/gpus/tune/output_values.rscrates/mesh-llm-config/src/model.rscrates/mesh-llm-config/src/model/built_in_schema.rscrates/mesh-llm-config/src/model/built_in_schema/control_behavior/speculative.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/runtime/config_state.rscrates/mesh-llm-host-runtime/tests/fixtures/config_schema_defaults_ui_reference.jsoncrates/mesh-llm-ui/src/features/configuration/api/config-adapter.test.tscrates/mesh-llm-ui/src/features/configuration/api/config-adapter.tscrates/mesh-llm-ui/src/features/configuration/lib/build-toml.test.tscrates/mesh-llm-ui/src/features/configuration/lib/build-toml.tsdocs/CLI.mddocs/USAGE.mddocs/skippy/CONFIGURATION.mddocs/specs/speculative-decoding-wiring-plan.md
✅ Files skipped from review due to trivial changes (2)
- docs/specs/speculative-decoding-wiring-plan.md
- docs/CLI.md
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/mesh-llm-commands/src/gpus/tune/output_values.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs
- crates/mesh-llm-commands/src/gpus/tune/output_types.rs
- crates/mesh-llm-commands/src/gpus/tune/benchmark.rs
- docs/USAGE.md
ndizazzo
force-pushed
the
feature/gpu-tune-cli
branch
from
July 5, 2026 06:59
20185a3 to
99ef0b9
Compare
ndizazzo
force-pushed
the
feature/gpu-tune-cli
branch
from
July 5, 2026 18:42
63a923a to
f9b93be
Compare
Collaborator
Author
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/skippy-server/src/frontend/embedded_generation.rs (1)
866-943: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftGrowing an already ~1800-line function further.
generate_embedded_stage_zero_tokensspans roughly lines 3-1802 in this file, and this PR adds several more non-trivial decision blocks to it (native MTP batched-draft materialization/verification at 866-943, ngram proposal fallback at 1154-1183, plus new telemetry fields). Verified the added logic itself (draft/target alignment, commit-count bounds, clone-vs-move usage) and found no correctness bug, but the method is far past any reasonable line-count/cognitive-complexity threshold and each addition compounds that.Consider extracting the native-MTP batched-verify block and the draft/ngram-proposal block into semantically named helpers (e.g.
run_native_mtp_batched_verify(...),propose_speculative_tokens(...)) to keep this method within the configured Clippy limits.As per coding guidelines, "Do not add Rust methods or functions over the configured Clippy line-count limit. Split long logic into semantically named helpers before it reaches the configured
too_many_linesthreshold" and "Do not add Rust code over the configured cognitive-complexity limit. Prefer small, named decision helpers and clear control-flow phases instead of nested branching."Also applies to: 1154-1183
🤖 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 `@crates/skippy-server/src/frontend/embedded_generation.rs` around lines 866 - 943, The issue is that generate_embedded_stage_zero_tokens is growing beyond the configured Clippy line-count and cognitive-complexity limits. Extract the new native MTP batched verification flow and the ngram/draft proposal fallback into small, semantically named helpers such as run_native_mtp_batched_verify and propose_speculative_tokens, and keep the main function focused on orchestration using those helpers. Preserve the current behavior by moving the existing logic from the native_mtp pending draft block and the ngram proposal block into those helpers, using the same symbols and control-flow boundaries already present in generate_embedded_stage_zero_tokens.Source: Coding guidelines
crates/skippy-correctness/src/runner.rs (1)
2862-2949: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExtract the new native-MTP report helpers from the oversized runner.
This file is already well over the 2,000-line Rust source limit, and this PR adds more separable parsing/verification helpers here. Move the new native-MTP report/sideband helpers and their unit tests into a focused module before merge. As per coding guidelines, "
**/*.rs: Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."🤖 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 `@crates/skippy-correctness/src/runner.rs` around lines 2862 - 2949, The new native-MTP parsing and verification helpers are being added to an already oversized runner module, so extract them into a dedicated owning module instead of keeping them in runner.rs. Move native_mtp_verification_report, native_mtp_verification_satisfies_requirement, and native_mtp_sideband_report together with their unit tests into a focused module, then update runner to use those symbols from the new module.Source: Coding guidelines
crates/skippy-runtime/src/lib.rs (1)
1263-1287: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove the new native-MTP runtime plumbing out of oversized
lib.rs.These additions are semantically cohesive and this file is already far beyond the 2,000-line limit. Split the new MTP draft conversion/attach/decode helpers into an owning runtime/native-MTP module and keep
lib.rsas a thinner facade. As per coding guidelines, "**/*.rs: Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."Also applies to: 1769-1795, 2865-2901, 3278-3423
🤖 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 `@crates/skippy-runtime/src/lib.rs` around lines 1263 - 1287, The new native-MTP plumbing is being added to an already oversized lib.rs, so move the draft conversion/attach/decode logic into a dedicated runtime/native-MTP module and keep lib.rs as a thin facade. Relocate the NativeMtpDraft type and its from_raw helper, along with the related attach/decode helpers referenced elsewhere in the diff, into the new owning module and re-export only what lib.rs needs. Make sure all call sites continue to use the same symbols after the split so behavior stays unchanged.Source: Coding guidelines
crates/mesh-llm-commands/src/gpus/tune/benchmark.rs (1)
1-2330: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit this new benchmark runner before merge.
This new Rust file is 2,330 lines, exceeding the workspace limit. Split by responsibility, e.g. request/trial execution, trial config rendering, speculative candidate generation, selection tests, and process/readiness helpers. As per coding guidelines, "
**/*.rs: Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs` around lines 1 - 2330, This benchmark runner file is oversized and exceeds the Rust source limit, so it should be split into smaller modules before merge. Move the responsibilities in TuneBenchmarkRunRequest, run_benchmark_plans, run_trial_inner, trial_config, speculative candidate generation, and selection/test helpers into owning modules with clear boundaries. Keep the public entrypoint in benchmark.rs and re-export or delegate to modules for trial execution, config rendering, speculative sweep generation, process/readiness helpers, and tests so the file stays under the limit.Source: Coding guidelines
♻️ Duplicate comments (1)
crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs (1)
142-228: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
pinned_diagnostic_allows_backend_fallbackis always true — gating logic is dead code.
missing_configured_deviceunconditionally appends"available pinnable GPU IDs: {}"to every message it builds (line 222), andresolve_requested_pinned_gpu's only error path goes through it. Sopinned_diagnostic_allows_backend_fallback's.contains("available pinnable GPU IDs")check is always satisfied, making the early-return-without-fallback branch at lines 133-135 unreachable. Backend-device fallback is effectively always attempted forModelHardwareDevice/DefaultsHardwareDevice, which may not be the intended selective-fallback behavior. This is the same string-matching fragility flagged in the prior review, now surfacing as an always-true (rather than silently-false) condition.🐛 Possible direction: gate on the underlying error before formatting the full diagnostic
fn resolve_requested_pinned_gpu<'a>( request: &ConfiguredTuneDeviceRequest, survey: &'a HardwareSurvey, ) -> Result<&'a GpuFacts, TuneDiagnostic> { - resolve_pinned_gpu_strict(Some(&request.requested_value), &survey.gpus) - .map_err(|error| missing_configured_device(request, survey, &error.to_string())) + resolve_pinned_gpu_strict(Some(&request.requested_value), &survey.gpus).map_err(|error| { + let allows_backend_fallback = /* structured check on `error`, not the final message */; + (missing_configured_device(request, survey, &error.to_string()), allows_backend_fallback) + }) }Use a structured signal from
resolve_pinned_gpu_strict's error type (if available) instead of substring-matching the fully-formatted diagnostic message.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs` around lines 142 - 228, The fallback gate in pinned_diagnostic_allows_backend_fallback is dead because missing_configured_device always includes the “available pinnable GPU IDs” text, so the substring check always passes. Update the fallback decision to use a structured signal from resolve_requested_pinned_gpu/resolve_pinned_gpu_strict (or the underlying error kind) instead of inspecting the formatted TuneDiagnostic.message, and keep combine_backend_fallback_error and resolve_backend_device unchanged except for wiring in the new gating result.
🧹 Nitpick comments (10)
crates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rs (1)
5-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the reason for
#[allow(dead_code)].These annotations appear justified because
Supported/TuneMlockLimitare only constructed under#[cfg(target_os = "linux")], making them genuinely unused on other platforms. A brief comment would make that reasoning explicit for future readers, since the guideline expects a documented "clear reason" for suppressing warnings.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rs` around lines 5 - 17, Document the `#[allow(dead_code)]` suppressions on `TuneMlockProbe::Supported` and `TuneMlockLimit` with a brief comment explaining that these variants are only constructed on Linux via `#[cfg(target_os = "linux")]`, so they are intentionally unused on other platforms. Add the explanation near the enum definitions in `mlock.rs` so future readers can see why the warning is being silenced.Source: Coding guidelines
crates/mesh-llm-commands/src/gpus.rs (3)
51-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_config_pathis unused.Unlike the benchmark dispatch path (which threads
config_pathintorun_benchmark_tune_command),dispatch_gpu_commandreceives but never uses it. Fine if this is deliberate scaffolding for future work, otherwise consider dropping the parameter until needed.🤖 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 `@crates/mesh-llm-commands/src/gpus.rs` around lines 51 - 55, The dispatch_gpu_command signature takes _config_path but never uses it, so either remove the parameter if it is not needed yet or thread it into the GPU command flow the same way dispatch_benchmark_command passes config_path into run_benchmark_tune_command. Update the dispatch_gpu_command function and any call sites so the parameter is either intentionally omitted or actually consumed.
45-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
#[path]attributes.
gpus.rsis a non-mod.rsfile, somod tune_hardware;/mod tune_runner;already resolve togpus/tune_hardware.rs/gpus/tune_runner.rsby default (astune_apply/tune_resolverbelow correctly rely on). The explicit#[path = ...]attributes here just restate the default and can be dropped.🧹 Proposed cleanup
-#[path = "gpus/tune_hardware.rs"] pub(crate) mod tune_hardware; pub(crate) mod tune_resolver; -#[path = "gpus/tune_runner.rs"] pub(crate) mod tune_runner;🤖 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 `@crates/mesh-llm-commands/src/gpus.rs` around lines 45 - 49, Remove the redundant #[path] attributes from the gpus.rs module declarations and let the default module resolution handle tune_hardware and tune_runner, matching how tune_apply and tune_resolver are declared. Update the module list so tune_hardware and tune_runner are plain pub(crate) mod declarations without changing their locations or behavior.
11-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider standard submodules instead of
include!-based composition.Splicing
gpus/tune/*.rsfiles into a singlemod tune { include!(...) }block bypasses Rust's normal module system (no per-file privacy boundaries, flat shared namespace across all included files, unusual for tooling). Standardmod types; mod matrix; ...withpub(crate)re-exports (or atune/mod.rs) would achieve the same file-size-limit goal more idiomatically.🤖 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 `@crates/mesh-llm-commands/src/gpus.rs` around lines 11 - 42, Replace the include!-based composition inside the tune module with standard Rust submodules so the code uses normal module boundaries instead of one flat shared namespace. Refactor the gpus::tune structure to use mod declarations (or a tune/mod.rs layout) for the existing symbols like types, matrix, metadata, benchmark, planning, recommendation, and the test modules, then add pub(crate) re-exports where needed to preserve the current public API.crates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rs (1)
107-124: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSilent fallback when path canonicalization fails on either side.
comparable_pathswallows canonicalization errors and falls back to the raw path, so if the requested path and a configuredhardware.model_pathdiffer in form (relative vs absolute, symlink vs target) and one side can't be canonicalized (e.g. file not yet materialized), the match silently fails rather than surfacing a diagnostic. This only affects the fallback lookup path (primary lookup is bymodel_id), so impact is bounded to local/path-based model configs whosemodel_iddoesn't match the config entry'smodelkey.🤖 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 `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rs` around lines 107 - 124, The fallback path matching in find_model_entry_by_resolved_path/comparable_path silently hides canonicalization failures, so update the lookup to surface a diagnostic when canonicalization cannot be performed on either the requested path or hardware.model_path. Keep the fallback lookup behavior in resolution.rs, but add logging or error reporting around comparable_path so mismatched relative/absolute or symlinked paths are not silently ignored. Use the existing find_model_entry_by_resolved_path and comparable_path symbols to locate the change, and make sure the fallback still returns the same ModelConfigEntry only after the path comparison succeeds.docs/specs/layer-package-repos.md (1)
311-314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting legacy value compatibility.
The docs no longer mention
native-mtp-n1, but the PR states legacy values are normalized tomtpat runtime. Operators with existingnative-mtp-n1configs may not realize the value still works after this rename. Consider a short compatibility note.🤖 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 `@docs/specs/layer-package-repos.md` around lines 311 - 314, Add a short compatibility note in the `speculative.strategy` docs explaining that the legacy value `native-mtp-n1` is still accepted and normalized to `mtp` at runtime. Update the section describing supported values so operators with existing `native-mtp-n1` configs know it remains valid, and reference `speculative.strategy` and the legacy rename in the same paragraph.crates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rs (1)
376-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the native MTP token-count fallback into a named constant.
native_mtp_max_tokens: if native_mtp_enabled { 3 } else { 0 }duplicates a magic literal in two constructors. Other defaults in this file (e.g.,BUILTIN_PREFILL_CHUNK_SIZE) use named constants for clarity.♻️ Proposed fix
+const BUILTIN_NATIVE_MTP_MAX_TOKENS: usize = 3; + ... - native_mtp_max_tokens: if native_mtp_enabled { 3 } else { 0 }, + native_mtp_max_tokens: if native_mtp_enabled { BUILTIN_NATIVE_MTP_MAX_TOKENS } else { 0 },Also applies to: 412-417
🤖 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 `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rs` around lines 376 - 381, The native MTP max-token fallback is using a hardcoded literal in multiple constructor defaults, so replace the `if native_mtp_enabled { 3 } else { 0 }` value in the translation resolver defaults with a named constant. Define and reuse a clear constant alongside the other file-level defaults in `translation.rs`, and update both constructor sites that set `native_mtp_max_tokens` to reference it instead of the magic number.crates/mesh-llm-commands/src/gpus/tune/metadata_tests.rs (1)
14-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueManual temp-file lifecycle instead of
tempfilefor RAII cleanup.
temp_file_path/write_byteswrite fixtures into the system temp dir and rely on an explicitfs::remove_fileat the end of each test. Anyexpect()/assert!panic earlier in the test body (several are chained before cleanup) will leak the file. Other tests in this same PR usetempfile::tempdir(), which cleans up automatically on drop even on panic.Also applies to: 88-112, 153-182, 184-213
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/metadata_tests.rs` around lines 14 - 29, The fixture helpers `temp_file_path` and `write_bytes` are manually creating files in the system temp dir, which can leak when tests panic before `fs::remove_file` runs. Replace this pattern in `metadata_tests.rs` with RAII-backed temp storage (for example `tempfile::tempdir()` or `NamedTempFile`) and update the affected tests that currently call `temp_file_path`/`write_bytes` so cleanup happens automatically even on early `expect`/`assert` failures.crates/mesh-llm-commands/src/gpus/tune_hardware/tests/selection.rs (1)
1-234: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for the pinned→backend-device fallback path.
None of these tests exercise
resolve_pinned_with_backend_fallback(pinned lookup fails, backend fallback succeeds or both fail and errors combine). Adding such a case would have caught the always-true gating issue flagged inevaluate.rs.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_hardware/tests/selection.rs` around lines 1 - 234, The current GPU tuning test suite does not cover the pinned-to-backend fallback path in `resolve_pinned_with_backend_fallback`, so add a test in this selection module that forces pinned lookup to fail and verifies the backend-device fallback succeeds, plus a failure case where both sources fail and the combined error is asserted. Use the existing helpers like `evaluate_with_probe`, `sample_gpu`, and the `TuneDeviceSelectionSource`/`TuneGpuTarget` assertions to keep the new case aligned with the selection flow. This will exercise the `evaluate.rs` gating path and catch regressions in fallback behavior.crates/skippy-server/src/frontend/native_mtp/verifier.rs (1)
43-79: 📐 Maintainability & Code Quality | 🔵 TrivialAdd unit test coverage for
observe_taken_draft_span.This new batched span-verification helper (early-exit on rejection, first-pair-only compute accounting) is exercised only indirectly if at all — the existing tests cover
observe_taken_draft_verification, not this function. Given its branching logic (index-0 special-casing, early break on rejection or missing target), a few direct unit tests (full accept, partial accept then reject, target shorter than draft) would guard against regressions.🤖 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 `@crates/skippy-server/src/frontend/native_mtp/verifier.rs` around lines 43 - 79, Add direct unit test coverage for observe_taken_draft_span in NativeMtpVerifier, since its batched logic is not covered by observe_taken_draft_verification. Create tests that exercise the index-0 compute_us special case, full acceptance across all tokens, early exit on a Rejected decision, and stopping when target_tokens is shorter than draft_tokens. Assert the returned NativeMtpSpanVerification fields accepted_count, rejected, and first_decision for each case.
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune_runner.rs`:
- Around line 29-171: The function run_tune_request_with_writer is still too
long and cognitively complex because it mixes resolution, safety handling,
benchmarking, and apply-mode write/bail logic in one body. Extract the global
safety error path into a named helper such as bail_on_global_safety_errors, and
move the apply-mode decision block into a helper such as handle_apply_mode so
the main flow stays as clear phases. Keep the existing symbols like
emit_runner_output_for, prepared, target_failures, and apply_mode centralized in
the helpers to reduce branching and keep the function under the configured
complexity/line limits.
In `@crates/mesh-llm-commands/src/gpus/tune/output_render.rs`:
- Around line 194-205: The Raw best `writeln!` block in `output_render.rs` is
misindented and appears unformatted compared with the surrounding code. Reformat
the `if let Some(raw_best) = &benchmark.raw_best` section so the `writeln!`
arguments and chained calls like `render_benchmark_candidate`,
`decode_tok_s.map`, and `render_timing_summary` follow the same style as the
nearby rendering blocks; this should be handled by running the Rust formatter on
the touched code.
In `@crates/mesh-llm-commands/src/gpus/tune/planning.rs`:
- Around line 133-141: snap_context_length_down currently violates its “snap
down” behavior by falling back to MIN_AUTO_CONTEXT_LENGTH when no step is found,
which can round small values up. Update snap_context_length_down in planning.rs
so the fallback returns the original input value (or an equivalent lower bound
based on value) instead of the floor constant, while keeping the CONTEXT_STEPS
search logic intact. This preserves the contract for callers like
planned_context_length and prevents ctx_size from exceeding a small model’s
native context length.
In `@crates/mesh-llm-config/src/validate.rs`:
- Around line 187-190: The draft model validation in validate.rs is using a
colon check that misclassifies Windows paths as identifiers and leaves the
legacy path branch without NUL/control-character checks. Update the shared
identifier predicate used by the speculative draft_model logic so it does not
rely on contains(':'), and make sure the path-validation path always enforces
the same NUL/control-character validation before accepting a value. Apply the
same fix consistently in the related draft_model validation branches and the
shared helper used by the legacy path handling.
In `@crates/skippy-server/src/frontend/speculative.rs`:
- Around line 52-78: The no-match path in propose_ngram_tokens is doing an
expensive nested scan over match_len and candidate_start with repeated slice
comparisons, which can become cubic during decoding. Refactor the matching logic
in propose_ngram_tokens to use a bounded search window or a hashed/suffix-based
lookup so the function quickly rules out non-matches without checking every
possible candidate substring.
In
`@third_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patch`:
- Around line 1427-1429: The draft token clamping logic is forcing a minimum of
1 via draft_limit, which breaks max_draft_tokens == 0 requests. Update the draft
creation flow that uses max_draft_tokens and SKIPPY_NATIVE_MTP_MAX_DRAFT_TOKENS
so the clamped limit can be 0, and when it is, return the already initialized
empty draft instead of generating a token. Keep the existing upper-bound clamp,
but remove the forced nonzero minimum in the draft_limit calculation.
---
Outside diff comments:
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark.rs`:
- Around line 1-2330: This benchmark runner file is oversized and exceeds the
Rust source limit, so it should be split into smaller modules before merge. Move
the responsibilities in TuneBenchmarkRunRequest, run_benchmark_plans,
run_trial_inner, trial_config, speculative candidate generation, and
selection/test helpers into owning modules with clear boundaries. Keep the
public entrypoint in benchmark.rs and re-export or delegate to modules for trial
execution, config rendering, speculative sweep generation, process/readiness
helpers, and tests so the file stays under the limit.
In `@crates/skippy-correctness/src/runner.rs`:
- Around line 2862-2949: The new native-MTP parsing and verification helpers are
being added to an already oversized runner module, so extract them into a
dedicated owning module instead of keeping them in runner.rs. Move
native_mtp_verification_report, native_mtp_verification_satisfies_requirement,
and native_mtp_sideband_report together with their unit tests into a focused
module, then update runner to use those symbols from the new module.
In `@crates/skippy-runtime/src/lib.rs`:
- Around line 1263-1287: The new native-MTP plumbing is being added to an
already oversized lib.rs, so move the draft conversion/attach/decode logic into
a dedicated runtime/native-MTP module and keep lib.rs as a thin facade. Relocate
the NativeMtpDraft type and its from_raw helper, along with the related
attach/decode helpers referenced elsewhere in the diff, into the new owning
module and re-export only what lib.rs needs. Make sure all call sites continue
to use the same symbols after the split so behavior stays unchanged.
In `@crates/skippy-server/src/frontend/embedded_generation.rs`:
- Around line 866-943: The issue is that generate_embedded_stage_zero_tokens is
growing beyond the configured Clippy line-count and cognitive-complexity limits.
Extract the new native MTP batched verification flow and the ngram/draft
proposal fallback into small, semantically named helpers such as
run_native_mtp_batched_verify and propose_speculative_tokens, and keep the main
function focused on orchestration using those helpers. Preserve the current
behavior by moving the existing logic from the native_mtp pending draft block
and the ngram proposal block into those helpers, using the same symbols and
control-flow boundaries already present in generate_embedded_stage_zero_tokens.
---
Duplicate comments:
In `@crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs`:
- Around line 142-228: The fallback gate in
pinned_diagnostic_allows_backend_fallback is dead because
missing_configured_device always includes the “available pinnable GPU IDs” text,
so the substring check always passes. Update the fallback decision to use a
structured signal from resolve_requested_pinned_gpu/resolve_pinned_gpu_strict
(or the underlying error kind) instead of inspecting the formatted
TuneDiagnostic.message, and keep combine_backend_fallback_error and
resolve_backend_device unchanged except for wiring in the new gating result.
---
Nitpick comments:
In `@crates/mesh-llm-commands/src/gpus.rs`:
- Around line 51-55: The dispatch_gpu_command signature takes _config_path but
never uses it, so either remove the parameter if it is not needed yet or thread
it into the GPU command flow the same way dispatch_benchmark_command passes
config_path into run_benchmark_tune_command. Update the dispatch_gpu_command
function and any call sites so the parameter is either intentionally omitted or
actually consumed.
- Around line 45-49: Remove the redundant #[path] attributes from the gpus.rs
module declarations and let the default module resolution handle tune_hardware
and tune_runner, matching how tune_apply and tune_resolver are declared. Update
the module list so tune_hardware and tune_runner are plain pub(crate) mod
declarations without changing their locations or behavior.
- Around line 11-42: Replace the include!-based composition inside the tune
module with standard Rust submodules so the code uses normal module boundaries
instead of one flat shared namespace. Refactor the gpus::tune structure to use
mod declarations (or a tune/mod.rs layout) for the existing symbols like types,
matrix, metadata, benchmark, planning, recommendation, and the test modules,
then add pub(crate) re-exports where needed to preserve the current public API.
In `@crates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rs`:
- Around line 5-17: Document the `#[allow(dead_code)]` suppressions on
`TuneMlockProbe::Supported` and `TuneMlockLimit` with a brief comment explaining
that these variants are only constructed on Linux via `#[cfg(target_os =
"linux")]`, so they are intentionally unused on other platforms. Add the
explanation near the enum definitions in `mlock.rs` so future readers can see
why the warning is being silenced.
In `@crates/mesh-llm-commands/src/gpus/tune_hardware/tests/selection.rs`:
- Around line 1-234: The current GPU tuning test suite does not cover the
pinned-to-backend fallback path in `resolve_pinned_with_backend_fallback`, so
add a test in this selection module that forces pinned lookup to fail and
verifies the backend-device fallback succeeds, plus a failure case where both
sources fail and the combined error is asserted. Use the existing helpers like
`evaluate_with_probe`, `sample_gpu`, and the
`TuneDeviceSelectionSource`/`TuneGpuTarget` assertions to keep the new case
aligned with the selection flow. This will exercise the `evaluate.rs` gating
path and catch regressions in fallback behavior.
In `@crates/mesh-llm-commands/src/gpus/tune/metadata_tests.rs`:
- Around line 14-29: The fixture helpers `temp_file_path` and `write_bytes` are
manually creating files in the system temp dir, which can leak when tests panic
before `fs::remove_file` runs. Replace this pattern in `metadata_tests.rs` with
RAII-backed temp storage (for example `tempfile::tempdir()` or `NamedTempFile`)
and update the affected tests that currently call `temp_file_path`/`write_bytes`
so cleanup happens automatically even on early `expect`/`assert` failures.
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rs`:
- Around line 107-124: The fallback path matching in
find_model_entry_by_resolved_path/comparable_path silently hides
canonicalization failures, so update the lookup to surface a diagnostic when
canonicalization cannot be performed on either the requested path or
hardware.model_path. Keep the fallback lookup behavior in resolution.rs, but add
logging or error reporting around comparable_path so mismatched
relative/absolute or symlinked paths are not silently ignored. Use the existing
find_model_entry_by_resolved_path and comparable_path symbols to locate the
change, and make sure the fallback still returns the same ModelConfigEntry only
after the path comparison succeeds.
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rs`:
- Around line 376-381: The native MTP max-token fallback is using a hardcoded
literal in multiple constructor defaults, so replace the `if native_mtp_enabled
{ 3 } else { 0 }` value in the translation resolver defaults with a named
constant. Define and reuse a clear constant alongside the other file-level
defaults in `translation.rs`, and update both constructor sites that set
`native_mtp_max_tokens` to reference it instead of the magic number.
In `@crates/skippy-server/src/frontend/native_mtp/verifier.rs`:
- Around line 43-79: Add direct unit test coverage for observe_taken_draft_span
in NativeMtpVerifier, since its batched logic is not covered by
observe_taken_draft_verification. Create tests that exercise the index-0
compute_us special case, full acceptance across all tokens, early exit on a
Rejected decision, and stopping when target_tokens is shorter than draft_tokens.
Assert the returned NativeMtpSpanVerification fields accepted_count, rejected,
and first_decision for each case.
In `@docs/specs/layer-package-repos.md`:
- Around line 311-314: Add a short compatibility note in the
`speculative.strategy` docs explaining that the legacy value `native-mtp-n1` is
still accepted and normalized to `mtp` at runtime. Update the section describing
supported values so operators with existing `native-mtp-n1` configs know it
remains valid, and reference `speculative.strategy` and the legacy rename in the
same paragraph.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ed5410f-36aa-4765-9b4c-63e8afc5d967
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (138)
.agents/skills/benchmark-tune/SKILL.md.agents/skills/benchmark-tune/agents/openai.yamlCargo.tomlcrates/llama-spec-bench/src/main.rscrates/mesh-llm-cli/src/benchmark.rscrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-commands/Cargo.tomlcrates/mesh-llm-commands/src/benchmark.rscrates/mesh-llm-commands/src/gpus.rscrates/mesh-llm-commands/src/gpus/tune/apply_collision_tests.rscrates/mesh-llm-commands/src/gpus/tune/apply_test_support.rscrates/mesh-llm-commands/src/gpus/tune/apply_write_tests.rscrates/mesh-llm-commands/src/gpus/tune/benchmark.rscrates/mesh-llm-commands/src/gpus/tune/benchmark_progress.rscrates/mesh-llm-commands/src/gpus/tune/benchmark_selection.rscrates/mesh-llm-commands/src/gpus/tune/matrix.rscrates/mesh-llm-commands/src/gpus/tune/metadata.rscrates/mesh-llm-commands/src/gpus/tune/metadata_tests.rscrates/mesh-llm-commands/src/gpus/tune/output_emit.rscrates/mesh-llm-commands/src/gpus/tune/output_launch.rscrates/mesh-llm-commands/src/gpus/tune/output_render.rscrates/mesh-llm-commands/src/gpus/tune/output_report.rscrates/mesh-llm-commands/src/gpus/tune/output_tests.rscrates/mesh-llm-commands/src/gpus/tune/output_types.rscrates/mesh-llm-commands/src/gpus/tune/output_values.rscrates/mesh-llm-commands/src/gpus/tune/planning.rscrates/mesh-llm-commands/src/gpus/tune/recommendation.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_defaults_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_existing.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_failure_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_reports.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_writes.rscrates/mesh-llm-commands/src/gpus/tune/tests.rscrates/mesh-llm-commands/src/gpus/tune/types.rscrates/mesh-llm-commands/src/gpus/tune_apply.rscrates/mesh-llm-commands/src/gpus/tune_hardware.rscrates/mesh-llm-commands/src/gpus/tune_hardware/device_request.rscrates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rscrates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/helpers.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/mlock_reporting.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/selection.rscrates/mesh-llm-commands/src/gpus/tune_hardware/types.rscrates/mesh-llm-commands/src/gpus/tune_resolver.rscrates/mesh-llm-commands/src/gpus/tune_resolver/tests.rscrates/mesh-llm-commands/src/gpus/tune_resolver/types.rscrates/mesh-llm-commands/src/gpus/tune_runner.rscrates/mesh-llm-commands/src/gpus/tune_runner_tests.rscrates/mesh-llm-config/src/model.rscrates/mesh-llm-config/src/model/built_in_schema.rscrates/mesh-llm-config/src/model/built_in_schema/control_behavior/speculative.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-config/src/validate_gpu_tune_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/deployment.rscrates/mesh-llm-host-runtime/src/inference/skippy/family_policy.rscrates/mesh-llm-host-runtime/src/inference/skippy/materialization.rscrates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/test_support.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/types.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/types.rscrates/mesh-llm-host-runtime/src/mesh/mod.rscrates/mesh-llm-host-runtime/src/mesh/stage_proto.rscrates/mesh-llm-host-runtime/src/mesh/tests.rscrates/mesh-llm-host-runtime/src/runtime/config_state.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-host-runtime/tests/fixtures/config_schema_defaults_ui_reference.jsoncrates/mesh-llm-system/src/benchmark.rscrates/mesh-llm-system/src/lib.rscrates/mesh-llm-system/src/util.rscrates/mesh-llm-ui/src/features/configuration/api/config-adapter.test.tscrates/mesh-llm-ui/src/features/configuration/api/config-adapter.tscrates/mesh-llm-ui/src/features/configuration/lib/build-toml.test.tscrates/mesh-llm-ui/src/features/configuration/lib/build-toml.tscrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/src/lib.rscrates/mesh-llm/tests/cli_errors.rscrates/model-artifact/src/gguf.rscrates/skippy-bench/src/distributed.rscrates/skippy-bench/src/local_split.rscrates/skippy-bench/src/token_lengths.rscrates/skippy-bench/src/verify_span_local.rscrates/skippy-correctness/src/report.rscrates/skippy-correctness/src/runner.rscrates/skippy-correctness/tests/parity_models/mod.rscrates/skippy-ffi/src/lib.rscrates/skippy-model-package/src/main.rscrates/skippy-model-package/src/preflight.rscrates/skippy-prompt/src/prompt_cli/args.rscrates/skippy-prompt/src/prompt_cli/binary_repl.rscrates/skippy-prompt/src/prompt_cli/draft.rscrates/skippy-prompt/src/prompt_cli/launch.rscrates/skippy-prompt/src/prompt_cli/stage_config.rscrates/skippy-protocol/proto/stage.protocrates/skippy-protocol/src/lib.rscrates/skippy-runtime/src/lib.rscrates/skippy-server/src/binary_transport.rscrates/skippy-server/src/binary_transport/forwarding.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/binary_transport/tests.rscrates/skippy-server/src/cli.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/embedded_execution.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/native_mtp/decode.rscrates/skippy-server/src/frontend/native_mtp/draft.rscrates/skippy-server/src/frontend/native_mtp/mod.rscrates/skippy-server/src/frontend/native_mtp/stats.rscrates/skippy-server/src/frontend/native_mtp/verifier.rscrates/skippy-server/src/frontend/prefix_cache.rscrates/skippy-server/src/frontend/speculative.rscrates/skippy-server/src/frontend/tests.rscrates/skippy-server/src/kv_integration/activation.rscrates/skippy-server/src/kv_integration/config.rscrates/skippy-server/src/kv_integration/identity.rscrates/skippy-server/src/runtime_state.rsdocs/CLI.mddocs/USAGE.mddocs/design/GLM_NATIVE_MTP_SKIPPY_ARCHITECTURE.mddocs/design/TESTING.mddocs/skippy/CONFIGURATION.mddocs/specs/layer-package-repos.mddocs/specs/speculative-decoding-wiring-plan.mdscripts/build-llama.shthird_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patchthird_party/llama.cpp/patches/0012-Wire-mmap-and-mlock-runtime-load-options.patchthird_party/llama.cpp/patches/0013-Add-external-MTP-draft-sidecar-attachment.patchthird_party/llama.cpp/patches/0014-Add-non-frame-native-MTP-decode-ABI.patchwebsite/src/docs/pages/CLI.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
crates/mesh-llm-commands/src/gpus/tune/benchmark/candidates.rs (2)
672-717: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the three near-identical
recommended_*lookup helpers.
recommended_u32,recommended_bool, andrecommended_cache_typerepeat the samefield_statuses.iter().find_map(...)scan, differing only in whichTuneRecommendedValuevariant they extract. A single helper returningOption<&TuneRecommendedValue>for a given field, with each caller doing its ownmatch, would remove the triplicated boilerplate.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark/candidates.rs` around lines 672 - 717, The three helpers recommended_u32, recommended_bool, and recommended_cache_type duplicate the same field_statuses scan and only differ in how they extract the value. Add a shared helper in candidates.rs that searches plan.field_statuses for the matching TuneField and returns the underlying TuneRecommendedValue, then have each recommended_* wrapper call that helper and match its own variant. Keep the existing behavior for Applied and ReportOnly statuses, and preserve the current return types for the three public-looking helpers.
320-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate MTP/Draft threshold cross-product logic.
push_mtp_threshold_cross_productandpush_draft_threshold_cross_productare ~50-line near-duplicates differing only in the enum variant anddraft_min_tokenstype (u32vsOption<u32>). Consider extracting a shared helper that builds candidates from a base-closure and the two optional threshold slices, parameterized by a constructor closure.Also applies to: 419-473
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark/candidates.rs` around lines 320 - 374, The cross-product generation in push_mtp_threshold_cross_product duplicates the logic in push_draft_threshold_cross_product, so factor the shared threshold expansion into a reusable helper that accepts a constructor closure for the candidate variant. Keep the helper responsible for handling empty/non-empty acceptance_thresholds and split_probabilities, and have both push_mtp_threshold_cross_product and push_draft_threshold_cross_product delegate to it while only supplying their variant-specific fields such as draft_min_tokens and the Mtp/Draft candidate construction.crates/mesh-llm-commands/src/gpus/tune/benchmark/trial_config.rs (1)
29-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilently swallows table-construction errors instead of propagating.
apply_trial_runtime_configreturns()and discards any error fromensure_trial_subtablevialet _ = error; return;, silently skipping the rest of runtime-config rendering. It's currently unreachable (the document is freshly built here so the table can't already be a non-table value), but if this function is ever called after other code populatesruntime/native_runtime, a real collision would be silently swallowed rather than surfaced to theanyhow::Result<String>caller.Consider making this function return
anyhow::Result<()>and propagating with?, matching the other builder functions in this file.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark/trial_config.rs` around lines 29 - 69, `apply_trial_runtime_config` is swallowing `ensure_trial_subtable` failures instead of surfacing them. Update this helper to return `anyhow::Result<()>`, propagate both `ensure_trial_subtable` calls with `?`, and let the caller handle the error so collisions in `runtime` or `native_runtime` are not silently ignored. Adjust the function’s callers in the trial config builder flow to use the result consistently with the other builder helpers in this file.crates/mesh-llm-commands/src/gpus/tune/benchmark/trial.rs (1)
277-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSend SIGTERM directly instead of spawning
kill
terminate_childdepends on an externalkillbinary and ignores its exit status. A direct Unix signal API would remove the PATH dependency and subprocess overhead while keeping the existingchild.kill()fallback for non-Unix targets.🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark/trial.rs` around lines 277 - 301, The terminate_child helper currently shells out to an external kill command on Unix, which adds a PATH dependency and drops any signal-send errors. Update terminate_child to use a direct Unix signal API in the Unix branch, keeping the existing child.kill() fallback for non-Unix targets, and preserve the current wait-and-escalate flow in terminate_child and its timeout loop.
🤖 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 `@crates/mesh-llm-commands/src/gpus/tune/benchmark/candidates.rs`:
- Around line 11-70: The default benchmark candidate generation in
benchmark_candidates is producing too many combinations before any trial starts.
Add a guard such as a max-trials cap or an explicit confirmation path in the
benchmark tune flow, and/or reduce the default ladders returned by
default_context_sizes and the default batch/ubatch fallbacks so the default
request path stays small and usable.
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark/mod.rs`:
- Around line 10-14: The `benchmark::mod` re-exports for `trial::*` and
`trial_config::*` are suppressing unused-import warnings without justification.
Either replace the glob re-exports with only the specific items that are
actually used downstream, or remove the `#[allow(unused_imports)]` attributes
and add a short comment explaining why the flat `tune` re-export path requires
them. Use the `trial`, `trial_config`, and `candidates` re-export block in
`mod.rs` to make the change.
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark/trial.rs`:
- Around line 444-447: The trial setup has a port-reservation TOCTOU issue:
reserve_local_port() only samples an OS-assigned port and releases it before the
child spawned in the trial setup can bind, so a later bind failure may be caused
by port reuse rather than a real trial error. Update the trial flow around
reserve_local_port() and the child spawn/readiness handling to detect this
specific bind failure from the child’s exit status or logs, then retry the same
trial with a newly reserved port instead of marking it failed immediately.
- Around line 397-419: In send_chat_request, the HTTP status is checked only
after response.json(), so non-JSON error responses fail with a decode error
before the intended status-based message. Update the flow to inspect
response.status() first, read the response body as text, and only parse JSON
when the status is successful; otherwise surface the existing “chat completion
failed with HTTP {status}: {body}” error using the captured body text.
---
Nitpick comments:
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark/candidates.rs`:
- Around line 672-717: The three helpers recommended_u32, recommended_bool, and
recommended_cache_type duplicate the same field_statuses scan and only differ in
how they extract the value. Add a shared helper in candidates.rs that searches
plan.field_statuses for the matching TuneField and returns the underlying
TuneRecommendedValue, then have each recommended_* wrapper call that helper and
match its own variant. Keep the existing behavior for Applied and ReportOnly
statuses, and preserve the current return types for the three public-looking
helpers.
- Around line 320-374: The cross-product generation in
push_mtp_threshold_cross_product duplicates the logic in
push_draft_threshold_cross_product, so factor the shared threshold expansion
into a reusable helper that accepts a constructor closure for the candidate
variant. Keep the helper responsible for handling empty/non-empty
acceptance_thresholds and split_probabilities, and have both
push_mtp_threshold_cross_product and push_draft_threshold_cross_product delegate
to it while only supplying their variant-specific fields such as
draft_min_tokens and the Mtp/Draft candidate construction.
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark/trial_config.rs`:
- Around line 29-69: `apply_trial_runtime_config` is swallowing
`ensure_trial_subtable` failures instead of surfacing them. Update this helper
to return `anyhow::Result<()>`, propagate both `ensure_trial_subtable` calls
with `?`, and let the caller handle the error so collisions in `runtime` or
`native_runtime` are not silently ignored. Adjust the function’s callers in the
trial config builder flow to use the result consistently with the other builder
helpers in this file.
In `@crates/mesh-llm-commands/src/gpus/tune/benchmark/trial.rs`:
- Around line 277-301: The terminate_child helper currently shells out to an
external kill command on Unix, which adds a PATH dependency and drops any
signal-send errors. Update terminate_child to use a direct Unix signal API in
the Unix branch, keeping the existing child.kill() fallback for non-Unix
targets, and preserve the current wait-and-escalate flow in terminate_child and
its timeout loop.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa8935a1-9ab0-4560-b5f8-111995747aa1
📒 Files selected for processing (51)
crates/mesh-llm-commands/src/gpus.rscrates/mesh-llm-commands/src/gpus/tune/apply_collision_tests.rscrates/mesh-llm-commands/src/gpus/tune/apply_test_support.rscrates/mesh-llm-commands/src/gpus/tune/apply_write_tests.rscrates/mesh-llm-commands/src/gpus/tune/benchmark/candidates.rscrates/mesh-llm-commands/src/gpus/tune/benchmark/mod.rscrates/mesh-llm-commands/src/gpus/tune/benchmark/tests.rscrates/mesh-llm-commands/src/gpus/tune/benchmark/trial.rscrates/mesh-llm-commands/src/gpus/tune/benchmark/trial_config.rscrates/mesh-llm-commands/src/gpus/tune/benchmark_progress.rscrates/mesh-llm-commands/src/gpus/tune/benchmark_selection.rscrates/mesh-llm-commands/src/gpus/tune/matrix.rscrates/mesh-llm-commands/src/gpus/tune/metadata.rscrates/mesh-llm-commands/src/gpus/tune/metadata_tests.rscrates/mesh-llm-commands/src/gpus/tune/mod.rscrates/mesh-llm-commands/src/gpus/tune/output_emit.rscrates/mesh-llm-commands/src/gpus/tune/output_launch.rscrates/mesh-llm-commands/src/gpus/tune/output_render.rscrates/mesh-llm-commands/src/gpus/tune/output_report.rscrates/mesh-llm-commands/src/gpus/tune/output_tests.rscrates/mesh-llm-commands/src/gpus/tune/output_types.rscrates/mesh-llm-commands/src/gpus/tune/output_values.rscrates/mesh-llm-commands/src/gpus/tune/planning.rscrates/mesh-llm-commands/src/gpus/tune/recommendation.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_defaults_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_existing.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_failure_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_reports.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_tests.rscrates/mesh-llm-commands/src/gpus/tune/recommendation_writes.rscrates/mesh-llm-commands/src/gpus/tune/tests.rscrates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rscrates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rscrates/mesh-llm-commands/src/gpus/tune_hardware/tests/selection.rscrates/mesh-llm-commands/src/gpus/tune_runner.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rscrates/mesh-llm-host-runtime/src/runtime/context_planning.rscrates/mesh-llm/src/commands/mod.rscrates/skippy-correctness/src/runner/mod.rscrates/skippy-correctness/src/runner/native_mtp.rscrates/skippy-runtime/src/lib.rscrates/skippy-runtime/src/native_mtp/mod.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/native_mtp/batched_verify.rscrates/skippy-server/src/frontend/native_mtp/mod.rscrates/skippy-server/src/frontend/native_mtp/verifier.rscrates/skippy-server/src/frontend/speculative.rsdocs/specs/layer-package-repos.mdthird_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patch
🚧 Files skipped from review as they are similar to previous changes (29)
- crates/skippy-server/src/frontend/native_mtp/mod.rs
- crates/mesh-llm-commands/src/gpus/tune/output_emit.rs
- crates/mesh-llm-commands/src/gpus/tune/benchmark_progress.rs
- crates/mesh-llm-commands/src/gpus/tune/output_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_reports.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_failure_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_defaults_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/tests.rs
- crates/mesh-llm-commands/src/gpus/tune/apply_collision_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/output_launch.rs
- crates/mesh-llm-commands/src/gpus/tune/apply_write_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/metadata_tests.rs
- crates/mesh-llm-commands/src/gpus/tune/output_report.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_existing.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/mlock.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rs
- crates/mesh-llm-commands/src/gpus/tune/planning.rs
- crates/mesh-llm-commands/src/gpus/tune/output_types.rs
- crates/mesh-llm-commands/src/gpus/tune_hardware/evaluate.rs
- crates/mesh-llm-commands/src/gpus/tune/matrix.rs
- crates/mesh-llm-commands/src/gpus/tune/output_values.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation_writes.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rs
- crates/mesh-llm-commands/src/gpus/tune/recommendation.rs
- crates/mesh-llm-commands/src/gpus/tune/output_render.rs
- crates/mesh-llm-config/src/validate.rs
- crates/mesh-llm-commands/src/gpus/tune_runner.rs
- crates/mesh-llm-commands/src/gpus/tune/metadata.rs
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) |
ndizazzo
force-pushed
the
feature/gpu-tune-cli
branch
from
July 6, 2026 15:48
ed592c8 to
b0a190c
Compare
ndizazzo
force-pushed
the
feature/gpu-tune-cli
branch
from
July 6, 2026 15:50
b0a190c to
a3921d7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
mesh-llm benchmark tuneas the automated performance tuning surface for model-serving throughput. It runs isolated benchmark trials, reports live progress, records lifecycle timings, compares raw best vs recommended settings, and can emit config edits for the winning setup.The benchmark tuner now covers context/batch/ubatch, mmap, mlock, and speculative decoding sweeps, including native MTP, draft-model, and ngram strategies.
End User Experience
Users run:
During the run they see progress like:
Final human output includes the recommendation, raw best, Pareto frontier, timings, and every trial:
Results
The tuner reports:
raw_best.10%.ctx_size.setup_ms,readiness_ms,request_ms,shutdown_ms,total_ms, and readiness attempts.This means a user no longer has to manually decide whether
20.41 tok/s at 32768 ctxis meaningfully better than19.88 tok/s at 131072 ctx; the command treats near-equivalent throughput as equivalent and chooses the more useful context window.What Changed
mesh-llm benchmark tune.draft_max_tokensanddraft_min_tokens.native-mtp-n1config values tomtp.Testing
Validated with:
Summary by CodeRabbit
benchmark tunefor local throughput benchmarking with candidate sweeps, tolerance-aware recommendations, JSON/text reporting, and options to apply changes or print launch previews.mmap/mlockcontrols across CLI, runtime, and embedded serving paths.mtp) strategy identifier.