Conversation
Contributor
Review StatusCurrent Status: ❌ PENDING Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member. |
Contributor
License compliance — cleanNo new dependency license findings in this PR. Warn-only (shadow) mode — this check does not block merges yet. Updated automatically by the canonical license compliance workflow. NOTICE presence (advisory)Missing NOTICE (advisory, does not block):
|
dev-nid
added
run-cpp-addon-tests
yingying0906
force-pushed
the
pr-2938-mtp-llamacpp
branch
from
September 10, 2026 15:59
697bb17 to
330d846
Compare
yingying0906
temporarily deployed
to
release
September 10, 2026 16:00 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Workflow security (shadow mode)zizmor found 1103 finding(s) in Findings are annotated inline on the changed files and listed in the job summary. Reproduce locally: pipx run zizmor==1.27.0 --offline .github/ |
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
1 similar comment
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
Signed-off-by: Ridwan Taiwo <donriddo@gmail.com>
Signed-off-by: Ridwan Taiwo <donriddo@gmail.com>
Signed-off-by: Ridwan Taiwo <donriddo@gmail.com>
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
1 similar comment
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
1 similar comment
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
gianni-cor
requested changes
Sep 18, 2026
gianni-cor
left a comment
gianni-cor
left a comment
Contributor
There was a problem hiding this comment.
One remaining MTP cache lifecycle issue needs a target-only fallback.
| std::vector<uint8_t> driverState; | ||
| uint64_t generation = 0; | ||
| if (saveMtpState) { | ||
| if (!common_speculative_get_state(spec_.get(), seqId_, driverState)) { |
Contributor
There was a problem hiding this comment.
[P2] Fall back to a target-only cache when MTP state is unavailable
rollbackDraftContext() can intentionally clear the draft KV and set pending_pos to -1 after cancellation while keeping a valid target prefix. That is a valid cold MTP state, but common_speculative_get_state() returns false for it. Throwing here turns an otherwise usable target cache into a failed save and invalidates the active cache. Please save the four-field target cache, remove or ignore stale .mtp-draft and .mtp-state files, and let MTP rebuild on a later request. Please apply the same fallback to MtmdLlmContext and add a cacheKey plus saveCacheToDisk cancellation test.
Contributor
There was a problem hiding this comment.
Fixed in dd8ab7572. Both TextLlmContext and MtmdLlmContext now save a target-only cache after cancellation and remove stale MTP sidecars. Added cancellation tests using cacheKey and saveCacheToDisk for both paths.
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
1 similar comment
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
gianni-cor
previously approved these changes
Sep 18, 2026
gianni-cor
left a comment
gianni-cor
left a comment
Contributor
There was a problem hiding this comment.
The target-only cache fallback is now handled correctly in both Text and MTMD, with cancellation and cold-restore regression coverage.
# Conflicts: # .github/workflows/integration-test-embed-llamacpp.yml
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
1 similar comment
Contributor
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
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
7 participants
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.
What problem does this PR solve?
draft-mtpself-speculative decoding support to@qvac/llm-llamacpp.How does it solve it?
draftAcceptedanddraftTotalruntime statistics.The MTP driver-state support and position-aware fresh-sequence guard are in qvac-fabric PR #269, including commit 9b14d8473. The pending MTP row is serialized with its target position and is only reused when the next batch starts at the consecutive position. Invalid state blobs are ignored without overwriting valid pending state.
This qvac branch carries a temporary fabric overlay so the addon and fabric changes can be validated together. The overlay is pinned to PR #269 directly, with no qvac-local fabric patch. It will be removed and replaced with the published fabric baseline after PR #269 lands.
Why are workflow and overlay changes included?
The Embed integration workflow downloaded an artifact named
prebuilds, but the reusable prebuild workflow publishesprebuilds-embed-llamacpp. The job therefore failed before any integration test started. The workflow file is repository code, so the artifact name must be fixed in this PR for the tests to run.The automatic PR workflow uses the reusable workflow from
main, so it still runs the old artifact name and cannot validate a workflow change from the PR branch. A manual dispatch from this branch used the corrected workflow and passed all seven platform jobs.The addon cache restore also depends on the MTP driver-state API from fabric PR #269. Registry fabric does not contain that API yet, so the temporary overlay lets CI build the qvac and fabric changes together. This validates the integration before fabric is published. The overlay is not the final dependency update and will be removed after PR #269 lands.
Benchmark results:
1.13xto1.21xTPS.1.80xto2.15xTPS.Validation
git diff --checkpasses.Mobile Device Farm runs
Pinned to a dev build of this branch because an unpinned dispatch installs the published
@latest, which has no MTP.@tetherto/llm-llamacpp-mono@0.52.0-tmp.runid-34784321981fromtmp-mtp-llamacpp-4390atedfbe3c2evia publish runmtpshard withrunMtpTestandrunMtpMtmdTestpassed on both platforms.The one iOS failure is
runGemma4Test, which does not use MTP. The app crashes while loading the Gemma 4 f16 mmproj inGemma 4 multimodal honours remove_thinking_from_context. It reproduces identically on the published@qvac/llm-llamacpp@0.52.0with none of this branch's code in the control run, and reproduces across three runs.Breaking changes
None. MTP is opt-in through
spec-type=draft-mtp. Unsupported models fall back to normal decoding, and the new statistics fields are additive.Known limitations
parallel=1. When continuous batching is enabled, the addon logs a warning and falls back to non-speculative decoding. Fabric's llama-server supports MTP with continuous batching, but the addon's scheduler does not yet manage the required per-slot speculative state. Scheduler integration is deferred to a follow-up change.