From 88878dc23a5f475266f693009b40d6a6fb9a8da3 Mon Sep 17 00:00:00 2001 From: Niko Maroulis Date: Thu, 6 Aug 2026 00:24:40 -0400 Subject: [PATCH 1/2] Corroborate the MTP rollback fix against upstream's reference path The off-by-one entry rested on reading our own code and reasoning about what the KV should contain. llama.cpp implements the same accept step for its server, so it can be checked rather than argued. common_sampler_sample_and_accept_n (common/sampling.cpp) samples at batch indices 0..k and returns every accepted token, the one at index 0 being the token that follows the `sampled` occupying batch element 0. server-context.cpp then commits it as: slot.prompt.tokens.insert({ids.begin(), ids.end() - 1}); slot.sampled = ids.back(); slot.mem.seq_rm(slot.id, slot.prompt.tokens.pos_next(), -1); Every accepted token except the last goes into the context; the last is carried into the next iteration as `sampled` and decoded there. That is exactly the invariant this branch restores, arrived at independently, which is about as good a second opinion as is available without an MTP GGUF to measure acceptance on. Worth noting the mechanisms differ: upstream trims with seq_rm because its verify batch already wrote the accepted prefix at the right positions, while this binding rolls the target back to n_past and re-decodes. Only the resulting context has to agree, and now it does. Whether the re-decode is needed at all is a separate question this does not touch. --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b68210f..4b10232 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,15 @@ the GPU, and passes the smoke suite: **528 tests, 0 failures**. Only reachable on a partial accept, which is why a working MTP setup can still post plausible acceptance rates while quietly drifting. + + Cross-checked against upstream's own reference path rather than reasoned about + alone. `common_sampler_sample_and_accept_n` (`common/sampling.cpp`) returns the + tokens sampled at batch indices `0..k`, and `tools/server/server-context.cpp` + then does `slot.prompt.tokens.insert({ids.begin(), ids.end() - 1})` followed by + `slot.sampled = ids.back()` — appending every accepted token *except the last*, + and carrying the last into the next iteration. That is the invariant this fix + restores. (Upstream trims with `seq_rm` where this binding rolls back and + re-decodes, so the mechanisms differ; the resulting context must not.) - **The CUDA NIF could not be loaded** — `ggml-cuda.a` leaves the CUDA runtime, cuBLAS/cuBLASLt and the CUDA driver API unresolved, but the Linux link line only ever added `-lstdc++ -lm -lpthread`. The resulting `.so` linked and then From 280ac601b3e3bfd832fd11f3eaf46775f0a6e766 Mon Sep 17 00:00:00 2001 From: Niko Maroulis Date: Thu, 6 Aug 2026 00:51:58 -0400 Subject: [PATCH 2/2] ci: stop paying twenty minutes to re-test a tree we just tested #70's master run was cancelled before its matrix even expanded, which is a reasonable thing to do to a merge that has just added ~20 minutes of nvcc to every landing. But cancelling it leaves master with no verdict at all, so the fix is to make the job cheap to leave on rather than something worth killing. A squash merge of an up-to-date branch lands exactly the tree the pull request tested, and re-running the CUDA legs against it re-confirms a known answer. The case that genuinely needs re-testing is master moving underneath the branch -- v0.8.42 landed under #70 mid-review, so this is not hypothetical. So the job now runs on every pull request, where it is the gate, and on master only when a CUDA build input moved: Makefile, c_src/, mix.exs, mix.lock, the vendor/llama.cpp submodule, or the workflow itself. Docs and Elixir-only merges skip it. Everything unexpected resolves to running: a force-push, an absent base commit, a zero SHA. The decision is computed with git rather than a third-party paths filter, to avoid adding an action to the surface this repo takes care to pin, and it is written as plain `if` blocks because whether `set -e` exits on a `test ... && cmd` whose left side is false is a corner of the standard nobody should have to recall while editing CI. Simulated across seven cases: PR runs, docs-only push skips, Makefile / submodule / mix.exs pushes run, empty and zero base SHAs run. The workflow also gains a concurrency group, since two runs on one branch now means two twenty-minute jobs queueing behind each other. master keys on run_id so landed commits are never cancelled by a later push -- the failure mode this commit exists to fix. Also carries the changelog note corroborating #70's MTP rollback fix against upstream's reference path, which was written after that PR merged. --- .github/workflows/ci.yml | 64 +++++++++++++++++++++++++++++++++++++++- CHANGELOG.md | 11 +++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bf33c31..6d7f4ae 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,6 +11,14 @@ on: permissions: contents: read +# Without this, pushing twice to a branch leaves both runs going and they queue +# behind each other — which matters now that the CUDA jobs hold a runner for +# ~20 minutes. master is excluded from cancellation by keying on run_id there: +# every commit that lands deserves its own verdict, superseded or not. +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref == 'refs/heads/master' && github.run_id || github.ref }} + cancel-in-progress: true + env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true MIX_ENV: test @@ -192,6 +200,59 @@ jobs: - run: mix dialyzer + # Each CUDA leg holds a runner for ~20 minutes, almost all of it nvcc. On a + # pull request that is the price of the gate and it gets paid. On master it + # usually re-confirms a tree the pull request just tested, because a squash + # merge of an up-to-date branch lands exactly that tree. + # + # "Usually" is the whole question: master can move under a branch — v0.8.42 + # landed under this one mid-review — and then the merged tree is genuinely + # untested. So the answer is not "skip master", it is "run master when the + # inputs moved". Anything unexpected resolves to running it. + cuda-scope: + name: CUDA inputs changed? + runs-on: ubuntu-22.04 + outputs: + changed: ${{ steps.q.outputs.changed }} + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + - id: q + env: + EVENT: ${{ github.event_name }} + BEFORE: ${{ github.event.before }} + run: | + set -eu + run_it() { echo "changed=true" >> "$GITHUB_OUTPUT"; echo "$1"; exit 0; } + + # Written as plain `if` blocks rather than `test ... && run_it`: under + # `set -e` a false left-hand side makes the whole && list non-zero, and + # whether that exits the shell is a corner of the standard nobody should + # have to recall while editing CI. + if [ "$EVENT" = "pull_request" ]; then + run_it "pull request: the link job is the gate" + fi + + # A force-push or a first push leaves no usable base. Unknown means run. + case "$BEFORE" in + ''|0000000*) run_it "no usable base commit; running to be safe" ;; + esac + if ! git cat-file -e "${BEFORE}^{commit}" 2>/dev/null; then + run_it "base commit ${BEFORE} not present; running to be safe" + fi + + # vendor/llama.cpp is a submodule, so a bump shows up as a change to + # that path — which is exactly the case that must not be skipped. + if git diff --name-only "$BEFORE" "$GITHUB_SHA" | tee /tmp/changed.txt \ + | grep -qE '^(Makefile|c_src/|mix\.exs|mix\.lock|vendor/llama\.cpp|\.github/workflows/ci\.yml)'; then + run_it "a CUDA build input changed" + fi + + echo "changed=false" >> "$GITHUB_OUTPUT" + echo "no CUDA build input in this push; skipping the link job" + cat /tmp/changed.txt + # Nothing in CI ever selected the CUDA backend, which is how a NIF that could # not resolve `cuMemCreate` reached a release. Runners have no GPU, so this # cannot execute a kernel; it proves the two things that were actually broken: @@ -200,7 +261,8 @@ jobs: cuda-link: name: CUDA link (cu${{ matrix.major }}) runs-on: ubuntu-22.04 - needs: [setup] + needs: [setup, cuda-scope] + if: needs.cuda-scope.outputs.changed == 'true' strategy: fail-fast: false matrix: diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b10232..b8032b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,6 +103,17 @@ the GPU, and passes the smoke suite: **528 tests, 0 failures**. with `ldd -r` — pointing the loader at the toolkit's driver stub, which carries the `libcuda.so.1` soname, so a GPU-less runner can still perform a real resolution. `enif_*` is excluded, being supplied by the BEAM at load. + + Each leg holds a runner for about twenty minutes, almost all of it nvcc, so + the job runs on every pull request but on master only when a CUDA build input + moved — `Makefile`, `c_src/`, `mix.exs`, `mix.lock`, the `vendor/llama.cpp` + submodule, or the workflow itself. A squash merge of an up-to-date branch + lands the exact tree the pull request tested; the case worth re-testing is + master having moved underneath it, which is what that path list detects. A + force-push, a missing base commit, or anything else unexpected resolves to + running the job. The workflow also gained a `concurrency` group so repeated + pushes to a branch cancel their predecessors instead of queueing behind them, + keyed on `run_id` for master so every landed commit keeps its own verdict. - **Precompiler unit tests** — `test/precompiler_test.exs` pins the artifact selection rules, including the case that motivates the driver check.