QVAC-22567 feat[bc]: remove dynamic tools (tools_compact) from llm-llamacpp addon - #3373
Conversation
Review StatusCurrent Status: ✅ APPROVED |
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):
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Mobile integration tests — @qvac/llm-llamacpp (iOS)Result: passed
|
Mobile integration tests — @qvac/llm-llamacpp (Android)Result: passed
|
QVAC E2E —
|
donriddo
left a comment
There was a problem hiding this comment.
isQwen3Architecture fix looks good. Exported from the header, six test cases pin the exact-match boundary including the qwen35 false case that was the original risk. Good to go.
Remove the dynamic tools feature (tools added mid-conversation and trimmed from the KV cache once the tool-call chain resolves). It only worked for Qwen3 via a custom dynamic template and relied on sliding-context anchor bookkeeping that does not carry over to newer models. - Delete ToolsCompactController and Qwen3ToolsDynamicTemplate - Stop parsing the `tools_compact` config key (breaking) - Strip the tools anchor plumbing from ContextSlider, ContextShifter, ReasoningBlockCompactor, Text/Mtmd contexts, LlamaModel, and the continuous-batch scheduler; general sliding/compaction is unchanged - Remove the two-pass (with/without tools) tokenization - Remove PromptLayout + validatePromptPolicy (existed only for tools_compact prompt-shape validation) - Remove runtimeDebugStats (nPastBeforeTools/toolsTrimmed) and getNPastBeforeTools - Delete feature unit/integration/mobile tests, docs, and examples; adapt shared tests to the new signatures No version bump; releasing is deferred to the release flow. SDK-side removal (toolsMode) is split into a companion PR. Introduced in #706 and #1379 (QVAC-16769).
- Tolerate a lingering `tools_compact` config key: erase it with a deprecation warning instead of letting it fall through to the generic CLI-arg conversion, where the unknown `--tools-compact` argument would throw InvalidArgument and fail the entire model load. Restores the pre-removal graceful behavior (no-op / ignored) and decouples merge and release ordering from the SDK-side removal (PR #3380). Adds a regression test (CommonParamsParseToolsCompactIgnored). - Remove the dead `hasKvCacheContext` block left in the continuous-batch admission path — a blocking file_size syscall performed under the scheduler lock whose only consumer (validatePromptPolicy) was deleted. - Reword two stale comments that still referenced the removed onGenerationCompletePolicy / generationStarted_ (ContinuousBatchScheduler .hpp finalizeTerminalDriver doc + .cpp notifyDone re-run rationale). - Fix the continuous-batching.md onCancel lifecycle row whose "Same policy as above" referred to the removed onGenerationCompletePolicy.
The tools_compact removal deleted `SupportsToolsCompactForModelMetadata ByArchitecture`, which was the only test pinning the exact-match `qwen3` arch predicate. That predicate survives (it still drives fixed-template selection via isQwen3Model -> getChatTemplateForModel) but had lost all direct coverage, so a regression that broadened it to match `qwen35` would slip through silently. Export `isQwen3Architecture` from ChatTemplateUtils (mirroring the already-exported `isQwen3ReasoningFamilyArchitecture`) and add `IsQwen3ArchitectureExactMatch` covering qwen3 (+case-insensitive) true and qwen35/qwen3moe/llama/"" false. Addresses PR #3373 review comment.
There was a problem hiding this comment.
QA review summary: the existing isQwen3Architecture thread is addressed. The main remaining concern is release ordering: this addon PR intentionally removes tools_compact/dynamic tools, but current SDK and inference code still expose toolsMode dynamic and emit tools_compact=true. Before release, the companion SDK/inference PR should remove or loudly deprecate that public mode so users do not get silent behavior changes. The test ask is limited to proving the remaining static tool-calling path, or covering dynamic-mode rejection/deprecation in the companion PR. QIP triage: public addon/SDK contract removal trigger applies; proposal scope is removal and release ordering of dynamic tools.
tobi-legan
left a comment
There was a problem hiding this comment.
Re-reviewed the latest commits. Prior comments are addressed: tools_compact=true now rejects clearly, tools_compact=false remains an ignored tombstone, and the warm tool-calling follow-up now validates declared parseable tool calls. No further findings from my pass.
🎯 What problem does it solve?
The dynamic tools feature (
tools_compact) lets a caller add tool definitions mid-conversation and have them trimmed from the KV cache once the tool-call chain resolves. It only works for Qwen3 — it depends on a custom Qwen3 dynamic template — and on sliding-context anchor bookkeeping that does not carry over to newer LLM models. This PR removes the feature from the llm-llamacpp addon.Introduced in #706 and #1379 (QVAC-16769). The SDK-side removal (
toolsMode) is split into companion PR #3380.📝 How is it solved?
ToolsCompactControllerandQwen3ToolsDynamicTemplate(normal Qwen3 templating keeps usinggetFixedQwen3Template)tools_compactconfig key is no longer parsedContextSlider,ContextShifter,ReasoningBlockCompactor,TextLlmContext/MtmdLlmContext,LlamaModel, andContinuousBatchScheduler— general context sliding, M-RoPE sliding, and reasoning compaction are unchangedPromptLayout/validatePromptPolicyprompt-shape validation chain, and thenPastBeforeTools/toolsTrimmeddebug statstools-compactintegration + mobile test suites, docs, and examples; adapted shared tests to the new signatures (kept general coverage — e.g. the tail-trim interaction tests were retained and renamed since the branches they pin still exist)🧪 How is it tested?
bare-make generate/build/install) and C++ unit tests — 704 ran, 698 passed, 0 failed, 6 pre-existing environmental skips (GPU-gated / fixture-gated); JS lint clean; mobile test manifest regenerated with group coverage validatedtest-linux-x64,test-darwin-x64,test-win32-x64) were verified byte-identical to same-day pre-PR baseline runs on other branches (pre-existing, non-gating)tools_compact|toolsCompact|tools_at_end|Qwen3ToolsDynamic|nPastBeforeTools|PromptLayout|validatePromptPolicyreturns only historical changelog entriesYes — the dynamic tools feature is removed. A lingering
tools_compactconfig key is now accepted but ignored with a deprecation warning (a tombstone incommonParamsParse) for one release, so existing configs — including older SDK builds that still emit the key — keep loading rather than failing model load on an unknown argument. Plan to drop the tombstone (making the key an error) in a future release once no consumer emits it.BEFORE:
AFTER: