Skip to content

ci(bump-hrx): resolve ggml/src/ggml-hrx conflicts toward our line, keep failing elsewhere - #335

Merged
bong-water-water-bong merged 2 commits into
mainfrom
fix/bump-hrx-resolve-ours
Oct 6, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
fix/bump-hrx-resolve-ours

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes the failing scheduled HRX pin sync (engine#332).

What failed

The scheduled bump-hrx run has failed on every run from 2026-10-01 to 2026-10-05 (the pin silently froze; #328 fixed the merge strategy, this fixes the content resolution). The step "Sync the fork and merge AMD's pin into our line" exits 1 at the merge of AMD's b802a507b into 1bit/hrx-vulkan-patched: 15 conflicting files, all under ggml/src/ggml-hrx/. The ancestry guard passes (1bit/hrx-vulkan is an ancestor of b802a507b with 0 extra commits), so the failure is the merge, not the guard.

Why the conflicts recur

Both lines rewrote ggml/src/ggml-hrx/ — AMD's kernel-family refactor against our backend. It will conflict on every scheduled run until one side's ggml-hrx is chosen once and committed as the merge resolution.

Which side — measured, not assumed

Fresh build directories, the engine's own ExternalProject arguments (cmake/hrx.cmake: fresh -B, Release, amdclang, -DHRX_SOURCE_DIR=…), full test-backend-ops -b HRX0, and a GLM-4.7-Flash Q4_K_M 269-token prompt at -ub 512:

build full gate test-backend-ops -b HRX0 model
AMD core + our ggml-hrx 1073/1073 — 0 FAIL 0 NaN, reply ' Paris.'
AMD core + AMD's ggml-hrx 1019/1071 — 54 FAIL 18 NaN, reply ??????

The 40 MUL_MAT failures (iq1_s/iq1_m/iq3_xxs/mxfp4/pq2_0/ptq1_0/q2_*) and the engine#315 all-NaN logits are properties of AMD's ggml-hrx, not of the core sync. Resolving these files toward AMD's side would import #315's NaN into the pinned line.

What this does

  • Conflicts inside ggml/src/ggml-hrx/ resolve toward ours (the branch being merged into) — the patched line.
  • AMD's side is kept everywhere else.
  • Any conflict outside ggml/src/ggml-hrx/ still aborts loudly (::error::), so a real divergence cannot be auto-resolved by accident.
  • Emits a ::notice:: naming the files it took.

Verification

python3 -c "import yaml; yaml.safe_load(open('.github/workflows/bump-hrx.yml'))"   # YAML OK

Conflict resolution logic A/B-simulated on a scratch repo: only-ggml-hrx conflict → merge commit with our side, AMD side elsewhere; mixed conflict → abort. Patch applies cleanly on current main (ef04beb, bump-hrx.yml blob 90e36c0).

Credit: the measurement and resolution were developed in the notes on engine#332; this PR lands them. Blocked until now only by the workflow scope on the pushing credential.

…ep failing elsewhere

The scheduled sync fails every run at the merge of AMD's pin into 1bit/hrx-vulkan-patched: 15
conflicting files, all under ggml/src/ggml-hrx/ (the workflow header says so, and I confirmed
the ancestry guard passes: 1bit/hrx-vulkan is an ancestor of AMD's b802a507b with no extra
commits, so the failure is the merge, not the guard).

Measured 2026-10-06 in fresh build directories with this workflow's own ExternalProject
arguments, three requests of a 269-token prompt at -ub 512:

  AMD core + our ggml-hrx   full gate 1073/1073 (0 FAIL)   0 NaN, reply " Paris."
  AMD core + AMD ggml-hrx   full gate 1019/1071 (54 FAIL)  18 NaN, reply "??????"

So resolving these files toward AMD's side imports engine#315's all-NaN logits into the
pinned line. The merge now takes ours for conflicts inside ggml/src/ggml-hrx/ and keeps
AMD's side everywhere else, and still aborts loudly on any conflict outside that path so
a real divergence cannot be auto-resolved by accident.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Careful with kDecodeSplitMaxKeyValueTokenCapacity — the right value depends on which side wins here

Resolving ggml/src/ggml-hrx toward our line is a different outcome from the AMD-side merge that was on the previous pin bump, and it changes a constant in dispatch_registration/common/dispatch-flash-attention.cpp from wrong to right:

provider set kDecodeSplitMaxKeyValueTokenCapacity
ours (monolithic FA kernel: direct_f32 64-256, cooperative_f32 257-2048, multipass_f32 2049-262144) 32768 is correct
AMD's refactor (ops/flash_attention/* + motifs/flash_attention/*: only direct_f32 64-256 and cooperative_f32 257-2048 — multipass removed) must be 2048

Measured on the AMD-side merge: with only those two providers, a capacity of 2049-32768 offers the decode-split dispatch with no provider, so the kernel selector rejects every candidate (all_rejected) and the whole decode fails — which is exactly what the comment above that constant warns about. I capped it to 2048 there for that reason.

So: if this PR restores our provider set, keep 32768 and do not carry the 2048 cap over. The constant and the corpus must agree, and nothing in CI catches the mismatch — it only shows up as a decode failure on a long context.

Verified tree-wide that with the AMD corpus the only two providers are the ones listed (reduce_fused_direct_f32, reduce_fused_cooperative_f32, both in motifs/flash_attention/completion_counter_reduce.loom) and reduce_fused_multipass_f32 exists nowhere.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 15f4beb)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

332 - Fully compliant

Compliant requirements:

  • Fix the failing scheduled HRX pin sync workflow
  • Resolve merge conflicts in the bump-hrx CI job
  • Ensure the workflow does not fail silently anymore

328 - Fully compliant

Compliant requirements:

  • Change merge strategy from rebase to merge to avoid 159-step replay
  • Failures now speak by naming conflicting files in error messages
  • Keep unchanged safety mechanisms (ancestry guard, force-with-lease push)
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Merge Conflict Resolution Logic

The new logic attempts to resolve merge conflicts by keeping the 'ours' version for files under ggml/src/ggml-hrx/, but it assumes that this choice is safe and correct without sufficient validation. The PR description mentions that this was measured to be safe, but the actual impact of choosing 'ours' over 'theirs' for these specific files needs to be verified against the runtime behavior of the HRX backend. If the 'theirs' version (AMD's) introduces critical changes that affect correctness or performance, this could lead to silent regressions.

if ! git -c user.name=hrx-bump -c user.email=hrx-bump@users.noreply.github.com merge --no-edit --no-ff "$AMD_LLAMA"; then
  conflicts=$(git diff --name-only --diff-filter=U || true)
  outside=$(printf '%s\n' "$conflicts" | grep -v '^ggml/src/ggml-hrx/' | grep -v '^$' || true)
  if [ -n "$outside" ]; then
    git merge --abort || true
    echo "::error::merging AMD's ${AMD_LLAMA:0:12} into 1bit/hrx-vulkan-patched conflicts outside ggml/src/ggml-hrx: $(printf '%s ' $outside); resolve those files by hand"
    exit 1
  fi
  # Every conflict is inside our HRX backend. Take ours (the branch being merged
  # into, i.e. the patched line) and keep the rest of AMD's pin.
  #
  # Measured 2026-10-06, fresh builds with this workflow's own ExternalProject
  # arguments - AMD's core plus our ggml-hrx: full gate test-backend-ops -b HRX0
  # 1073/1073 with 0 FAIL, and GLM-4.7-Flash at -ub 512 returns " Paris." with 0
  # NaN events. AMD's own ggml-hrx on the same core: 1019/1071 with 54 FAIL and 18
  # NaN events (engine#315). Resolving these 15 files toward AMD's side therefore
  # imports that NaN into the pinned line, so the merge keeps our files.
  printf '%s\n' "$conflicts" | while read -r path; do
    [ -n "$path" ] && git checkout --ours -- "$path" && git add -- "$path"
  done
  git -c user.name=hrx-bump -c user.email=hrx-bump@users.noreply.github.com commit -q --no-edit
  echo "::notice::merged AMD's ${AMD_LLAMA:0:12} keeping our ggml/src/ggml-hrx for: $(printf '%s ' $conflicts)"
fi

@bong-water-water-bong
bong-water-water-bong enabled auto-merge (squash) October 6, 2026 19:33
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 15f4beb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant