QVAC-24251 feat[bc|api]: drop n_discarded from the SDK config schema - #4163
Conversation
The llm addon no longer consumes n_discarded (#3938), so the key would reach llama's own argument parser and fail model load as an unknown option. Remove it from the zod schema so loadModel() rejects it at validation, regenerate the exported contract and the Python client, and drop it from every e2e model config. The KV-cache guide now describes the two overflow surfaces instead of recommending sliding.
parseContextOverflowMessage matched two of the addon's numeric overflow wordings. The two warm-cache guards and the multimodal single-prompt wording parsed as nothing, so the typed error carried no sizes — the multimodal gap is live against the published 0.47.x, not only against the new wordings. Every pattern now captures the context size last and sums the leading groups, KV cells are preferred where a guard reports positions too, and isAddonContextOverflowError recognises the 'at batch prefill step' wording on the message-only fallback. Sizes are validated as finite, and the tests assert the >= floor the guards actually trigger on, since the failing requirement can equal the window.
…kv-cache path toolBlockEvictable existed because the addon's discard window opened exactly where the static tool block sat, so while sliding was possible the block had to travel with every turn. Nothing evicts it any more, so a warm turn skips it whenever the prefix is known to hold a rendered one. The regression test now pins that a config still carrying the retired key does not force a resend.
…he e2e consumers A dedicated 512-token llm resource (same constant as llm, no new download) backs two completion tests on desktop, electron and mobile: a generation that fills the window must surface as the public stopReason 'length' with its produced tokens retained (predict is -1 and the prompt has no natural terminus, so the boundary is the only length source), and an oversized prefill must reject with the typed ContextOverflowError whose parsed sizes reach the window.
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):
|
…stion path Only the TypeScript options schema was strict, so the raw wire request, the plugin's loadConfigSchema and both deviceDefaults entries stripped the retired key and loaded with sliding silently off. All four now use llmConfigBaseSchema.strict(). The contract's LLM modelConfig gains additionalProperties: false, which the Python generator turns into extra="forbid", so a Python caller constructing a config with n_discarded gets a ValidationError instead of a silent drop. Rejection tests pin the wire schema, both deviceDefaults keys and the generated Python model.
…rflowError The parser summed cached totals — and, on multimodal guards, KV-cell counts — into the token-named promptTokens, so truncation logic could overestimate the prompt by the whole cached conversation and multimodal callers received cells under a token name. promptTokens now carries only figures the guard denominates in tokens; two new optional fields, cachedTokens and requiredTokens, carry the cached conversation and the total that failed the guard, in the same units as ctxSize. The fields serialize through toErrorResponseFields, rebuild in the SDK RPC reconstructor and the Python reconstructor, and the error message names both halves on a warm cache.
The guide promised stopReason "contextOverflow", which the public SDK never returns: the plugin maps the addon's context-boundary stop and a positive predict cutoff both to "length". Say so, and that stopReason alone does not distinguish the two.
The parser's most substantial additions are the cached-plus-prompt guards, and nothing public exercised them. A third consumer test caches a first turn that fills most of the 512 window under a per-run key, then sends a follow-up that fits the window alone but not on top of the cache, and asserts the typed error carries a requiredTokens no smaller than the window; the cache is deleted in cleanup. The cold-prefill test now also pins ctxSize to the configured 512, and the boundary-stop comment no longer claims an early EOS is impossible.
donriddo
changed the title
…w form Both emitters of "(N tokens, max M)" format a cached total (text: nPast_ + nTokens, multimodal: cacheTokens + nTokens), so on a warm cache the figure is not the prompt alone. The short form now maps to requiredTokens only, matching the field's prompt-only contract, and the short-form test pins the unset field. Comments trimmed to a line or two and version strings dropped from source.
The addon release carrying the sliding-context removal. Caret on 0.x is patch-only, so the pins in inference (dependency and peer) and sdk move to ^0.48.0 explicitly. The overflow parsers keep the older wordings, so a worker still on 0.47.x keeps parsing.
…quest schema Dispatch applies device defaults before the request schema runs, and the default-applying parse was non-strict, so it stripped n_discarded ahead of every strict validation added so far. llmConfigBaseSchema is now strict at the source, so that first parse and every derivation — wire, deviceDefaults, plugin loadConfigSchema, the public export — reject the key, and dispatch wraps the defaults parse so the failure surfaces as a structured RequestValidationFailedError. Regressions pin send() and the config-resolution path.
… structurally The catch-all's built-in exclusion was a zod refine, which does not serialize, so the exported contract and the generated Python union accepted a built-in modelType with arbitrary config through the permissive arm. A regex over the canonical types and aliases carries the rule into both. Closing the leak exposed the wire arms it had been masking: they required modelConfig even though the server injects defaults, so their optionality now mirrors the options schemas (llm, whisper, bci, embeddings, ocr optional; nmt, tts, audiogen required), and the transport tests that validated enum-typed requests through the leak now send the wire strings. Python tests pin the union, the public load_model() (rejects before the transport; custom types still pass) and the generated model.
…Size docs A lone requiredTokens can be a cold multimodal prompt in KV cells or the retired short form's cached total in tokens, so the message no longer blames a 'prompt spanning N KV cells' — it says what is known: the request needs N context tokens and no longer fits. ctxSize is documented as the effective per-request ceiling (ctx_size split across slots at parallel > 1), not the configured total, in the parser, both error classes, the Python docstring and the KV-cache guide. The parser's separators are horizontal whitespace only, matching its single-line claim. Message-level regressions pin the warm and required-only shapes.
…p failures The warm-cache e2e now asserts the first turn ended commit-eligible (no stop reason, non-empty output) and that the error carries a positive cachedTokens, so a rolled-back first turn or a cold full-history resend fails instead of passing through the plain prefill guard. A failed cache deletion fails the test rather than leaking a named cache. The boundary test replaces predict -1 with a 480 budget above the window's remaining capacity and asserts generatedTokens lands under it, so a 'length' stop provably means the boundary. The JS RPC round-trip test pins cachedTokens and requiredTokens across the envelope.
Comment blocks across the diff shrink to one or two lines each, and the tests/test_load_model.py import moves to its alphabetical position, which the Ruff import-order check requires.
…uality A lone or cached total can be KV cells, so the message says 'context units', and the guards trigger at equality — a generating request needs a free slot — so it is phrased as leaving no room to generate rather than exceeding, which read as a contradiction when the total equalled the capacity. The Python default message follows suit, and the stale class docs that still described a prompt against a configured window now describe the optional fields. Regressions pin the unit-neutral and equality wordings.
…rm assertions The custom-plugin regression dereferenced .root.root on a union arm mypy cannot narrow from the input dict, failing the required Python typecheck; an isinstance assertion narrows it. The dispatch regression now asserts the structured RequestValidationFailedError class, not just the message. The warm-cache executor deletes the per-run cache even when the flow throws before its inner handling.
The MtmdLlm guards trip on EITHER positions or KV cells against the same ceiling, but the mappings captured only one side, so a positions-dominant overflow reported a figure below the window — contradicting the requiredTokens >= ctxSize contract the fields document. Both figures are captured now and the larger is what failed the guard. Positions-dominant regressions pin all three multimodal wordings.
The enum's members are not str, so they only ever validated through the custom-plugin catch-all; with that arm excluding built-ins the public load_model() started rejecting them. The signature accepts the enum and coerces it to its wire string, with a regression pinning it.
The boundary test's 480 budget sat ~2% from the window's usable capacity, so a generous tokenizer could stop on the prediction cutoff and fail the boundary proof; 1000 is unambiguous. The warm test's first turn gets 48 tokens so a short ramble still ends on EOS and stays commit-eligible.
…on regex The base schema is strict, so the six per-site strict() wrappers are no-ops. A guard test pins that every built-in type and alias stays a plain kebab identifier, since the custom-plugin exclusion regex interpolates them unescaped.
…fault message A commit rollback returns normally and turn two re-primes the cache, so a positive cachedTokens alone does not prove the first turn survived — the assertion now requires a floor well above what a system-prompt prime can hold. The Python direct-construction default said 'exceeds', which is false at the equality boundary the guards trigger on; it now matches the TS wording's neutrality, with direct-construction regressions for the equality and warm shapes.
QVAC E2E —
|
QVAC E2E —
|
QVAC E2E —
|
QVAC E2E —
|
QVAC E2E —
|
yingying0906
left a comment
yingying0906
left a comment
There was a problem hiding this comment.
Reviewed at fbe5d4936. Two things I think need a change before merge, both in ops/context-overflow.ts, plus three notes on files this diff doesn't touch.
1. Scheduler cap overflows never become a typed error. The four per-sequence cap forms are in PRE_MUTATION_REFUSAL_FORMS but not in CONTEXT_OVERFLOW_FORMS, so with parallel > 1 the same "no room to generate" condition gives an untyped error. Same conversation at parallel: 1 gives ContextOverflowError. Details inline.
2. Addon media-load failures unlink a valid cache. The three loadMedia throws are pre-decode and pre-save, but they're not in the refusal list, so session.rollback deletes a committed cache file that was never touched. You already handle the SDK-side twin AttachmentNotFoundError, so this looks like an oversight. Details inline.
Three notes I can't anchor to a diff line:
rpc-error.ts:181 reads RECONSTRUCTORS[response.name] off an object literal, so the lookup walks Object.prototype. A payload with name: "constructor" resolves to the Object constructor, passes the truthiness check at :182, and Object(response) returns the response itself, so reconstructError returns a non-Error while the signature says Error. The other inherited keys throw and get caught at :186-198, so constructor is the only one that gets through. It's a pre-existing line, but you're changing the reconstructor it dispatches to. Fix is Object.create(null) or a hasOwnProperty gate. A typeof === 'function' check won't do it since Object is a function.
load-model.ts:157 and :286 wrap the base schema as llmConfigBaseSchema.strict(), so n_discarded now throws on the options path, while the wire schema at :566 and the Python client strip it. I can see from the test comment at llm-config-schema.test.ts:59-60 that the split is deliberate. Can it go in the PR body too? A JS caller gets a load-time throw and a Python caller gets silence for the same input, and right now that only lives in a test comment.
packages/rag/package.json:77 is still @qvac/llm-llamacpp: ^0.47.0 while inference and sdk went to ^0.48.0. It's a devDependency and rag has no source use of the addon, so nothing ships wrong, but a 0.x caret won't cross the minor so rag's tests run against the addon that still consumes n_discarded.
Rest of the parse layer checks out. I diffed all six ContextOverflow throw sites against the regexes character by character and the wordings, capture indices and units all match, including the exceeds? singular/plural split and ctxCeiling() matching the ctxSize doc. Detection is also harder to spoof than the old substring fallback.
…erve cache on media-load failures The scheduler's per-sequence-cap refusals are the same out-of-context condition the model guards report, so their wordings join the overflow forms and the parser maps cap to ctxSize (reservation plus prompt on the n_predict form) — previously the identical conversation was typed at parallel 1 and untyped above it. Interim wording-based fix; the addon carrying a real ContextOverflow status stays the recorded follow-up. The multimodal media-load failures join the pre-mutation refusals: they reject before any decode or save, like the SDK-side missing attachment, so the committed cache now survives them.
The reconstructor map is an object literal, so a hostile envelope name like "constructor" resolved through Object.prototype and returned a non-Error. Own-key gate plus a fall-through regression.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Rest all looks good, checked the fixes at Only thing left is |
Intentional. I omitted it because a separate PR might be better, raised as #4207. |
4 participants
🎯 What problem does this PR solve?
n_discardedin QVAC-23752 feat[bc]: remove sliding-context support from the llm-addon #3938, so the key reaches llama's own argument parser and fails model load as an unknown option. Every in-repo consumer still sends it.parseContextOverflowMessagematched two of the addon's numeric overflow wordings. The two warm-cache guards and the multimodal single-prompt wording parsed as nothing, so the typed error crossed the RPC with no sizes — and the multimodal gap is live against the published0.45.0/0.47.0, not only against QVAC-23752 feat[bc]: remove sliding-context support from the llm-addon #3938's new wordings.isAddonContextOverflowError's message-only fallback did not know theat batch prefill stepwording. The async transport strips status metadata from every run error, so in production the message alone identifies an overflow; the fallback now covers every emitted wording — the model guards' tagged forms and the batch scheduler's per-sequence-cap refusals, which are the same out-of-context condition and previously surfaced untyped atparallel >= 2— start-anchored so wrapper or cause-chain text cannot select cache preservation, and a present status code is authoritative — exactContextOverflowaccepts, any other code rejects without a message fallback. (Typing the scheduler refusals by wording is the interim fix; giving them a realContextOverflowstatus addon-side is a recorded follow-up.)promptTokens, so a caller's truncation logic could overestimate the current prompt by the entire cached conversation, and multimodal callers received cells under a token name. The error message had the same problem for the retired short form (it blamed a "prompt" of the cached-total size), andctxSizewas documented as the configuredctx_sizewhen the addon actually reports the effective per-sequence ceiling (ctx_size / parallelunder continuous batching).This re-does #3999 on top of the
inference/sdkpackage split; that PR editspackages/sdkserver files the split has since deleted, so its runtime changes are re-homed here into the canonicalpackages/inferencesources. The parser, schema and doc work is @yingying0906's.📝 How does it solve it?
n_discardedremoved from the zod schema, soloadModel()rejects it at validation, andcontract/schema.jsonplus the sdk-python client regenerated from it with the pinned generators.n_discardedis deleted from the config schema, following the retired-key precedent (toolsModeQVAC-22567 feat[bc]: remove SDK dynamic tools mode (toolsMode) #3380,no_mmapQVAC-24073 feat[bc]: adopt fabric b10297 consumers and replace no_mmap with load_mode #4078): the long-standing strictloadModel()options schema rejects it for TypeScript/JS callers, and the other ingestion paths strip unknown keys as they always have. No schema gained strictness or changed shape beyond the field removal (an earlier revision strictified the base and reworked the request union; both were reverted to match the recorded QVAC-24073 decision).n_discarded: 256.toolBlockEvictableis gone. It existed because the addon's discard window opened exactly where the static tool block sat, so while sliding was possible the block had to travel with every turn. Nothing evicts it now, so a warm turn skips it whenever the prefix is known to hold a rendered one.promptTokenscarries only figures the guard denominates in tokens — never a cached total, never KV cells. Two new optional fields onContextOverflowErrorcarry the rest:cachedTokens(the cached conversation a warm-cache guard reports) andrequiredTokens(the total that failed the guard), both in the same units asctxSize— KV cells, whichctx_sizecounts, so the pair stays comparable; cells equal tokens for text. The multimodal guards trip on EITHER positions or KV cells against the same ceiling; valid addon state keeps cells >= positions, so capturing the larger measure is defensive parsing (pinned by labelled malformed-input probes), andrequiredTokens >= ctxSizeholds on every reachable guard. The guards trigger on>=for a request that must still generate, sorequiredTokensmay equal the window; the tests assert the>=floor, not>. The fields serialize throughtoErrorResponseFields, rebuild in the SDK client's RPC reconstructor and the Python reconstructor, and the error message names both halves on a warm cache instead of blaming the appended prompt alone.llm, no new download): a generation that fills the window must surface as the publicstopReason: "length"with its produced tokens retained andgeneratedTokensbelow the 1000 predict budget (far above the whole window), proving the boundary rather than budget exhaustion fired (the plugin maps the addon's context-boundary stop to"length", there is no publiccontextOverflowstop reason); an oversized cold prefill must reject with the typedContextOverflowErrorwhose sizes reach the window and whosectxSizeequals the configured 512; and a real warm cache grown past the window must reject the follow-up with the warm signature — acachedTokensfloor well above what a fresh system-prompt prime can hold, so a silently rolled-back first turn cannot pass, after asserting the first turn ended commit-eligible — with the per-run cache deleted in cleanup and a failed deletion failing the test rather than being swallowed — and the same follow-up retried before cleanup must overflow warm again, pinning that the rejection did not destroy the committed cache.releaseTurn, a non-destructive counterpart used when the failure is a thrown addonContextOverflow, one of the addon's pre-mutationInvalidArgumentrefusals (the scheduler and generationParams guards, and the multimodal media-load failures), or anAttachmentNotFoundError(caller input rejected by the SDK before the addon runs) — the scheduler's per-sequence-cap and batcherAddStatussubmit rejections, and the generationParams apply-step validation (reachable atparallel = 1viaresponseFormat) — recognised by complete, end-anchored wordings (one per emitting guard; batcherAddStatusrestricted to its real error values). The async transport delivers the addon'sexception.what()alone — no status code — so the wording is the boundary; a status code is only checked when present, and must beInvalidArgument. Locks, refs and the deferred auto-cache retention sweep behave as on rollback; the disk cache and its recorded prefix survive. A cache the failing turn itself primed rolls back instead — a refused first request leaves no cache behind. Preservation is deliberately limited to these recognised cases — other pre-persistence failures (a media prompt on a text-only model, an empty history, an atomic-saveUnableToSaveSessionFilefailure that leaves the previous canonical file valid) still take the destructive default and are recorded follow-ups, pending a machine-readable addon status. Unit regressions pin the warm retry for the refusal shapes, throwing the production plain-Error shape the transport actually delivers (each fails with its branch reverted), and a session-level test pins that release preserves the file/prefix and admits a same-key waiter.n_discardedfor long conversations, named it as the fix for a prefill overflow, and promised astopReason: "contextOverflow"the public SDK never returns. It now describes the two ways a caller runs out of context, says the boundary surfaces as"length"and thatstopReasonalone does not distinguish it from a positivepredictcutoff, and thatloadModel()rejects the retired key at validation.Adoption. #3938 released as
0.48.0while this PR was in review, so the@qvac/llm-llamacpppins ininferenceandsdkmove to^0.48.0here, the same shape as theload_modeadoption in #4078. Parser compatibility with a0.47.xworker is pinned by exact-string unit tests (the retired wordings stay as patterns); the e2e suite in its current, strengthened form is proven against0.48.0— the warm-cache test requires the richcachedTokenssignature that0.47.x's short form does not emit. (Earlier, weaker versions of these e2e tests also passed against0.47.0before the bump.)🧪 How was it tested?
packages/inference: lint clean, full unit suite green (1603+ tests). That includes one case per addon guard in the context-overflow parser suite using the exact emitted strings with per-guard field expectations, the pre-existing0.47.xwordings kept as regression cases, a case assertingrequiredTokensreaches the window (>=, the floor the guards trigger on), the flipped KV-cache regression (a config still carrying the retired key no longer forces a tool-block resend), the schema-rejection tests for the options path, and the typed-fields round-trip for the new error fields.packages/sdk: lint, typecheck, unit tests andcontract:checkall clean.packages/sdk-python:generate.py --checkclean, pytest suite passes, including the reconstructor carryingcached_tokens/required_tokens.faf569b87; the later commits are unit-covered refinements on top): the three overflow tests passed on macOS, Linux, Windows, Android and iOS (iOS leg fully green). The only per-leg failures are outside this PR — an identicalrag-turbovec-ingest-searchmiss on all three desktops and one Androidcancel-broad-embeddings. Electron/Snap were skipped by that workflow, so Electron still lacks execution evidence (locally blocked by the harness--install-linksissue). Local desktop e2e, completion (50/50) and kv-cache (21/21) categories against the pinned0.48.0, coverscompletion-context-boundary-stop(stopReasonlengthat the boundary, produced tokens retained),completion-context-overflow-prefill(typedContextOverflowError, sizes reach the window,ctxSizeequals the configured 512),completion-context-overflow-warm-cache(a real cached first turn, follow-up refused with the failing total, cache deleted in cleanup), and the tool-block skip/resend paths thetoolBlockEvictableremoval touches.💥 Breaking Changes
n_discardedis removed from the llamacpp model config schema. A TypeScript/JSloadModel()call passing it fails validation (the options schema has been strict since long before this change); raw wire requests,deviceDefaults, and the generated Python client strip it like any unknown key — the same treatmenttoolsMode(#3380) andno_mmap(#4078) received. The behavioural change is the addon's: on 0.48 a conversation that previously slid its window now stops at the context boundary (stopReason: "length") or is refused up front with a typedContextOverflowError. (Release note: belongs in the CHANGELOG at the release cut.)BEFORE:
AFTER:
🔌 API Changes
ContextOverflowErrorgains two optional fields, in TypeScript and in the Python client (cached_tokens/required_tokens).promptTokensnow carries only figures known to be the prompt alone, in tokens — on a warm-cache overflow it is the appended prompt, not the cached total; multimodal guards that report KV cells leave it unset; and the published 0.47.x short form(N tokens, max M)leaves it unset too, since both of its emitters format a cached total.The canonical constructor takes the four measurements as one record —
new ContextOverflowError(contextSizes, modelId, cause)— with the positional form kept as a deprecated overload for existing callers.The error message is unit-neutral where the value can be KV cells — "Conversation uses 8201 context units (8170 already cached) and leaves no room to generate within the effective context capacity (8192 units)" — phrased around the free output slot so it also reads coherently at the
>=equality boundary, and it never blames a "prompt" of the cached-total size.