Repository navigation
fix(lanes): a pipelined stream adopted between rounds starts in verify - #928
Conversation
A stream adopted between rounds holds neither a mode nor a queued step: detach drops a verify or exit stream's mode, and a stream another driver opened never had one. pipelinedRound read the missing mode as pipe and unwrapped a step that was never queued; it now starts such a stream as verify, from its pending token.
cwschroeder
left a comment
There was a problem hiding this comment.
Mac receipt for 5d6a0abf on its c7137bb3 base: M5 Max, macOS 26.6.2 (25G83), Zig 0.17.0.
zig build test test-golden -Dcpu=apple_m1 --summary all
zig fmt --check build.zig zigBoth pass: 164/164 build steps, 1985 tests passed and eight skipped. The frozen server answers have zero unexpected differences.
I also ran each new handoff test separately in Debug on an isolated copy of the core. Both pass on this head. Replacing only lanes/engine.zig with the unchanged base makes each fail with attempt to use null value in pipelinedRound. The Mac control therefore reproduces the failure your tests target.
Source review found no material issue in the mode selection: a missing queued step starts in verify; a queued step retains pipe, and explicit modes retain their meaning. One small test gap: expectDecode also accepts a correct short prefix. Comparing the terminal reason and expected length would catch a prematurely finished reply too.
These are host-side checks. No GPU, model, throughput or current-main integration result is claimed.
… reason expectDecode compared only the tokens a reply held with the fake target's decode, so a reply that ended early passed. It now builds the decode to the case's max_new or first eos token and compares the whole reply and the stream's reason; like expectRows, it takes the case.
… leaves The hand-back test detached the lone stream again at every chance, so when an adopted round left no step queued, no round ran on what it left: after a copy window that keeps more than one token, the stream still holds neither a mode nor a queued step and starts in verify again. It now skips the hand-back in the step right after one, and the copy-drafted set runs 60 tokens, so that round comes before its reply ends.
|
Thanks for the Mac run and the review, @cwschroeder. You're right about Looking for more of the same kind, I found one: the hand-back test handed the stream back at every chance, so when an adopted round left no step queued, no round ran on what it left. 721c29b skips the hand-back in the step right after one, so a round runs on what the adopted round leaves, and a variant that leaves an adopted stream in pipe after its copy window, with no step queued, now fails the hand-back test too. With the fix, both tests pass in Debug, ReleaseSafe and ReleaseFast, and on the unchanged base they still panic in Debug at engine.zig:488:52. Both test commits change only engine_test.zig. On 1.0.12 (46a08c1) the three commits cherry-pick cleanly and the tests behave the same, with the unwrap at engine.zig:510:52. I've updated the receipt in the description. |
cwschroeder
left a comment
There was a problem hiding this comment.
The updated assertions close the test-coverage gap from my earlier review. expectDecode now constructs the complete expected reply independently of the observed prefix and checks the finish reason. The extra settled round also exercises the state left by the adopted round before another hand-back.
I independently cherry-picked all three commits onto current main 46a08c18, preserving authorship. Both named adoption/hand-back regressions pass in Debug, Safe and Fast. The complete Lane Core CPU suite passes 120/120 tests there. With only engine.zig restored to unchanged main, each regression independently panics at the s.inflight.? unwrap at line 510.
Whole-tree formatting, whitespace and lean comparison pass (517 on base and port, zero new). Source review found no issue in the two test commits. All four checks on the original PR head 721c29b6 are green. My current-main verification covers the core CPU behavior; it is not a full-workflow, device or performance receipt for that port.
|
Thank you, @chaog992, for extending the adoption controls through the next round and comparing complete replies. We merged your three commits for 1.1.0 in 8bf9f9c. An adopted stream without a mode or queued step enters verify from its pending token; existing modes and queued steps keep their meaning. The complete-token and finish-reason assertions catch the earlier false passes. No shipped family reaches this adoption branch yet. Posted by Ash's AI Assistant Synthena |
…n verify instead of unwrapping a step it never queued
What this changes
A pipelined stream handed to the lane core with
Engine.adopt(engine.zig:87) holds neither a mode nor a queued step. Its firstpipelinedRoundread the missing mode as pipe and unwrappeds.inflight.?(engine.zig:488), a step that was never queued: a panic in Debug and ReleaseSafe, undefined behaviour in ReleaseFast. It now starts such a stream as verify, which is where a drain step leaves one (engine.zig:494): it tries a copy window frompending, or queuespendingand pipelines on as before.Such a stream comes from
detach(engine.zig:92, added with #780), which gives away a pipelined stream in verify or exit mode with no step queued and clears its mode, or from a lone driver that opened the stream itself (lane_host_lone.zig:71). The lane core queues a step when it opens a stream (open.zig:52), and every round on a stream it opened leaves one queued or leaves it in verify or exit. So only states that reach the null unwrap today take the new branch.No shipped family reaches this. Lone drivers take only drafted requests (lane_host_lone.zig:18) and are installed only with a head or drafter (families/nemotron/cuda_native.zig:98, native/metal.zig:191, native/qwen27_served.zig:293, and on 1.0.12 native/xpu.zig:146), so their drafted streams take windowed rounds (lanes/config.zig:107, engine.zig:148).
tensorfold run --hand-over(zig/src/main.zig:249) needs the head too (gpu_round.zig:283).Refusing such a stream in
detachwould cover only the first way in, and fixing it inadoptwould needstep's windowed rule a third time, or a mode on every adopted windowed stream;pipelinedRoundis the only code that acts on the mode. Starting in exit, or queuingpendingand piping, also replies correctly but gives up the copy window: the hand-back test's copy-drafted stream then records 59 ins.roundsfor its 60 tokens, against 27 with this change, the same as when it is never handed over.Receipt
expectDecodebuilds the fake target's one-token decode to the case'smax_newor first eos token and compares the whole reply and its finish reason.modelWith(false)is a family without a head, whose drafted streams step pipelined.detachallows it, except in the step right after a hand-back, so a round also runs on what an adopted round leaves. Each set must hand back at least once.Loneopens one, then adopted, with and without a head.step(engine.zig:149); CI'szig build testbuilds these tests in Debug. ReleaseSafe panics too, with the site misattributed to depth.zig:262:81. ReleaseFast fails the whole-reply check (50 where the decode has 42 at index 4; 9 where it has 75 at index 1), an outcome undefined behaviour doesn't guarantee. The hand-back test in Debug, with the runner's counter, thread id, addresses, local paths, source lines and runner frames cut (the driver-opened test fails the same way, from engine_test.zig:545:57):zig build test46/46 steps and 745/750 tests with 5 skipped (743/748 on the base);test-golden(0 unexpected differences),fuzz-cortheonandzig fmt --check build.zig zigpass. Lean lists the same 527 problems as the base; the one in a touched file, engine.zig:86, is inherited.zig build test84/84 steps and 1378/1385 tests with 7 skipped (1376/1383 there),test-kernel-contract,test-golden,fuzz-cortheonand lean. With 1.0.12's own engine.zig both tests panic in Debug at engine.zig:510:52.detachinstead fails the hand-back test's check that every set hands back. Ending or dropping an adopted stream at once, stopping one token early or running one late, or ending with the wrong reason each fail at least one test; the first version ofexpectDecode, which compared only the tokens a reply held, passed all of them. Leaving an adopted stream in pipe after its copy window, with no step queued, fails the hand-back test; both earlier versions of the test passed it.serveat.mtp = falsepanics on the base on the lane host's thread, and gives exact replies with the fix."draft": falserequest without logprobs or a grammar on Metal Nemotron and on GLM on one Mac. In an x86_64 ReleaseFast host fixture, single-step counts show one fewer instruction for each additional pipelined round with the mode unset, and no change with it set to pipe.pipelinedRound.zig build test test-golden -Dcpu=apple_m1passing on an M5 Max at 5d6a0ab, and the base failure reproduced there. The two test commits change only engine_test.zig, which here only cross-compiles foraarch64-macos. No Metal or CUDA run, since no shipped family reaches the new branch.