Repository navigation
placement, model, gpu, gpu-gates: a Machine's host and unified pool live in Machine::nodes, this process's node first - #94
Conversation
|
Thank you for the three Spark PRs (#94, #95, #96). The 0.3.2 release freezes at 13:00 KST today, so these go into review after its tag, in order: #94 first, then #95 on top of it, then #96. They change device and placement code, so each one needs a box run before it lands. The review bot fails on fork PRs for now; a workflow setting refuses the checkout. That is ours to fix. |
b240344 to
83205a0
Compare
There was a problem hiding this comment.
Verdict: CLEAN: The field move preserves existing single-node planning; the remaining findings are non-blocking.
Non-blocking
⚠ Test first-node selection: crates/placement/src/placement.rs:261. Every migrated fixture has one node, so replacing first() and first_mut() with last() and last_mut() passes those assertions. Add a fixture with distinct host budgets and pools; check all four accessors and that mutations leave the second node unchanged.
⚠ Attach move-class proof: crates/placement/src/placement.rs:253. The body supplies host/static results but no identical just ptx-scan pair. Attach the covering binaries' base/head scans required by AGENTS.md's “move, split, rename” class; --no-jit suffices.
Checked
Duplicates: the new Node type and 6 methods checked; first-node selection has one owner.
Complexity: the 6 new methods are straight-line accessors and add no model-specific branch.
Silent failure: 3 added-line sites followed. Two expect calls give a named empty-node panic; the moved test fallback selects the defined discrete-memory fixture.
Tests: fixture/accessor edits preserve assertions. Static mutation review: dropping the unified-pool check at crates/placement/src/placement.rs:3627 fails the one-byte-short case; dropping the checkpoint reserve push fails the reserve assertions. First-to-last selection survives the migrated fixtures.
Gates: no pin, threshold, assertion or case removed or relaxed.
Performance: changed callers perform planning or load-time validation; no new work per token, launch, copy or float reduction.
Escape catalog: C1–C4 match no changed code. No ledger lessons are in force.
Notes
This review used source inspection and mutation reasoning; no builds or gates were executed. The supplied evidence lacks a card gate run meeting CONTRIBUTING.md's evidence list.
Card check: needs-gpu. This pull request changes crates/gpu-gates/src/bin/gate_deepseek41_tier.rs, crates/gpu-gates/src/bin/gate_glm5next_residency.rs, crates/gpu-gates/src/bin/shared/residency_clauses.rs … and carries no gate run on a card (CONTRIBUTING's evidence list); a maintainer adds gpu-verified after one.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
Filed outside this PR: #110
…re read and changed through Machine::host, host_mut, unified and unified_mut Both name this process's node. Every reader outside the struct's own literals goes through the four accessors, so moving the two fields into a per-node list (docs/contrib/dgx-spark.md §8.2) changes the accessors and the literals only. No plan changes: the accessors return the fields as they are.
…n Machine::nodes, this process's node first Node holds what a node of a machine has beside its cards: its host and, on a unified machine, the pool its cards and host share. Machine::nodes lists them with this process's node at 0, which Machine::host, host_mut, unified and unified_mut read; a machine with no node panics by name there. Every machine built today has one node, so every plan is unchanged.
83205a0 to
7395c7d
Compare
|
@midagedev 리베이스 완료했습니다. 리뷰 부탁드립니다. |
There was a problem hiding this comment.
Verdict: CLEAN: No must-fix finding remains. The first-node test gap and missing move-class proof remain non-blocking.
Non-blocking
⚠ Pin first-node selection: crates/placement/src/placement.rs:261 and crates/placement/src/placement.rs:268.
Every changed machine fixture has one node. Replacing first() with last() and first_mut() with last_mut() leaves their assertions green.
Add a two-node case with distinct hosts and pools. Exercise all four accessors and assert that writes leave the second node unchanged. Also assert the named empty-node panic. This pins the contract before per-node planning depends on it.
⚠ Attach the move-class proof: crates/placement/src/placement.rs:253.
The body reports host checks and tests but carries no base/head PTX comparison. AGENTS.md's move class requires identical just ptx-scan results and unchanged host dispatch structural lines.
Attach the affected bins' base/head scans (--no-jit suffices) and the structural checks. Recording this now keeps the proof separate from the next planning change.
Checked
Duplicates: the new Node and six accessor helpers centralize current-node access. Dryer reported no qualifying pairs.
Complexity: crapper flagged no changed functions.
Silent failure: all four accessors reach a named panic on an empty node list.
Tests: all 35 node-vector literals in changed files contain one node; first-to-last selection is equivalent on those fixtures. The diff adds no tests. Returning None from unified() fails the existing integrated-card and pool-bound assertions by inspection.
Gates: no pins, bands, thresholds or cases changed or removed.
Escape catalog: no C1–C4 patterns matched. No ledger lessons are in force.
Notes
Dryer excluded gpu-gates. Validation used source inspection; no builds, tests or hardware gates were executed. The supplied runs do not meet the GPU evidence list.
Card check: needs-gpu. This pull request changes crates/gpu-gates/src/bin/gate_deepseek41_tier.rs, crates/gpu-gates/src/bin/gate_glm5next_residency.rs, crates/gpu-gates/src/bin/shared/residency_clauses.rs … and carries no gate run on a card (CONTRIBUTING's evidence list); a maintainer adds gpu-verified after one.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
…the machine's node, after midagedev#94 moved host and unified onto nodes[0]
…the machine's node, after midagedev#94 moved host and unified onto nodes[0]
…the machine's node, after #94 moved host and unified onto nodes[0]
What and why
Item 1 of P5's placement half in
docs/contrib/dgx-spark.md§8.2: aMachinelists its nodes, each with its own host and unified pool, and this process's node comes first. Every plan today readsnodes[0], so no plan changes. Part of #13 (P5, agreed in #13 not to wait for S10).Two commits, so each one reads on its own:
be407ec1addsMachine::host,host_mut,unifiedandunified_mut, and moves every read and write of the two fields to them. The fields stay where they are.7395c7dcmoves the fields intoMachine::nodes: Vec<Node>(Node { host, unified }). The accessors readnodes[0]and panic by name on a machine with no node. EveryMachineliteral now carriesnodes: vec![Node { .. }].The node plans (the next PR, #95) plan each node of
Machine::nodesagainst its own pool, which is why this one goes first.Rebased onto main at
bd29e9c9, which carries themac-remoteself-test fix (#113). That rebase had no conflicts, andgit range-diffshows both commits unchanged:203cf8e5is nowbe407ec1and83205a05is now7395c7dc. The runs under "What I ran" are on the commits before it. The rebase before that, ontod24cb1a4past 0.3.2, had conflicts only in the second commit, in the testuselists ofcrates/gpu-gates/src/bind.rsand the glm5next and qwen35moeplace.rs, where 0.3.2 changed the imports next to theNodethis PR adds. The change itself is the same.Code shape
nodes[0]itself.place.rsfiles change only theirMachineliterals and field reads.cargo fmtandcargo clippyare clean on what I touched; the lint count did not go up.Duplication found or removed (
path:line), or "none": none.What I ran
On the Mac, at
83205a05(andtools/mac-check.sh checkat203cf8e5alone):(each step's line cut after its time; the lint step counts 41 warnings, the ratchet's 41)
On one DGX Spark (aarch64), in the dev container, at
83205a05:The one red step is
check-recipes: main'stools/mac-remote.sh --self-test(fromd24cb1a4) fails 4 of its lock checks on an aarch64 Linux host. Those checks stubuname -sasDarwinbut read the host's ownuname -m, which isaarch64here, so the preflight stops atmissing: a macOS host (this is Darwin aarch64). This PR does not touchtools/, and every other step is green. #113, now on main under this PR, fixes that self-test. Before the rebase ontod24cb1a4, at27760114on main at9ab442ca, the same T1 hadcheck-recipesgreen; its one red wascheck-levers, on main'sBLOOMERY_MAC_REMOTEbefored24cb1a4gave it a registry row.just affected upstream/main --no-boxselects 140 of 156 gate recipes, since every gate readscrates/placement(a static walk, no dep-info). The change moves two fields and keeps every plan's numbers. The placement and model tests above hold every plan's literal machine, and the next PR's Spark runs (its "What I ran") comparebloomery-serve --planagainst main byte for byte on top of this one.Hardware (if the change is for a machine)
Not for a machine. Mac (Apple M5, 32 GB) and one DGX Spark (GB10, driver 595.84, Ubuntu 24.04, kernel 6.17.0-1032-nvidia) for the aarch64 T1;
rustc 1.100.0-nightly (e457a7b0d 2026-08-27), the toolchainrust-toolchain.tomlpins.AI help
AI 코딩 에이전트가 코드를 작성한 후 설명을 썼고, 테스트를 마쳤습니다. diff 리뷰와 PR 초안 리뷰는 제가 직접 했습니다.