Skip to content

fix(ep-cpu): update the four head_sink call sites #1956 did not (main does not compile) - #1963

Closed
justinchuby wants to merge 1 commit into
mainfrom
squad/gaff-fix-main-build-break-head-sink
Closed

justinchuby wants to merge 1 commit into
mainfrom
squad/gaff-fix-main-build-break-head-sink

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

main does not compile

On a pristine checkout of 1b99d2232 with a clean git status:

$ cargo clippy --locked --all-targets -p onnx-genai-engine --features native-backend -- -D warnings
error[E0061]: this function takes 10 arguments but 9 arguments were supplied
error[E0061]: this function takes 11 arguments but 10 arguments were supplied
error: could not compile `onnx-genai-engine` (lib) due to 2 previous errors
exit 101

Both that command and cargo clippy --locked --all-targets -p onnx-runtime-ep-cpu -- -D warnings are steps of the required Rust quality check (ci.yml:377 and ci.yml:469). So this is red on every open PR in the repo, not only for whoever happens to build the engine.

Cause

#1956 (215559597, implement GroupQueryAttention head_sink on CPU) added a head_sink parameter to three public functions in onnx-runtime-ep-cpu::kernels::sdpa:

function new parameter
sdpa_decode_row head_sink: Option<f32>
sdpa_decode_row_accessor head_sink: Option<f32>
sdpa_decode_group head_sink: Option<&[f32]>

It touched exactly two files, both inside onnx-runtime-ep-cpu, and updated the callers in group_query_attention.rs. Four callers in two other targets were missed:

file line function
onnx-genai-engine/src/native_decode/paged_gqa.rs 164 sdpa_decode_row_accessor
onnx-genai-engine/src/native_decode/paged_gqa.rs 251 sdpa_decode_row
onnx-runtime-ep-cpu/benches/gqa_decode.rs 101 sdpa_decode_row
onnx-runtime-ep-cpu/benches/gqa_decode.rs 124 sdpa_decode_group

The bench pair is the interesting half: a bench is compiled only under --all-targets, so it is invisible to every ordinary cargo build/cargo test and shows up only in the clippy gate.

The fix, and why it is not a behaviour change

Every site passes None. That is not a placeholder — it is what #1956's own doc comment specifies:

None reproduces the pre-sink numerics bit-for-bit (matches the CUDA reference kernel's head_sink == …)

Neither call site binds a sink input. paged_gqa.rs exists to be the paged/reference decode the fused kernels are checked against, and gqa_decode.rs compares two schedules — both must keep computing exactly what they computed before #1956 or they stop being the thing they are for.

Evidence

Both required commands, on this branch, from clean:

cargo fmt --all --check                                                          -> 0
cargo clippy --locked --all-targets -p onnx-runtime-ep-cpu -- -D warnings        -> 0
cargo clippy --locked --all-targets -p onnx-genai-engine
    --features native-backend -- -D warnings                                     -> 0

Attribution is from a build, not from git log: 215559597^ compiles, 215559597 does not.

Scope

git grep sdpa_decode_row confirms there are no other callers in the workspace — the four above and the in-crate ones #1956 already updated are the complete set.

Deliberately nothing else in this PR. There is a second, unrelated red on main that I hit while tracking this one down (a test added by #1900 that #1892 later broke); it needs a real fix rather than a call-site update and it is not going to share a PR with a build unbreak.

@justinchuby
justinchuby enabled auto-merge (squash) August 24, 2026 08:02
…missed

`cargo clippy --locked --all-targets -p onnx-runtime-ep-cpu -- -D
warnings` fails with two E0061s on a pristine `ae7c6536a`. That command
is `ci.yml:377`, a step of the **required** `Rust quality` check, and it
runs before the engine step #1961 widened.

#1956 added `head_sink` to `sdpa_decode_row` and `sdpa_decode_group` and
updated the callers inside `group_query_attention.rs`. #1961 then
repaired the engine's two call sites. The two in ep-cpu's own
`gqa_decode` bench were missed by both, and a bench is compiled only
under `--all-targets`, so no ordinary build or test run reaches it.

Both sites pass `None`, which #1956's doc comment states "reproduces the
pre-sink numerics bit-for-bit". This bench compares two *schedules*
against each other, so both arms have to keep computing what they
computed when the comparison was calibrated.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby force-pushed the squad/gaff-fix-main-build-break-head-sink branch from 78a72f2 to 91670c5 Compare August 24, 2026 08:08
@justinchuby

Copy link
Copy Markdown
Owner Author

Closing: superseded, and the tree is genuinely repaired. #1961 fixed the engine's two call sites and #1958 fixed the bench's two, which is the complete set this PR carried. Both landed while I was validating.

Verified rather than assumed, on a pristine bb39883e3 with a clean git status — both steps of the required Rust quality check:

cargo clippy --locked --all-targets -p onnx-runtime-ep-cpu -- -D warnings                       -> 0
cargo clippy --locked --all-targets -p onnx-genai-engine -p onnx-genai-server \
    --features onnx-genai-engine/native-backend,onnx-genai-server/native-backend -- -D warnings -> 0

Worth recording, because three separate PRs chased one signature change and the sequence is the interesting part:

revision ep-cpu --all-targets engine native-backend
1b99d2232 ❌ E0061 ×2 (bench) ❌ E0061 ×2
ae7c6536a (after #1961) ❌ E0061 ×2 (bench) ✅
bb39883e3 (after #1958) ✅ ✅

So main spent the middle row still failing a required check, after a PR titled "repair the native-backend build, and widen the check that missed it". The widening in #1961 extended the engine step to onnx-genai-server — correct for #1944's class, but the ep-cpu bench was never a coverage gap at all: ci.yml:377 already compiled it with --all-targets, and that step runs before the one that got widened. The gate that would have caught it was present, ordered first, and red the whole time.

That is the shape I keep filing on #1817: a guard that was never executed in the failing direction. Here it is one step further along — the guard was executing and was failing, and three PRs were merged past it while the queue meant nobody read the colour. Widening a check that was already going to be red does not make it redder.

The concrete cost was ~40 minutes of my own work fixing something already fixed. The one thing that would have prevented it for all three of us is checking the first failing step of the required job rather than the one nearest the symptom.

The unrelated second red I mentioned above — native_generate_in_workflow_session_rejects_over_kv_byte_budget, added by #1900 and broken by #1892 — is still red on bb39883e3 and still unowned. Taking that one separately.

auto-merge was automatically disabled August 24, 2026 08:12

Pull request was closed

@justinchuby
justinchuby deleted the squad/gaff-fix-main-build-break-head-sink branch August 24, 2026 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant