Lock split topology placement - #1050
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds a hidden ChangesLocked split topology
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Startup
participant LockLoader
participant Planner
participant Coordinator
CLI->>Startup: provide --split-topology-lock
Startup->>LockLoader: load and validate lock file
LockLoader->>Planner: return resolved stage assignments
Planner->>Coordinator: initialize locked split topology
Coordinator->>Coordinator: withdraw after locked-stage loss
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/mesh-llm-host-runtime/src/runtime/split_planning.rs (1)
208-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLogging parity gap vs. the unlocked planning path.
plan_runtime_slice_topology_with_resources(lines 159-206) logs a "planning resource-aware split runtime topology" event before planning (withparticipants,layer_count,native_context_length) and a richer post-plan log includingestimated_decode_network_ms_per_token/decode_tpot_target_met.plan_locked_runtime_slice_topology_with_resourcesonly logs after a successful plan and omits both the pre-plan context and the decode-latency fields, even thoughTopologyPlanfor the locked path carries the sameestimated_decode_network_ms_per_token/decode_tpot_target_metfields (seefit_locked_candidateinlocked.rs). For a benchmarking-oriented feature, matching observability would help diagnose failed/degraded locked plans.🤖 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/runtime/split_planning.rs` around lines 208 - 245, Add logging parity to plan_locked_runtime_slice_topology_with_resources: emit the same pre-planning resource-aware event as plan_runtime_slice_topology_with_resources using participants, layer count, and native context length, then include plan.estimated_decode_network_ms_per_token and plan.decode_tpot_target_met in the successful post-plan log. Preserve the existing locked planning and validation flow.crates/skippy-coordinator/src/topology.rs (1)
724-803: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining locked-topology validation error variants.
Only
LockedNonContiguousRangeandLockedTopologyDoesNotFitare exercised here.LockedStageCount,LockedUnknownNode,LockedDuplicateNode,LockedInvalidRange, andLockedIncompleteCoverage(all newly added in this PR at lines 89-112 and validated inlocked.rs'svalidate_locked_stages/locked_stage_nodes) have no tests. Given this validation gates a runtime-safety-critical feature (mis-locking a topology could otherwise silently start a broken split), each rejection path is worth a dedicated regression test.🤖 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-coordinator/src/topology.rs` around lines 724 - 803, Add dedicated regression tests for the remaining locked-topology validation errors in the existing locked-topology test module: assert plan_locked_topology returns LockedStageCount, LockedUnknownNode, LockedDuplicateNode, LockedInvalidRange, and LockedIncompleteCoverage for minimal inputs that trigger each condition. Match the existing assertion style and validate the expected error payloads, while retaining the current tests for non-contiguous ranges and capacity failures.
🤖 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/skippy-coordinator/src/topology.rs`:
- Around line 3-6: Move the three locked_topology_* tests from topology.rs into
a #[cfg(test)] mod tests in the locked submodule alongside plan_locked_topology
and LockedTopologyStage. Move only the imports required by those tests,
preserving their existing assertions and behavior while removing the duplicate
test code and imports from topology.rs.
In `@docs/SKIPPY_SPLITS.md`:
- Around line 93-111: Update the stage-range contract in the documented
lock-file example to explicitly state that layer_start is inclusive and
layer_end is exclusive, preserving the shown contiguous ranges and preventing
boundary ambiguity.
---
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/runtime/split_planning.rs`:
- Around line 208-245: Add logging parity to
plan_locked_runtime_slice_topology_with_resources: emit the same pre-planning
resource-aware event as plan_runtime_slice_topology_with_resources using
participants, layer count, and native context length, then include
plan.estimated_decode_network_ms_per_token and plan.decode_tpot_target_met in
the successful post-plan log. Preserve the existing locked planning and
validation flow.
In `@crates/skippy-coordinator/src/topology.rs`:
- Around line 724-803: Add dedicated regression tests for the remaining
locked-topology validation errors in the existing locked-topology test module:
assert plan_locked_topology returns LockedStageCount, LockedUnknownNode,
LockedDuplicateNode, LockedInvalidRange, and LockedIncompleteCoverage for
minimal inputs that trigger each condition. Match the existing assertion style
and validate the expected error payloads, while retaining the current tests for
non-contiguous ranges and capacity failures.
🪄 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: 06e748c7-78c6-456e-a984-64254ec8b3da
📒 Files selected for processing (22)
crates/mesh-llm-cli/src/parser.rscrates/mesh-llm-cli/src/parser/commands.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-host-runtime/src/runtime/local_split.rscrates/mesh-llm-host-runtime/src/runtime/local_split/coordinator.rscrates/mesh-llm-host-runtime/src/runtime/local_split/recovery.rscrates/mesh-llm-host-runtime/src/runtime/local_split/test_support.rscrates/mesh-llm-host-runtime/src/runtime/local_split/tests.rscrates/mesh-llm-host-runtime/src/runtime/mod.rscrates/mesh-llm-host-runtime/src/runtime/model_lifecycle.rscrates/mesh-llm-host-runtime/src/runtime/options.rscrates/mesh-llm-host-runtime/src/runtime/run_auto.rscrates/mesh-llm-host-runtime/src/runtime/serving_surface.rscrates/mesh-llm-host-runtime/src/runtime/split_planning.rscrates/mesh-llm-host-runtime/src/runtime/split_topology_lock.rscrates/mesh-llm-host-runtime/src/runtime/startup_handles.rscrates/mesh-llm-host-runtime/src/runtime/startup_models.rscrates/mesh-llm/src/lib.rscrates/skippy-coordinator/src/topology.rscrates/skippy-coordinator/src/topology/locked.rsdocs/SKIPPY_SPLITS.mddocs/design/TESTING.md
| mod locked; | ||
|
|
||
| pub use locked::{LockedTopologyStage, plan_locked_topology}; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Locked-topology tests added to topology.rs instead of the extracted module.
This PR extracts the locked-topology responsibility into a new locked submodule (re-exported here), but the tests exercising plan_locked_topology/LockedTopologyStage (lines 724-803) were added to topology.rs's own mod tests rather than to locked.rs. Repo guidelines require tests to travel with the extracted behavior when splitting a file that is already over 1,000 lines (this file now spans ~1,112 lines).
♻️ Suggested fix
Move the three locked_topology_* tests (and the LockedTopologyStage/plan_locked_topology imports they need) into a #[cfg(test)] mod tests block inside crates/skippy-coordinator/src/topology/locked.rs, alongside the extracted plan_locked_topology implementation.
As per coding guidelines, "When modifying a Rust file already over 1,000 lines, extract any separable responsibility into a named module, keep the new file under 1,000 lines, and move or add its tests with the extracted behavior."
🤖 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-coordinator/src/topology.rs` around lines 3 - 6, Move the three
locked_topology_* tests from topology.rs into a #[cfg(test)] mod tests in the
locked submodule alongside plan_locked_topology and LockedTopologyStage. Move
only the imports required by those tests, preserving their existing assertions
and behavior while removing the duplicate test code and imports from
topology.rs.
Source: Coding guidelines
There was a problem hiding this comment.
@i386 I had started chipping away at pieces of this with the config work, but right now it doesn't fully support everything you'd need for it. The idea was to incorporate the split topology in the configuration pages (which ultimate wrote some TOML like):
[[models]]
model = "hf://meshllm/example-layers@immutable-revision"
[models.model_fit]
ctx_size = 65536
[models.throughput]
parallel = 1
[models.topology]
mode = "locked"
manifest_sha256 = "0123456789abcdef..."
[[models.topology.stages]]
node = { endpoint_id = "iroh-endpoint-id-a" }
layer_start = 0
layer_end = 31
[[models.topology.stages]]
node = { hostname = "studio54-3.local" }
layer_start = 31
layer_end = 47I think the configuration TOML is the better long-term product surface, but the existing impl can't currently express it. What would be good is to accompany this PR with a definition for how we want to transition it. Here's what Codex said about the gap:
## How far PR #1050 is from this
The PR is close at the runtime layer but not at the configuration layer:
**Already implemented:**
* immutable model/manifest verification;
* node selector resolution;
* contiguous full-model layer validation;
* capacity-aware locked planning;
* locked recovery and withdrawal behavior;
* runtime propagation.
**Still needed:**
* ModelTopologyConfig, stage, and typed node-selector structs;
* TOML serialization/deserialization;
* structured config diagnostics and canonical paths;
* built-in schema and UI metadata;
* per-model startup propagation;
* CLI-versus-config precedence;
* authoring/editor and schema fixture coverage;
* documentation and validation tests.In the short term - nothing jumps out at me for this implementation, but I think we should definitely consider bringing them closer together from a product standpoint, so that we can take advantage of the configuration page for split configs in the web console.
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
I think very neeed - if uyou can get build passing.
|
Created #1052 to track the product transition: replace the hidden |
…ngram * origin/main: Add GLM DSA correctness trace reporting (#1033) Add GLM DSA package contract validation (#1032) Add direct quant layer package tooling (#1031) Lock split topology placement (#1050) fix nightly stability Qwen thinking ui(data-mode): migrate legacy harness storage to v2 (#1040) fix: Windows Vulkan runtime dependencies (#1046) test: validate MI300 runtime selection and placement (#1045) fix: support ROCm RDNA APU kernels safely (#1044) fix: ROCm GPU enumeration (#1039) Document public packaging installation channels Fix Windows autoupdate import (#1030) Update llama.cpp upstream pin
3 participants
Why this exists
The automatic split planner is the right default for normal serving: it chooses participants and layer boundaries from the capacity currently advertised by the mesh. That flexibility is undesirable during controlled lab benchmarks, where discovery timing, peer availability, or a changed VRAM report can otherwise produce a different split between runs.
A topology lock makes placement an explicit benchmark input. The same lock file is copied to every host and pins:
This ensures every host in the lab agrees on the same split topology and makes benchmark results comparable across binaries, branches, runtime settings, and repeated runs. If the requested topology cannot be reproduced exactly, startup fails instead of silently benchmarking a different placement.
How the lock is used
Create a versioned JSON manifest such as:
{ "version": 1, "model": "hf://meshllm/example-layers@immutable-revision", "manifest_sha256": "<sha256 of model-package.json>", "stages": [ { "node": "micstudio.local", "layer_start": 0, "layer_end": 31 }, { "node": "studio54-3.local", "layer_start": 31, "layer_end": 47 } ] }Place the same file on every serving host and launch each node with:
At startup, the runtime:
Node selectors may be full iroh endpoint IDs or uniquely advertised hostnames.
Fail-closed behavior
A locked topology is not a preference or planner hint. Membership changes do not replace its stages or collapse it to local serving. If an assigned stage is lost, the route becomes unavailable and is withdrawn after the normal stage-loss grace period.
This prevents a benchmark from continuing under a materially different execution shape while still appearing healthy.
Implementation summary
--split-topology-lock <path>runtime option;Validation
cargo fmt --all --checkcargo check -p mesh-llmcargo clippy -p skippy-coordinator -p mesh-llm-cli -p mesh-llm-host-runtime -p mesh-llm --all-targets -- -D warningscargo test -p skippy-coordinator --lib(32 passed)cargo test -p mesh-llm-cli --lib(72 passed)cargo test -p mesh-llm-host-runtime --lib(1703 passed, 8 ignored)just buildSummary by CodeRabbit
--split-topology-lock <PATH>to pin split node order and layer ranges via a validated JSON lock (requires--split).