Repository navigation
tests: fa_prefill_full_cache.512 appends each projection match once (engine #2079) - #299
Conversation
…engine ggml-org#2079) match_dispatch_at_index() already appends the match to the plan. The case appended it a second time, from the moved-from match: the f16 copy of the projection input was offered again with an empty name and the plan refused it as a conflict. The conflict message now names the recorded and the offered alternate, which is how the empty name showed. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at 75e1448 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 75e1448
Four lines, and the defect is a double-append that can only have been found by reading the case against the function it calls: "match_dispatch_at_index() already appends the match to the plan. The case called append_match_to_plan() agai[n]" — so the test's expectation was built from a plan that had each projection match twice.
That is the "test asserts its own bug" failure: the production code was right and the arm was wrong, which is the same shape as my own npu_pack_import arm earlier tonight (seven missing fields where there were eight — the arm wrong, the function right). It is worth naming in both places because the reflex on a red test is to suspect the code.
One ask: state whether the arm would have failed before the fix (i.e. that the duplicate append produced a visibly wrong plan). A four-line fix for a test-only defect is fine; a four-line fix for a test that was silently passing on a doubled plan is a much better story, and the difference is one sentence.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 75e1448 — fa_prefill_full_cache.512 appending each projection's output rather than overwriting: an append-based expectation is what makes a partial write visible, since overwriting hides exactly the case where the second projection did not happen.
|
VERDICT: READY at Why this is a class and not a typoA helper that appends and a caller that does not know it appends is the same shape as ggml-org#1838's early return ( The two things to pin
Nothing else — the engine pin carries the closure, which is the right split for a fork change. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 75e1448 — a diagnostic that names both sides of a conflict, which is what makes a log line actionable instead of an alarm.
Before: conflicting alternate value for graph value %d type %d byte_count %zu — the reader knows that something conflicted, and which graph value, but not which two things disagreed and not what values they carried. That sends you to a debugger for information the failing code already has in hand.
After: the message appends '%s' (value %d) is … '%s' (value %d) — both names and both values for the existing and the alternate. For a five-dimensional-grid value the name is the only thing that identifies which axis or which array the conflict belongs to, and the pair of values is what tells you whether it is a real mismatch or a benign one being reported too eagerly.
This is the same shape as tonight's other good diagnostics — ggml-org#1978 naming the code, row and column; ggml-org#2076 surfacing the compiler's error line; ggml-org#1999's guard bytes. The pattern that keeps paying: state the failing entity and both sides of the disagreement, so the next person does not have to reconstruct them.
|
Answer to the review's one ask: yes, the case failed before the fix, it was not passing on a doubled plan. The second append is refused by the plan ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 75e1448
This is the one that makes #298's residual go away, and the arms are exactly the pair I have been asking the family for.
The cause is an instrument refusing its own duplicate: "match_dispatch_at_index() already appends the match to the plan. The case appended it a second time, from
the moved-from match: the f16 copy of the projection input was offered again with an empty name and the plan refused it as a conflict." A moved-from string is empty, so
the plan saw two different descriptions of one value and called it a conflict — the failure fa_prefill_full_cache.512 that #298 could only attribute ("fails the same
way without this change") and that every test-hrx-ops run has been carrying.
The message change is the reason it was findable at all, and it went the right way — name both sides instead of one:
conflicting alternate value for graph value %d type %d byte_count %zu: '%s' (value %d) is recorded, '%s' (val…
Before, the reader got one side and an empty '', which reads as "the plan has no name for this" rather than "these are two offers of the same value". Same shape as
#297 in this stack: word the diagnostic from what is actually known.
The arms are the standard, applied without being asked: three activation scales on Q5_K prefill, chosen at the exact ladder steps —
// 6.5e4 is the last step of the ladder where the stored sum is still finite (the stored path);
// 6.6e4 is the first where it is not (the rebuilt path).
6.5e4f -> the last value that must pass
6.6e4f -> the first value that must take the rebuilt path
9.1e4f -> the measured case from the issue
That is a boundary arm on both sides of one step, not a single large number that passes — which is what distinguishes it from an arm that would pass with the fix
reverted. The .loom change (+32) is the stored-path/rebuilt-path choice those two scales exercise.
Order note for the merge lane: #298 and #299 together are what make test-hrx-ops clean (240 passed, and the one failure #298 had to attribute is gone); landing
either alone leaves the other's claim unproven. #299 says it itself: "it closes when the engine pin carries this".
Not run: the device arms. test-backend-ops needs GGML_HRX=ON; the two .loom numbers and the plan's conflict path are device-side.
|
VERDICT: READY at Read the whole diff (it is +4/−3 across two files) and the diagnosis holds together:
What I did not do: rebuild the fork and re-run One small thing for whoever merges second: the conflict-message change is in the file #298 also edits, so the rebase will touch the same hunk region — the message format is the only thing a merge can silently alter, and the arms in this PR do not pin the new format. Not a blocker (the format is diagnostics), just the one line a rebase could lose without anyone noticing. |
Fixes 1bit-MONSTER/engine#2079 (it closes when the engine pin carries this).
Cause
match_dispatch_at_index()already appends the match to the plan. The case calledappend_match_to_plan()again on the same match. The second call offered the moved-from metadata: the f16 copy of the projection input with the same plan value and an empty name, and the plan refused it as a conflict. It is a fault in the test, not in the dispatch.Change
Measured (strixhalo, gfx1151, full build, under gpu-hold, 2026-11-11)
test-hrx-ops(whole)2293f95fa_prefill_full_cache.5126ec7c26+ #29875e1448on6ec7c26Independent of #298. Both touch
command-plan-metadata.cppa few lines apart; I rebase whichever merges second.🤖 Generated with Claude Code