Skip to content

QVAC-23752 feat[bc]: drop n_discarded from the SDK config schema - #3999

Open
yingying0906 wants to merge 1 commit into
mainfrom
feat/QVAC-23752-sdk-drop-n-discarded
Open

QVAC-23752 feat[bc]: drop n_discarded from the SDK config schema#3999
yingying0906 wants to merge 1 commit into
mainfrom
feat/QVAC-23752-sdk-drop-n-discarded

Conversation

@yingying0906

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

  • The llm-llamacpp addon stops consuming n_discarded in 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.
  • parseContextOverflowMessage matched one of the addon's five numeric overflow wordings. The two that report a warm cache parsed as nothing, and on a warm cache it reported the appended prompt alone, which renders as "31 prompt tokens exceeds the 8192-token context window".
  • isAddonContextOverflowError did not know about at batch prefill step, which both text guards and the MTMD batch guard emit, so on the message-only path those errors reached the client raw and instanceof ContextOverflowError failed.

📝 How does it solve it?

  • n_discarded removed from both zod schemas, so it is no longer accepted or forwarded, and contract/schema.json plus the sdk-python client regenerated from them with the pinned generators. One field each.
  • Removed from every e2e config that passed n_discarded: 256.
  • toolBlockEvictable is 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.
  • Every overflow pattern now captures the context size last and the parts of the failing requirement before it, so promptTokens is the sum of the leading groups. One rule covers both shapes: the cached guards have two leading groups, the rest have one and the sum is that figure. Where a guard reports positions and KV cells, the cells are captured, since M-RoPE media occupies more cells than positions.
  • The KV-cache guide told readers to add n_discarded for long conversations and named it as the fix for a prefill overflow. It now describes the two ways a caller runs out of context.

Ordering. This does not depend on #3938 shipping first. The old patterns are kept alongside the new ones, so the parsers match what 0.45.0 emits today and what #3938 will emit later, and no addon version pin moves here. Worth knowing for review: the e2e configs passed n_discarded: 256, which with the currently published addon turns sliding on, so dropping it means those runs no longer slide. That is the permanent behaviour once #3938 is released.

🧪 How was it tested?

  • Context-overflow parser suite: 10/10 in both sdk and inference, with one case per addon guard using the exact emitted strings, the pre-existing older wordings kept as regression cases, and a case asserting the reported figure exceeds the window, which is what makes the formatted message coherent.
  • KV-cache tool-block suite: 5/5 in both packages. The case that asserted a resend under sliding now asserts that a config still carrying the retired key does not force one, so the removal keeps a regression guard.
  • contract:check and the sdk-python generate.py --check both clean, so the generated files match their sources.
  • prettier and lint clean in both packages.

💥 Breaking Changes

n_discarded is no longer accepted in the llamacpp model config, and it is gone from the exported contract and the generated Python client.

BEFORE:

await loadModel({
  modelSrc: MODEL,
  modelType: 'llm',
  modelConfig: { ctx_size: 2048, n_discarded: 256 }
})

AFTER:

await loadModel({
  modelSrc: MODEL,
  modelType: 'llm',
  modelConfig: { ctx_size: 2048 }
})

@yingying0906
yingying0906 requested review from a team as code owners August 21, 2026 15:39
@github-actions

Copy link
Copy Markdown
Contributor

Review Status

Current Status: ❌ PENDING
Approvals so far: none

Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member.

@github-actions

Copy link
Copy Markdown
Contributor

License compliance — clean

No 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):

  • ./.github/actions/release-merge-guard
  • ./docs/website
  • ./packages/ggml-coload-smoke
  • ./packages/fabric/test/integration
  • ./packages/inference-addon-cpp/mobile
  • ./packages/sdk/e2e
  • ./packages/llm-llamacpp/benchmarks/performance
  • ./packages/llm-llamacpp/benchmarks/server
  • ./packages/vla-ggml/sim/server
  • ./packages/embed-llamacpp/benchmarks/performance
  • ./packages/embed-llamacpp/benchmarks/server
  • ./packages/asr-ggml/benchmarks/server

yingying0906 added a commit that referenced this pull request Aug 24, 2026
The addon could evict tokens from the middle of the KV cache once the
context filled, during prefill or decode, and tracked `firstMsgTokens` so
the system prompt and tool definitions were never the tokens dropped. It
was opt-in through `n_discarded` and defaulted to off, so the shipped
behaviour was already the non-sliding path while the slide machinery
added state every other path had to keep correct. The generation-time
slide also invalidated a tracked `<think>` span, which wiped the sequence
and hard-failed instead of shifting the span.

- Removed `ContextSlider` (slide policy) and `ContextShifter` (slide
  budget and counter). The surviving KV primitives, the ops seam plus
  `compactKvRange`, move to `KvCacheOps` since reasoning-block compaction
  still needs them.
- Removed `firstMsgTokens` / `protectedPrefix` from both contexts and the
  `LlmContext` / `SequenceDriver` accessors that exposed them, along with
  `slideCapable` admission handling, `applySlide`, `supportsSliding` and
  `SequenceStepResult::discarded`.
- Removed the compactor's slide-invalidation state and its
  `FailedKvWiped` branch, which nothing could reach once the shifter
  went.
- Overflow is now the single path `n_discarded=0` already took. A prefill
  that does not fit throws `ContextOverflow`. A generation that fills the
  window stops with `stopReason=contextOverflow` and still returns what
  it produced. The generation check is a plain "no room for even one more
  token" test, so a caller can tell a full context from a
  prediction-limit cutoff.
- A batched slot never reached that check. `advance` marks the request
  the moment `currentPos` hits `maxTokensPerSequence` and `isFinished`
  includes that, so the slot is filtered out before the driver is asked,
  and ordinary text generation reported `sequenceLimit` for a full
  window. That limit IS the slot's share of the context, and `submit`
  rejects any request whose prompt plus a positive `n_predict` would not
  fit, so a full window is the one thing it can mean. It now says so.
- Both prefill guards name the quantity they report, so `cached tokens N
  plus prompt tokens M` is no longer formatted into the same field as the
  slice alone.
- Session metadata keeps its four-slot width so a file written by either
  build still loads, with slots 1 and 3 written as 0. `ContextSlideFailed`
  stays reserved so a new code does not reuse 26, which builds up to
  0.45.0 emit. Rolling this package back over an existing cache dir does
  drop the system prompt, because the old build reads slot 1 as its
  protected prefix and 0 makes it evict from position 0.
- Leftovers cleared: a dead `applyContextDiscard` declaration, the unused
  `IKvCacheOps::nCtx`, comments still describing sliding, `%zu` for two
  `size_t` counts formatted `%ld`, and the OpenCL error string that still
  said "sliding context".

`sliding-context.test.js` and `mrope-sliding-context.test.js` are deleted,
with all three mobile registries updated so a run does not abort on an
unregistered test. `KvCacheOpsTest` holds the 7 `compactKvRange` cases
moved out of `test_context_slider.cpp`, and the plain text overflow case
is back as a direct test in `api-behavior.test.js`.

`n_discarded` is no longer consumed, so it reaches llama's own argument
parser and fails model load as an unknown option. The SDK half is in
#3999.
@yingying0906
yingying0906 force-pushed the feat/QVAC-23752-sdk-drop-n-discarded branch from 15de969 to 0b19d85 Compare August 24, 2026 06:17
@yingying0906 yingying0906 added run-cpp-addon-tests CI: run C++ addon tests (requires verified) run-desktop-addon-tests CI: run desktop integration tests (requires verified) labels Aug 24, 2026
The llm-llamacpp addon stops consuming `n_discarded` in #3938, so the key
reaches llama's own argument parser and fails model load as an unknown
option. Every in-repo consumer still sent it.

- Removed from both zod schemas, so it is no longer accepted or
  forwarded, and regenerated `contract/schema.json` plus the sdk-python
  client from them with the pinned generators. One field each.
- Removed from every e2e config that passed `n_discarded: 256`.
- `toolBlockEvictable` is 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. The case that asserted the resend now asserts that a
  config still carrying the retired key does not force one, so the
  removal keeps a regression guard.
- `parseContextOverflowMessage` matched one wording, so only one of the
  addon's five numeric guards parsed. The two that report a warm cache
  give the cached size and the appended prompt separately, and taking the
  appended half alone handed truncation logic a number two orders of
  magnitude too small. Every pattern now captures the context size last
  and the parts of the failing requirement before it, so `promptTokens`
  is the sum of the leading groups. Where a guard reports positions and
  KV cells, the cells are captured, since M-RoPE media occupies more
  cells than positions.
- `isAddonContextOverflowError` did not know about `at batch prefill
  step`, which both text guards and the MTMD batch guard emit, so on the
  message-only path those errors reached the client raw and
  `instanceof ContextOverflowError` failed.
- The KV-cache guide told readers to add `n_discarded` for long
  conversations and named it as the fix for a prefill overflow. It now
  describes the two ways a caller runs out of context.

The old wordings are kept alongside the new ones and no addon version pin
moves here, so this does not depend on #3938 shipping first.
yingying0906 added a commit that referenced this pull request Aug 24, 2026
The addon could evict tokens from the middle of the KV cache once the
context filled, during prefill or decode, and tracked `firstMsgTokens` so
the system prompt and tool definitions were never the tokens dropped. It
was opt-in through `n_discarded` and defaulted to off, so the shipped
behaviour was already the non-sliding path while the slide machinery
added state every other path had to keep correct. The generation-time
slide also invalidated a tracked `<think>` span, which wiped the sequence
and hard-failed instead of shifting the span.

- Removed `ContextSlider` (slide policy) and `ContextShifter` (slide
  budget and counter). The surviving KV primitives, the ops seam plus
  `compactKvRange`, move to `KvCacheOps` since reasoning-block compaction
  still needs them.
- Removed `firstMsgTokens` / `protectedPrefix` from both contexts and the
  `LlmContext` / `SequenceDriver` accessors that exposed them, along with
  `slideCapable` admission handling, `applySlide`, `supportsSliding` and
  `SequenceStepResult::discarded`.
- Removed the compactor's slide-invalidation state and its
  `FailedKvWiped` branch, which nothing could reach once the shifter
  went.
- Overflow is now the single path `n_discarded=0` already took. A prefill
  that does not fit throws `ContextOverflow`. A generation that fills the
  window stops with `stopReason=contextOverflow` and still returns what
  it produced. The generation check is a plain "no room for even one more
  token" test, so a caller can tell a full context from a
  prediction-limit cutoff.
- A batched slot never reached that check. `advance` marks the request
  the moment `currentPos` hits `maxTokensPerSequence` and `isFinished`
  includes that, so the slot is filtered out before the driver is asked,
  and ordinary text generation reported `sequenceLimit` for a full
  window. That limit IS the slot's share of the context, and `submit`
  rejects any request whose prompt plus a positive `n_predict` would not
  fit, so a full window is the one thing it can mean. It now says so.
- Both prefill guards name the quantity they report, so `cached tokens N
  plus prompt tokens M` is no longer formatted into the same field as the
  slice alone.
- Session metadata keeps its four-slot width so a file written by either
  build still loads, with slots 1 and 3 written as 0. `ContextSlideFailed`
  stays reserved so a new code does not reuse 26, which builds up to
  0.45.0 emit. Rolling this package back over an existing cache dir does
  drop the system prompt, because the old build reads slot 1 as its
  protected prefix and 0 makes it evict from position 0.
- Leftovers cleared: a dead `applyContextDiscard` declaration, the unused
  `IKvCacheOps::nCtx`, comments still describing sliding, `%zu` for two
  `size_t` counts formatted `%ld`, and the OpenCL error string that still
  said "sliding context".

`sliding-context.test.js` and `mrope-sliding-context.test.js` are deleted,
with all three mobile registries updated so a run does not abort on an
unregistered test. `KvCacheOpsTest` holds the 7 `compactKvRange` cases
moved out of `test_context_slider.cpp`, and the plain text overflow case
is back as a direct test in `api-behavior.test.js`.

`n_discarded` is no longer consumed, so it reaches llama's own argument
parser and fails model load as an unknown option. The SDK half is in
#3999.
@yingying0906
yingying0906 force-pushed the feat/QVAC-23752-sdk-drop-n-discarded branch from 0b19d85 to deaf0b3 Compare August 24, 2026 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-cpp-addon-tests CI: run C++ addon tests (requires verified) run-desktop-addon-tests CI: run desktop integration tests (requires verified)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant