Repository navigation
npu(p4): the lm-head context is the target's, so the forward no longer creates one (A2b, last setup block) - #1478
Conversation
|
#ready for review at The claim is narrow: the head block's body is unchanged, only its frame, and The gate I am not claiming yet: this block runs only for a |
…r creates one (A2b, last setup block) The block the previous commit cut out of init_impl() moves behind the target, body verbatim: the pb16i ternary-image branch, the int8 ladder, the q4_1 gemv head, every init_elf call and every refusal message. With this, a grep for make_head_ctx in q4nx_forward.cpp is empty -- the forward creates no I8Ctx at all any more; the prefill set moved in #1380 and the head moves here. What stayed in the forward, deliberately, and arrives through Setup: the container's NAMING. Deciding whether the head's ternary image exists at all is a path rule built from the head's sidecar stem and the image extension, which is this file's knowledge and not a target's, so the forward computes a head_pb16i_images boolean and passes the sidecar directory, the decode-dir list and the head geometry (hidden, vocab, kpad). A target that recomputed that rule would be a second copy of the container's naming, which is how a plan and its images drift apart. head_pb16i_ stays a forward flag -- the model path reads it in four places -- and the target writes its own copy which the forward reads back through head_is_pb16i(), so which leg opened the head is still visible where it matters. Gate: ctest 128/128 on a full Release build (ONEBIT_NPU=ON, runtime from ONEBIT_NPU_RT_DIR, under thermal-run.sh). The device half is the folded 27B pb16i row, A = main and B = this head, requiring the same text and the same 582/582 pb16i trace and 14/14 qkv taps: this block runs ONLY for a pb16i container and the 0.6B on this box is rm, so no ctest reaches it, and a build-only claim would be claiming the one path it cannot reach. That row is armed with a watcher because the box is at load 30+ with another lane holding the device.
0bb50f8 to
e4cea32
Compare
|
Build failure fixed at Root cause: this commit carried an unrelated submodule bump — Fix: rebased onto current Verified locally before pushing:
The rebase was a conflict on exactly that gitlink and it was resolved with Checks are running at |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e4cea323.
The pin gate is clean, and I verified it with the tool's real interface. The PR makes no submodule change at all — its four files are
npu/forward/{include/q4nx_forward.h,src/q4nx_forward.cpp} and npu/target/xdna/xdna_target.{cpp,h} — and the llama.cpp gitlink equals main's
exactly (fb10c19caf0142d4029421d487de596900e6f73a on both sides):
tools/check_pins.py $(git rev-parse origin/main) rc 0 (no differing gitlinks -> nothing to compare)
tools/registry_build.py --check-pins rc 0 "registry/architectures.json records the committed pins"
mergeStateStatus BLOCKED (no longer DIRTY; the rebase cleared the conflict)
One note on my own first attempt, in case it saves someone else a minute: I ran tools/check_pins.py with no argument and got rc 1 plus a usage
block — the tool takes <base-commit>, and its docstring is the clearest explanation of this gate I have read ("for every submodule whose gitlink
differs between base and HEAD, GitHub's compare API must say the new commit is ahead of the old one… a pin that is behind or has diverged drops
commits the engine already shipped, as #154 did to third_party/llama.cpp"). Your "exit 0 locally" is about the correct invocation and it holds.
The change itself is the next step of P4 A2 and it is a complete move, not a partial one: open_lm_head_context leaves the forward
(0 occurrences there — it was #1458's, now merged) and lands in XdnaTarget (14 references), which is what #1428's ledger says A2 is for
("the design derivation moves out of the forward and into the target"). And the risk in this class of change — a branch losing its cleanup while
moving — does not apply here: the three call sites (:517, :558, :608) each keep their head_ctx_ref().reset() (:530, :565, :614).
Three branches, three cleanups. (The fourth make_head_ctx in the file is the definition at :303.)
So: the gate that failed is gone because the pin move is gone, the conflict is gone, and the code is the A2 step with its failure paths intact. CI on
this head is the first place the built form of it runs.
|
Green at
For the record, the only real failure on this PR was the pin gate, and the fix is a one-line-in-effect change: the commit no longer touches |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e4cea323 — head unchanged, so this is the same code with its CI now complete, and everything real on it is green.
build: success <- the gate that was failing (6m10s)
Analyze (actions / c-cpp / javascript-typescript / python / rust): success
CodeQL: success
hw_gate: skipped <- by design, no device in CI
pr_agent: cancelled <- the only non-success
And pr_agent: cancelled is structural, not a failure — .github/workflows/pr-agent.yml:40-42:
concurrency:
group: pr-agent-${{ github.event.pull_request.number || github.event.issue.number }}
cancel-in-progress: trueso any second event on the same PR cancels the in-flight run. A cancelled conclusion is the designed outcome for a PR that has been pushed to,
commented on or edited more than once; it is the same at the previous head, which is what you would expect from a concurrency rule rather than from a
change of code. It explains the eight engine PRs that read UNSTABLE earlier for exactly this reason too.
The consequence a merger should know: main in this repository is not protected (GET /branches/main/protection → 404 "Branch not
protected"), so no required check exists to be failed and nothing here blocks a merge — mergeable: MERGEABLE right now. But the aggregate
mergeStateStatus will read UNSTABLE for any PR with more than one event, forever, because of that cancel-in-progress. So key the merge decision
on the named checks (build, and hrx_build where it runs) rather than on the aggregate, or the whole queue will look permanently unsettled for a
reason that has nothing to do with code.
Nothing to fix on this PR: gate gone, conflict gone, open_lm_head_context moved into the target with its three branches and three cleanups, and the
build that was red is green on the exact head.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e4cea323eb74d3e960620c963d23ef9d2b62330a
The move is clean, and the part that makes it more than a cut-and-paste is stated: what stayed behind is the container's naming rule, what moved is the target's device work.
forward -143 open_lm_head_context() body out; the sidecar naming stays (head_pb16i_stem(0) + image_ext())
and is handed over as data: setup.pb16i_dir / head_pb16i_images / decode_dirs / hidden / vocab / kpad
reads back: head_pb16i_ = xd_->head_is_pb16i();
target +137 XdnaTarget::open_head() -- "the body moved verbatim out of Q4nxNpuForward::open_lm_head_context"
writes head_pb16i_ in open_head(), exposes it through head_is_pb16i()
Checks I ran rather than eyeballed:
- no stale call sites:
git grep open_lm_head_context -- npu/finds only the history comment inxdna_target.h:161; the function itself is gone. This is the usual failure of a 143-line extraction — a second caller left reaching for the deleted method — and there is none. - the flag has one writer and one reader:
xdna_target.cpp:526setshead_pb16i_,:168exposes it,q4nx_forward.cpp:1336reads it into the member, and the four later uses (:4681,:4761,:4920,:4932) all read that member. Nothing else writes it, so the "written by the target, read by the forward" contract inxdna_target.h:210is the actual code and not a comment. - the split itself is right: the existence of the head image and the shape of the decode dirs are properties of the model directory, which the container owns; opening a context on the device is the target's.
setup.head_pb16i_imagesis computed by the container with its ownfile_exists(head_pb16i_stem(0) + image_ext())precisely so the target never has to know the file-naming convention — and the target'sopen_head()gates on!setup_.pb16i_dir.empty() && setup_.head_pb16i_images, i.e. a flag the container already decided.
Not run, stated plainly: a compile. g++ -fsyntax-only on both files gets as far as npu_engine_i8ctx_inc.h, a generated header that only exists inside a configured private build; the one copy on this box (/tmp/r1482/npu/forward/engine-src/) is from a different tree and mismatches this plan.h (Plan has no kv_key/Design), so the errors it produces are its own. A device arm would also need an xclbin. The command that would produce both: the NPU lane's own configure+build with ONEBIT_NPU_RT_DIR and its generated engine-src.
What I can take is the ownership contract, the writer/reader uniqueness of the flag, and the absence of leftovers — which is what this step of #803 A2b is about.
|
The device row is inconclusive on this box tonight, and here is exactly what I did and did not get — rather than a green that means nothing. Three attempts, all released clean, none completed an A-vs-B pair:
So the gate I designed is the right one — t2s2 does exercise What this PR therefore claims: build clean; Plan to close it (either is cheap, whichever comes first): a quiet-box window where one arm takes ~2 min instead of >9, or the harness split so each arm is its own ≤10-minute ticket with the comparison run afterwards on the two artefacts. I am happy to run either; say which you prefer. The |
The device row is now measured, and it PASSES — via a split harness rather than a quiet boxThe row I left open is closed with a real A-vs-B capture. Two tickets, one arm each ( Both arms exercised the block under test, not just the graph: each serve log carries so the ternary-image branch of Why t2s2 and not pb16i, recorded because it is a box fact and not a preference: the 27B pb16i container cannot run on this machine at all — And why the row needed a new harness: inside The PR's claims are now: build clean, |
…as two items left, not one Review at cfa7829 is right and this is the correction: the paragraph said the setup side is the target's and that q4nx_forward.cpp creates no I8Ctx at all. The prefill ELF set (#1380) and the lm-head context (#1458, #1478) did move - the forward calls open_head() at :1357 and then asks head_ctx() for it - but the M=16 decode family is still built here: the target allocates the memory (XdnaTarget::make_ctx16) while the forward decides which decode contexts exist and configures each one field by field (MD, KD, ND, blocked_b, pb16i, t2s, planes) at q4nx_forward.cpp:1306-1330, and docs/npu-p4-target-split.md section 3 says exactly that ('still in the forward, and therefore still owed: the M=16 decode family'). So A2 has two items left: A2c moves that decode-family setup behind the target, and load_block stays a named no-op whose explicit block step is an optimisation with a tokens/s row as its evidence, not a structural move. The paragraph now says that, cites the lines, and no longer claims the forward creates no I8Ctx. Docs only; copyright --check rc 0.
…w stops saying the derivation is all there (#1529) * docs/npu-plan: the schema table learns about designs[], and the P4 row stops saying the derivation is all there is Three statements on this page were true when written and are not now, all of them about fields the loader reads: * the schema table had no row for designs[] -- the per-role kernel set the plan may name (#1330, #1333), with its both-directions agreement check, its bare-file-name artifact, the generator writing it only where it can be complete, and Plan::check_artifacts() refusing a missing artifact at load (and npu_plan.py --check asking the same question at import time); * the layers[] row said the forward 'derives designs from the container', which is now only the no-section case; * the 'Not yet' P4 entry stopped at A2b part 1 (#1380). A2b is complete: the M=128 ELF set in open_prefill (#1380) and the lm-head context in open_head (#1458, #1478), so q4nx_forward.cpp creates no I8Ctx at all. What is genuinely still open is the residency step -- load_block is a named no-op, the cache fills lazily inside gemv -- and that is an optimisation whose evidence is a tokens/s row, not a structural move. Docs only; no code change. * docs/npu-plan: the M=16 decode family is still the forward's, so A2 has two items left, not one Review at cfa7829 is right and this is the correction: the paragraph said the setup side is the target's and that q4nx_forward.cpp creates no I8Ctx at all. The prefill ELF set (#1380) and the lm-head context (#1458, #1478) did move - the forward calls open_head() at :1357 and then asks head_ctx() for it - but the M=16 decode family is still built here: the target allocates the memory (XdnaTarget::make_ctx16) while the forward decides which decode contexts exist and configures each one field by field (MD, KD, ND, blocked_b, pb16i, t2s, planes) at q4nx_forward.cpp:1306-1330, and docs/npu-p4-target-split.md section 3 says exactly that ('still in the forward, and therefore still owed: the M=16 decode family'). So A2 has two items left: A2c moves that decode-family setup behind the target, and load_block stays a named no-op whose explicit block step is an optimisation with a tokens/s row as its evidence, not a structural move. The paragraph now says that, cites the lines, and no longer claims the forward creates no I8Ctx. Docs only; copyright --check rc 0. --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@users.noreply.github.com> Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
…ine#803 A2b step one) (#2062) A2b moves the M=16 decode family behind XdnaTarget::open_decode_family(), mirroring open_head (#1478). This is the step that makes that move a copy instead of surgery inside init_impl -- the same step the lm head went through before it became open_head, and the tree's own comment on that block says so. What moved: q4nx_forward.cpp's 'if (want_m16 \&\& !host_gemm_) { ... }' block (157 lines) is now Q4nxNpuForward::open_decode_family_local(bool want_m16, xrt::device* dev, const std::string\& mdir, const std::string\& dir) -- body copied verbatim, de-indented one level, with the three locals it reads threaded in as parameters. Two things deliberately unchanged, because gates read them: * the log strings are byte-for-byte ('[forward] ... (decode)', '[forward] decode family: N role(s) at M=16 ...', the pb16i 'N of M designs have a decode kernel' line) -- the first is grepped by [pb16i-trace] and the second by the window11 gates, so improving a message here breaks a gate that reads it; * the guard shape is preserved: the call site still passes want_m16 (from NPU_DECODE_M16), and the method still prints the 'disabled' line rather than silently returning. Verified: builds (onebit, with the dx add-on and the private runtime), and npu_forward_loader_test passes ('all cases ok'). NOT included, and stated on the issue so it is not assumed: the move into XdnaTarget itself -- the block reaches six forward members (family_tag(), designs_, designs16_, pb16i_, t2s_, host_gemm_) plus these three locals, so the target method needs a parameters struct or accessors, and that is a design decision the next diff has to make rather than a mechanical copy. Co-authored-by: pi-inferzo <pi-inferzo@1bit.gg>
What
The block #1458 cut out of
init_impl()now lives behind the target, body verbatim: the pb16i ternary-image branch, the int8 ladder, the q4_1 gemv head, everyinit_elfcall and every refusal message. With this,grep make_head_ctx npu/forward/src/q4nx_forward.cppis empty — the forward creates noI8Ctxat all any more (the prefill set moved in #1380, the head here).What deliberately stayed in the forward
The container's naming. Deciding whether the head's ternary image exists is a path rule from the head's sidecar stem + the image extension — this file's knowledge, not a target's. So the forward computes
head_pb16i_imagesand passes, viaSetup: the sidecar directory, the decode-dir list (m16dirs_like_head), and the head geometry (hidden,vocab,kpad). A target that recomputed that rule would be a second copy of the container's naming, which is how a plan and its images drift apart.head_pb16i_stays a forward flag (the model path reads it in four places); the target writes its own copy and the forward reads it back throughhead_is_pb16i().Rows
ONEBIT_NPU=ON/ runtime fromONEBIT_NPU_RT_DIR, underthermal-run.shctest -j1, whole tree17890cb4) vs B = this head (5b31a483), one arm per ticketcontent=32cboth),[pb16i-trace]72/72 identical,[qkv-dump]52/52 identical, and both arms' serve logs carry thelm head … (pb16i ternary images)line, so the branch under test is the branch that produced the tapstools/copyright.py --checkTwo box facts the row exposes, both recorded rather than worked around: the 27B pb16i container cannot run here at all (
~/models/npu/bonsai2-pb16i-ternary/, the directory itsconfig.jsonnames, is gone; both binaries fail identically), and a single 27B arm takes >9 min at load 30–55, which is why the row is one arm per ticket (~/wt/a2b-head-split.sh) with the comparison run device-free.Refs #803 (A2b), #799.