Skip to content

feat(sampling): implement XTC sampling end to end#802

Merged
inureyes merged 3 commits into
mainfrom
feature/issue-772-xtc-sampling
Jul 16, 2026
Merged

feat(sampling): implement XTC sampling end to end#802
inureyes merged 3 commits into
mainfrom
feature/issue-772-xtc-sampling

Conversation

@inureyes

@inureyes inureyes commented Jul 16, 2026

Copy link
Copy Markdown
Member

Summary

Implement XTC (Exclude Top Choices) sampling end to end: the server request schema already accepted xtc_probability and xtc_threshold but nothing read them, so clients setting these fields silently got default sampling.

What changed

  • src/lib/mlxcel-core/src/sampling.rs: new apply_xtc_filter (removes all but the least-probable token among those exceeding xtc_threshold, no-op below two candidates, never removes allowlisted ids) and apply_xtc_step (gates the filter with a draw from the same per-request seeded global MLX RNG the fused categorical sampler consumes, keeping the whole decode step reproducible for a fixed seed). Both are wired into sample_token_optimized_core, gated on xtc_probability > 0.0 so a disabled request never advances the RNG stream or pays any array-op cost.
  • src/lib/mlxcel-core/src/generate.rs: SamplingConfig gains xtc_probability, xtc_threshold, and xtc_special_token_ids.
  • src/execution/sampling.rs: ResolvedSamplingParams / build_sampling_config thread the new fields through for both the greedy and sampled branches.
  • src/server/request_options.rs: RequestOptionOverrides / build_server_generate_options resolve per-request values against a disabled default (XTC has no server-level CLI default).
  • src/server/routes/chat.rs: build_generate_options maps SamplingParams into the overrides, and the shared validate_xtc_params helper (0.0..=0.5 for xtc_threshold, 0.0..=1.0 for xtc_probability) sits next to the existing top_logprobs range check. It now runs in chat_completions, completions, and create_response, each returning a 400 invalid_request_error before any generation work begins.
  • src/server/model_worker.rs / src/server/batch/scheduler.rs: the special-token allowlist (tokenizer newline id(s) plus the full merged end-of-sequence set) is resolved once per model load (resolve_xtc_newline_token_ids, wired into both scheduler-construction call sites) and combined with the per-request merged EOS set in BatchScheduler::enqueue_request, only when xtc_probability > 0.0.
  • src/server/routes/native_completion.rs, src/distributed/disaggregated/serving_protocol.rs, src/bin/bench_decode.rs, src/commands/generate.rs, src/commands/chat_tests.rs: updated every other SamplingConfig / ResolvedSamplingParams / RequestOptionOverrides construction site to keep the previous disabled defaults, so nothing outside the OpenAI-compatible chat/completions/responses routes changes behavior.

Review-cycle fixes

  • CRITICAL, fused-batch bypass: config_supports_fused_batch only excluded rows that need token history or carry a token bias, so an XTC-only request (no penalties, no bias) was admitted to the batched fused sampler, which calls fused_sample directly and never runs apply_xtc_step. Both the batched decode dispatch and the async lookahead priming go through this gate via row_supports_fused_batch, so XTC applied only to the first token (finish_prefill uses the per-row sampler) and was silently dropped for every token after it, failing issue feat(sampling): implement XTC sampling (request params are accepted but ignored) #772's acceptance criterion that xtc_probability=1.0 observably changes the distribution. XTC is now treated as a per-row logit edit like a non-empty token bias: xtc_probability > 0.0 disqualifies the fused fast path, so every decode token runs through the per-row sampler.
  • MEDIUM, route validation coverage: validate_xtc_params was wired only into /v1/chat/completions. /v1/completions and /v1/responses both reach XTC through the shared build_generate_options and both flatten SamplingParams, so an out-of-range xtc_probability / xtc_threshold bypassed the documented 400 and still mutated sampling on those two routes. The helper is now pub(crate) and called from the completions and create_response handlers too.

Test plan

  • cargo build --release --features cuda --workspace (zero errors)
  • cargo test --release --features cuda -p mlxcel-core sampling::: 8 XTC tests: filter keeps the least-probable token, allowlist survives removal, no-op below two/zero candidates, gate at 0.0 never fires, gate at 1.0 always fires, mid-probability gate reproducible for a fixed seed, and the fused-batch gate disqualifies xtc_probability > 0.0
  • cargo test --release --features cuda --lib execution::sampling::: 2 passed
  • cargo test --release --features cuda --lib request_options::: 12 passed
  • cargo test --release --features cuda --lib server::routes::chat::: 27 passed (5 validate_xtc_params tests)
  • cargo test --release --features cuda --lib server::routes::completions::: 3 new tests: out-of-range xtc_probability, out-of-range xtc_threshold, in-range accepted
  • cargo test --release --features cuda --lib server::routes::responses::: 3 new tests: out-of-range xtc_probability, out-of-range xtc_threshold, in-range accepted
  • cargo test --release --features cuda --lib server::batch::scheduler::tests::: 69 passed
  • cargo test --release --features cuda --lib model_worker: 28 passed (2 resolve_xtc_newline_token_ids tests)
  • cargo test --release --features cuda --lib serving_protocol: 10 passed
  • cargo test --release --features cuda --bin mlxcel chat_options_new_sets_conventional_defaults: 1 passed
  • cargo test --release --features cuda --all-targets (server crate, full run): exit 0
  • cargo fmt --all -- --check: clean

Closes #772

The server request schema already accepted xtc_probability and xtc_threshold (src/server/types/request.rs) but nothing read them, so clients setting these fields silently got default sampling.

Core filter (src/lib/mlxcel-core/src/sampling.rs): apply_xtc_filter removes all but the least-probable token among those exceeding xtc_threshold (no-op below two candidates), always preserving ids in an allowlist. apply_xtc_step gates the filter with a draw from the same per-request seeded global MLX RNG the fused categorical sampler consumes (mlx::core::random's default key is a thread-local sequence split synchronously at graph-construction time, so drawing here before the categorical draw keeps the whole step reproducible for a fixed seed). Both are wired into sample_token_optimized_core alongside the existing penalty steps, gated on xtc_probability > 0.0 so a disabled request never advances the RNG stream or pays any array-op cost.

Plumbing (mirrors the DRY parameter path from request to sampler): SamplingConfig gained xtc_probability, xtc_threshold, and xtc_special_token_ids (src/lib/mlxcel-core/src/generate.rs); ResolvedSamplingParams and build_sampling_config thread the first two through for both the greedy and sampled branches (src/execution/sampling.rs); RequestOptionOverrides and build_server_generate_options resolve per-request values against a disabled default, since XTC has no server-level CLI default (src/server/request_options.rs); chat.rs's build_generate_options maps SamplingParams into the overrides, which the /v1/completions and /v1/responses routes inherit for free since they already call it.

The special-token allowlist (tokenizer newline id(s) plus the full merged end-of-sequence set) is resolved once per model load (resolve_xtc_newline_token_ids in src/server/model_worker.rs, wired into both scheduler-construction call sites) and combined with the per-request merged EOS set in BatchScheduler::enqueue_request, only when xtc_probability > 0.0.

Request validation: chat_completions rejects xtc_threshold outside 0.0..=0.5 and xtc_probability outside 0.0..=1.0 with a 400 invalid_request_error, via a small pure validate_xtc_params helper alongside the existing top_logprobs range check.

Tests: mlxcel-core sampling:: covers the filter (keeps least-probable, allowlist survives, no-op below two/zero candidates) and the step gate (0.0 never fires, 1.0 always fires, a mid value is reproducible for a fixed seed). Server-side: request_options:: and execution::sampling:: cover default/override resolution; chat:: covers the validation helper's boundaries; model_worker_tests:: covers newline-id resolution. All pre-existing tests in the touched modules continue to pass unchanged, and every non-XTC construction site was updated to keep the previous defaults, so requests that never set these fields behave byte-identically to before.

Closes #772
@inureyes inureyes added type:enhancement New features, capabilities, or significant additions priority:low Low priority status:review Under review labels Jul 16, 2026
XTC was silently skipped for the entire decode phase. config_supports_fused_batch only excluded rows that need token history or carry a token bias, so an XTC-only request (no penalties, no bias) was admitted to the batched fused sampler, which calls fused_sample directly and never runs apply_xtc_step. Both the batched decode dispatch and the async lookahead priming go through this gate via row_supports_fused_batch, so XTC applied only to the first token (finish_prefill uses the per-row sampler) and was dropped for every token after it, failing issue #772's acceptance criterion that xtc_probability=1.0 observably changes the distribution. XTC (xtc_probability > 0.0) is now treated as a per-row logit edit like a non-empty token bias, so it disqualifies the fused fast path and runs through the per-row sampler for every decode token.

XTC range validation (the documented 400) was wired only into /v1/chat/completions. /v1/completions and /v1/responses both reach XTC through the shared build_generate_options and both flatten SamplingParams, so out-of-range xtc_probability / xtc_threshold bypassed the 400 and still mutated sampling. validate_xtc_params is now pub(crate) and called in the completions and create_response handlers, reading the same fields those handlers already forward into the sampling config.

Adds a fused-batch gate unit test and out-of-range XTC rejection tests for both /v1/completions and /v1/responses. Refs #772.
@inureyes

Copy link
Copy Markdown
Member Author

Implementation Review Summary

Intent

Implement XTC (Exclude Top Choices) sampling end to end for issue #772: a per-step probability gate that, among tokens above xtc_threshold, removes all but the least probable to promote diversity, with a newline + merged-EOS allowlist and request-layer range validation.

Findings Addressed

  • CRITICAL XTC was silently bypassed for the entire decode phase. config_supports_fused_batch did not treat XTC as a per-row logit edit, so an XTC-only request (no penalties, no token bias) was admitted to the batched fused sampler (batched_fused_sample and the async lookahead priming), which calls fused_sample directly and never runs apply_xtc_step. XTC applied only to the first token (finish_prefill uses the per-row sampler) and was dropped afterward, failing the acceptance criterion. Fixed by requiring xtc_probability <= 0.0 in config_supports_fused_batch, which routes XTC rows through the per-row sampler for every decode token (covers both the batched decode dispatch and lookahead priming via row_supports_fused_batch).
  • MEDIUM XTC range validation (400) was wired only into /v1/chat/completions. /v1/completions and /v1/responses both reach XTC through the shared build_generate_options and both flatten SamplingParams, so out-of-range xtc_probability / xtc_threshold bypassed the documented 400 and still mutated sampling. Fixed by making validate_xtc_params pub(crate) and calling it in the completions and create_response handlers.

Remaining Items

  • Informational (no change made) XTC computes softmax on pre-temperature, pre-truncation logits (it sits before the fused temperature/top-k/top-p/min-p sampler), so the threshold is temperature-independent. This differs from text-generation-webui's default warper ordering but matches issue feat(sampling): implement XTC sampling (request params are accepted but ignored) #772's "thread it like DRY" plan and is defensible.
  • Informational (out of scope) The disaggregated decode node keeps XTC disabled (serving_protocol.rs), documented as a follow-up that parallels the existing loop_detection handoff limitation. Outside this PR's single-node server scope.

Verification

  • All stated requirements implemented (filter semantics, plumbing, allowlist, gate, validation)
  • No placeholder/mock code remaining
  • Integrated into project code flow (XTC now applies in batched decode + lookahead; validation on all three OpenAI endpoints)
  • Project conventions followed (reuses validate_xtc_params, merged_eos_token_ids, the fused-batch eligibility gate; disabled-default plumbing at non-XTC sites)
  • Existing modules reused where applicable
  • No unintended structural changes
  • Tests pass (mlxcel-core: 8 XTC tests incl. the new config_supports_fused_batch_false_for_xtc; server --lib xtc: 13 incl. 3 new /v1/completions + 3 new /v1/responses rejection tests)

Fix commit: 7d09dd7

`docs/python-client.md`'s "Sampling parameters" section lists the server-specific knobs that are not part of the OpenAI schema (top_k, min_p, repetition_penalty, DRY settings, loop-detection fields) but never mentioned xtc_probability / xtc_threshold, which PR #802 wires end to end. Add both fields to that list along with their valid ranges (xtc_probability 0.0-1.0, xtc_threshold 0.0-0.5, both defaulting to disabled) and a one-line note that out-of-range values return a 400 on /v1/chat/completions, /v1/completions, and /v1/responses.

No other doc under docs/ enumerates per-request sampling fields (responses-api.md only tracks the official OpenAI subset), and there is no docs/ko twin yet, so this is the only place XTC's request-facing surface needed a mention. Test coverage was already reviewed as sufficient (21 XTC tests: 8 in mlxcel-core, 13 in the server crate) and no source changed here.

Validation:
- cargo fmt --all -- --check: clean

Refs #772
@inureyes

Copy link
Copy Markdown
Member Author

PR Finalization Complete

Test coverage

Reviewed the existing 21 XTC tests (8 in mlxcel-core's sampling:: module, 13 in the server crate: 5 validate_xtc_params in chat.rs, 3 each in completions.rs and responses.rs, 2 resolve_xtc_newline_token_ids in model_worker.rs) and reran them directly (cargo test --release --features cuda -p mlxcel-core -- xtc and cargo test --release --features cuda --lib -- xtc), both exit 0 with the counts above. The only unexercised path is the allowlist-merge logic inline in BatchScheduler::enqueue_request (newline ids union merged EOS ids), but it composes two already-unit-tested pure functions (resolve_xtc_newline_token_ids, merged_eos_token_ids) behind a simple xtc_probability > 0.0 gate, so no new test was added to avoid padding.

Documentation

docs/python-client.md is the only doc under docs/ that enumerates server-specific (non-OpenAI) sampling request fields; added xtc_probability / xtc_threshold to that list with their valid ranges (0.0-1.0 and 0.0-0.5, both defaulting to disabled) and a note that out-of-range values 400 on /v1/chat/completions, /v1/completions, and /v1/responses. There is no docs/ko twin yet (repo-wide, not specific to this PR), so no translation was needed. CHANGELOG.md was left untouched per scope.

Updated the PR body: it was stale after the review-cycle fix commit, still describing the pre-fix state (validation only in /v1/chat/completions, no mention of the fused-batch bypass) and outdated test counts. Added a "Review-cycle fixes" section covering the CRITICAL fused-batch bypass and the MEDIUM route-validation gap, and corrected the test plan to the verified counts. Closes #772 preserved verbatim.

Lint/format

cargo fmt --all -- --check clean before commit. No source changed, so clippy was not rerun (already covered by the review-cycle build).

Pushed commit 30a0c96 (docs: document XTC sampling parameters in the Python client guide) on top of 7d09dd7. Ready for merge.

@inureyes inureyes added status:done Completed and removed status:review Under review labels Jul 16, 2026
@inureyes
inureyes merged commit 25f8a75 into main Jul 16, 2026
5 checks passed
@inureyes
inureyes deleted the feature/issue-772-xtc-sampling branch July 16, 2026 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:low Low priority status:done Completed type:enhancement New features, capabilities, or significant additions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(sampling): implement XTC sampling (request params are accepted but ignored)

1 participant