Mtp upstream corroboration - #83
Merged
Merged
Conversation
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.
#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.