docs: explain CLI DRY runs without sequence breakers by inureyes · Pull Request #1118 · lablup/mlxcel

inureyes · 2026-08-13T15:18:20Z

Summary

The CLI exposes --dry-multiplier, so DRY can be switched on from the command line, but resolved_cli_sampling_params hardcodes dry_sequence_breakers: Vec::new(). With no breakers the backward match never stops at a newline or punctuation boundary, so match_len keeps growing and the penalty is stronger than the same nominal settings produce on the server. Neither the help text nor the code said so, so a user tuning --dry-multiplier with mlxcel run and then deploying those numbers to mlxcel-server got different behavior from identical values with nothing explaining why.

This is the documentation fix the issue settled on after investigation, not the feature. No --dry-sequence-breakers CLI flag is added, and there is no behavior change.

What changed

src/commands/generate.rs: comment above dry_sequence_breakers: Vec::new() recording that the CLI runs DRY without sequence breakers as a deliberate scope decision, and why it differs in kind from the four neighbouring "feature off" defaults. Those four (frequency_penalty, presence_penalty, xtc_probability, xtc_threshold) are genuinely off because the CLI has no flag that could enable them. This one is not, because --dry-multiplier can switch DRY on, at which point the empty vector is an unchangeable configuration rather than a disabled feature.
src/main.rs: one sentence added to the --dry-multiplier clap help on SamplingOptions, stating that CLI DRY matches across all boundaries and that the server's --dry-sequence-breakers has no CLI equivalent.

Notes

SamplingOptions is flattened into both GenerateArgs and RunArgs, and the chat path shares the same SamplingConfig assembly, so the one help-text sentence covers mlxcel generate, mlxcel run, and mlxcel chat. The second dry_multiplier field in src/main.rs belongs to ServeArgs, which already exposes --dry-sequence-breakers as its own flag; it is the server side of the very asymmetry being documented and is intentionally left untouched.

The user-facing sentence went into the clap help rather than a file under docs/ to keep the change inside src/.

Test plan

cargo fmt --check clean
python3 scripts/ci/check_cross_repo_refs.py passes (no bare #NNN added)
Diff reviewed as comment-and-help-text-only: 13 added lines, all // comment or /// doc-comment lines, no executable statement changed

Compilation was not run in this worktree, which has no target/ directory and would trigger a full MLX source build. The diff changes no executable line; multi-line doc comments on clap fields are already used in this same struct (for example seed), so the pattern is established.

Closes #1108

The CLI exposes `--dry-multiplier`, so DRY can be switched on from the command line, but `resolved_cli_sampling_params` hardcodes `dry_sequence_breakers: Vec::new()`. With no breakers the backward match never stops at a newline or punctuation boundary, so `match_len` keeps growing and the penalty is stronger than the same nominal settings produce on the server. Nothing in the help text or the code said so, and a user tuning `--dry-multiplier` with `mlxcel run` and then deploying those numbers to `mlxcel-server` got different behavior from identical values with no explanation. Two text-only changes, no behavior change and no new flag. The comment in `src/commands/generate.rs` records that the empty vector is a deliberate scope decision and why it differs in kind from the four neighbouring "feature off" defaults: those four are genuinely off because the CLI has no flag that could enable them, whereas this one is an unchangeable configuration of a feature the CLI can enable. The `--dry-multiplier` clap help in `src/main.rs` gains one sentence stating that CLI DRY matches across all boundaries and that the server's `--dry-sequence-breakers` has no CLI equivalent. `SamplingOptions` is flattened into both `GenerateArgs` and `RunArgs`, so the help-text sentence covers `mlxcel generate`, `mlxcel run`, and the chat path that shares the same `SamplingConfig` assembly. The second `dry_multiplier` field in `src/main.rs` belongs to `ServeArgs`, which already exposes `--dry-sequence-breakers`, and is intentionally left untouched. Validated with `cargo fmt --check` and `scripts/ci/check_cross_repo_refs.py`. Compilation was not run in this worktree; the diff adds only comment and doc-comment lines and changes no executable statement. Closes #1108

## Summary Four PRs merged on 2026-08-13. Two of them, #1120 and #1122, got their bilingual technical reports from the chain workflow. #1118, #1119 and #1121 merged without one. This PR closes that gap after the fact so the batch is uniformly documented. Six files, `.en.md` and `.ko.md` per PR, following the naming and section shape of the two reports already in `TECHNICAL_REPORTS/` from this same batch. No source, documentation, or build file is touched. ## What is here | PR | Issue | Squash commit | Report | |---|---|---|---| | #1119 | #1104 | `cf4e22cd` | `1119-paged-decode-v2-env-vars-20260814.{en,ko}.md` | | #1118 | #1108 | `33322d66` | `1118-cli-dry-sequence-breakers-20260814.{en,ko}.md` | | #1121 | #1110 | `f0bf3a2c` | `1121-utils-used-by-annotations-20260814.{en,ko}.md` | Each report was written from the merged diff (`git show <commit>`) and the linked issue body, not from the PR description alone. Each records at least one thing the issue did not contain. ## Findings recorded in the reports **#1121 (issue #1110).** The issue was wrong on a load-bearing point. It recorded `create_causal_mask` as carrying no `Used by:` annotation. It carried one, and that annotation named Llama, Mixtral, Gemma, Cohere, Phi, GLM4, StarCoder2 and OLMo among its users while grep at that commit shows `mixtral.rs`, `phi.rs`, `phi3small.rs`, `starcoder2.rs`, `llama3.rs`, `gemma.rs`, `gemma2.rs`, `cohere.rs`, `glm4.rs`, `olmoe.rs` and `qwen3_moe.rs` each calling it zero times. Those families moved to the implicit-causal fused-SDPA path (`mask: None` when `seq_len > 1`). The PR therefore replaced a misleading roster rather than adding a missing one, which is a stronger defect than the issue described: a missing annotation sends a contributor to look, a wrong one tells them not to. Measured counts at that commit: 44 non-test callers under `src/models` (the issue's 46 counted `diffusion_gemma/tests.rs` and `phi3small_tests.rs`), 55 across all of `src`. **#1119 (issue #1104).** All six variable defaults were traced to their definition sites rather than to the issue text. Two out-of-scope defects are recorded rather than fixed: `docs/turbo-kv-cache.md` around lines 298 to 312 still carries the same pre-#899 "not a server knob" framing this PR corrected in `environment-variables.md`, and in `src/execution/memory_estimate.rs` the `Err(_)` arm of `resolve_paged_slab_blocks` warns "using the derived slab size" and then returns `None`, which keeps the 32-block pool default instead, so the warning describes behavior that does not happen. **#1118 (issue #1108).** The issue was reframed away from a flag-parity feature request into a docs fix after investigation showed the CLI omits nine server sampling knobs, not one. The change is comment-and-help-text only, 13 added lines all matching `^\s*(///|//)`. The second `dry_multiplier` field in `src/main.rs` belongs to `ServeArgs`, which already exposes `--dry-sequence-breakers` sixteen lines below, so it was deliberately left alone. ## Notes `TECHNICAL_REPORTS/` is listed in `.gitignore` but is tracked through the `TECHNICAL_REPORTS/.keep-reports` marker, so the six files were staged with `git add -f`. ## Test plan - [x] `git status --porcelain` shows exactly the six intended files staged and nothing else - [x] Every claim in each report checked against the merged commit: caller counts re-run with `git grep` at `f0bf3a2c`, variable defaults re-read at their definition sites, diffstats taken from `git show --stat` - [x] No build, test, lint, or format surface involved: the diff is six new Markdown files under `TECHNICAL_REPORTS/`

…1124) ## Summary The greedy branch of `build_sampling_config` (`src/execution/sampling.rs`) forwarded four of the five DRY fields and let `dry_sequence_breakers` fall through to `SamplingConfig::greedy()`, which sets an empty vector. DRY is not gated on temperature, so a server request with `temperature: 0` and a positive `dry_multiplier` ran the penalty with no breakers at all. ## Related issues Closes #1102 ## Type of change - [x] `fix` (bug fix) - [x] `docs` (the technical report and the `CHANGELOG.md` entry that accompany it) ## Why it mattered The breakers are the DRY backward match's termination condition (`config.dry_sequence_breakers.contains(&window[p1])` in `src/lib/mlxcel-core/src/sampling.rs:846`), not a sampling knob. That test precedes the token-equality test in the loop, so removing breakers can only lengthen the match or leave it unchanged: `match_len` runs past the intended boundary and `dry_multiplier * dry_base.powi(match_len - dry_allowed_length)` comes out at or above what the request asked for, strictly above whenever the tokens beyond the boundary also match. At `temperature: 0` the argmax can then land on a different token, silently, with the request having been accepted as valid. The path is server-only and reachable from both request shapes (`src/server/types/request.rs:601` and `:978`), so `{"temperature": 0, "dry_multiplier": 0.8, "dry_sequence_breakers": [198]}` was a supported request whose breakers did nothing. The CLI cannot reach it: `resolved_cli_sampling_params` hardcodes `Vec::new()` and exposes no flag (documented in #1118). ## What changed **`src/execution/sampling.rs`**: the greedy branch now forwards `dry_sequence_breakers: params.dry_sequence_breakers` alongside the four DRY fields it already carried. The added comment records why the field belongs here rather than in `SamplingConfig::greedy()`: it is a match-termination condition, and leaving it empty inflates the penalty rather than disabling anything. This mirrors the reasoning the neighbouring XTC comment already states for the same branch. `SamplingConfig::greedy()` still supplies `top_k: 1` and `top_p: 1.0`, so greedy determinism is untouched, and a request with `dry_multiplier: 0.0` sees no behavior change at all because DRY is not applied. After the change the greedy branch forwards every field of `ResolvedSamplingParams` except `temperature`, `top_k` and `top_p`, which are exactly the three that define greedy, so the class of defect (a silent per-field omission in the branch's struct literal) is closed rather than just this instance of it. **`src/execution/sampling_tests.rs`**: `build_sampling_config_uses_greedy_defaults_when_temperature_is_zero` asserted the empty vector while asserting none of the four DRY fields the branch did forward, so it stated half the contract with no comment saying why. It now asserts all five DRY fields with a comment recording that DRY runs independently of temperature. A named regression test, `build_sampling_config_keeps_dry_sequence_breakers_at_zero_temperature`, covers the reported request directly and re-asserts `top_k` / `top_p` so a future change cannot fix the breakers by weakening greedy. **`src/server/request_options_tests.rs`**: `build_server_generate_options_applies_request_overrides` is the end-to-end shape of the bug report (`temperature: Some(0.0)`, `dry_multiplier: Some(0.9)`, `dry_sequence_breakers: Some(vec![1, 2])`) and asserted `Vec::<i32>::new()`. It failed on the fix, which is the second independent confirmation that the defect was real rather than a test-only artifact; it now asserts the requested breakers. **`CHANGELOG.md`**: an `## [Unreleased]` / `### Fixed` entry, because this changes generated tokens for existing callers at `temperature <= 0` with DRY enabled. The v0.5.0 "Changed" section documents two narrower sampler token-stream changes the same way. **`TECHNICAL_REPORTS/1124-greedy-dry-sequence-breakers-20260814.{en,ko}.md`**: the report for this PR, committed with the fix per the `TECHNICAL_REPORTS/.keep-reports` marker. ## Test plan - [x] `cargo fmt --all -- --check` clean - [x] `cargo clippy --profile test-fast --features cuda --lib --tests -- -D warnings` clean (exit 0). The template's `--workspace --all-targets` form was not run: this host builds MLX from source and that invocation does not finish inside the available window. The scoped run covers both edited test modules and the edited production module. - [x] `cargo test --profile test-fast --features cuda --lib execution::sampling` (3 passed) - [x] `cargo test --profile test-fast --features cuda --lib server::request_options` (35 passed) - [x] `cargo test --profile test-fast --features cuda --test cli_help_consistency` (17 passed, unchanged by this PR, run as a CLI-surface regression check) - [x] Verified before the fix that `build_server_generate_options_applies_request_overrides` reproduces the defect independently (`left: [1, 2]`, `right: []`) - [ ] `cargo test --workspace --profile test-fast` and `cargo deny check` not run here; the change adds no type, signature, or dependency, and both structs already carried the field, so nothing outside the two edited test modules can fail to compile because of it. - [ ] No real-checkpoint validation. The defect and the fix are both at the configuration-assembly boundary; a generation-level assertion would be seed- and checkpoint-dependent and would not pin the contract that was broken.

#1125) ## Summary `mlxcel serve` and `mlxcel-server` are two hand-maintained clap definitions of the same server sharing 112 flag spellings, and four had drifted apart. Each missing spelling is added as a `visible_alias`, the DRY breakers get one primary spelling on both binaries, and `tests/cli_help_consistency.rs` grows a named-list contract so a fifth divergence cannot land silently. ## Related issues Closes #1109 ## Type of change - [x] `fix` (bug fix) ## Why it mattered `--n-parallel` worked only on `mlxcel serve`, `--parallel` only on `mlxcel-server`, so a command line copied between the two failed with `error: unexpected argument '--parallel' found`. Both flags read the same `LLAMA_ARG_N_PARALLEL` env var, so the divergence was purely in the name, and an operator who worked around it via the env var would not discover the gap until someone else read the script. `README.md` advertises llama-server compatibility as a migration aid, and three of the four broke it on `mlxcel serve`. This is drift, not design. The repository already establishes the intended pattern twice in the same structs: `--draft-model` / `--model-draft` and `--draft-max` / `--draft` are aliased both ways, and the doc comment on the first states the goal outright ("so commands copied between the two binaries work unchanged"). The four flags below simply never got the same treatment. ## What changed **Aliases, mirroring the drafter flags.** `visible_alias = "parallel"` on `ServeArgs::n_parallel`, `visible_alias = "n-parallel"` on `ServerArgs::parallel`, `visible_alias = "predict"` on `ServeArgs::n_predict`, `visible_alias = "adapter"` on `ServerArgs::lora`. `visible_alias` renders the alternate spelling in `--help`, which is what the existing drafter flags use. No primary spelling changes here, so nothing that worked before stops working. **DRY breakers: one primary on both binaries.** This row needed a decision rather than an alias, because the two binaries disagreed on the name itself (`--dry-sequence-breakers` plural on `serve`, `--dry-sequence-breaker` singular on `mlxcel-server`). The singular becomes primary on both. It is what llama-server uses, it is what `mlxcel-server` already used, and the flags in this group carry `LLAMA_ARG_*` env vars precisely because llama-server compatibility is the point. Unlike the drafter flags, there is no competing mlx-lm spelling to honor here, so this pair can do better than the drafters' opposite-primaries compromise and share a single primary. The plural is kept as a `visible_alias` on both binaries, so the spelling `mlxcel serve` used to require still parses. **Two references to the old primary spelling are updated.** The help sentence added by #1118 on `SamplingOptions::dry_multiplier` and the scope comment at `src/commands/generate.rs:921` both named the plural; they now name the primary. Both remain accurate: the plural still works, but naming the primary is what keeps the help text pointing at the documented spelling. **The forked `--parallel` help paragraph is reconciled.** The two binaries carried the same paragraph in two wordings, each edited to match its own spelling, which is how the name drift stayed invisible. They now share the more informative `mlxcel-server` text plus a closing sentence stating the copied-command-line property, identical on both. `docs/CONTINUOUS_BATCHING.md` no longer presents the two spellings as one per binary. **`tests/cli_help_consistency.rs`, the durable half.** Three assertions, each failing on something the others cannot: - `the_two_server_binaries_accept_the_same_flag_surface` compares the whole long-name surface of the two binaries against `SERVE_ONLY_FLAGS` / `SERVER_ONLY_FLAGS`. This is the one that catches a NEW divergence: a flag added to one binary and forgotten on the other fails immediately, with no list to remember to update. The issue offered a named list as a fallback on the assumption that a whole-surface comparison would be too noisy; measuring it killed that assumption. The two binaries share 134 spellings and differ by exactly three, so the allowlist is `--estimate-memory` and `--force` on `mlxcel serve` (subcommand-shaped one-shot actions) and `--version` on `mlxcel-server` (`mlxcel` carries it at the top level). - `SHARED_SERVER_FLAG_GROUPS` lists six concepts (the four repaired here plus the two drafter pairs) and pins which alternate spellings belong to one concept, which a set comparison cannot express. It is the REGRESSION guard: dropping `--parallel` from `mlxcel-server` and `--n-parallel` from `mlxcel serve` in the same change would leave the two surfaces equal, and only this assertion would notice. It compares sorted sets, so which spelling each binary makes primary stays a free choice. - `SHARED_SERVER_FLAG_DESCRIPTIONS` requires the help prose, the environment variable, and the default value to match for the four non-drafter concepts. The drafter pairs are excluded because their descriptions name each binary's own primary and alias roles, which are opposite by design, so identical prose there would make one of the two wrong. Six helpers support this. `signature_long_name` requires the exact shape clap renders, so a prose line starting with `--` cannot anchor an entry, and it strips the `[=<VALUE>]` marker clap emits as part of the same token for an optional-value flag. `flag_entry_by_long_name` anchors on the long name alone, because clap derives the value name from the Rust field name and the two binaries therefore render different ones for the same concept. `top_level_help` cuts the help at the first flattened-subcommand heading, because `mlxcel-server` sets `flatten_help` and its `--help` carries a second flag surface under `mlxcel-server download:` in which `--models-dir` and `--help` already appear twice. `all_documented_spellings`, `flag_description`, and `flag_env_and_default` split an entry into the three parts the contract treats differently. `flag_help_entry`'s body-slicing loop is factored into a shared `entry_body` so the two entry finders cannot disagree about where an entry ends. **Two clap name-uniqueness guards.** `[profile.test-fast]` inherits `release`, so `debug-assertions` is off and clap's own duplicate-name `debug_assert` never runs in the profile this repository verifies with: a `visible_alias` colliding with an existing flag would be silently last-wins rather than a panic, and this PR adds four of them. `serve_flag_names_and_aliases_are_unique` and `server_flag_names_and_aliases_are_unique` walk the built `Command` and assert uniqueness directly, so the guard holds in every profile. **`CHANGELOG.md`**: an `### Fixed` entry under `## [Unreleased]`, because the accepted CLI surface changes on both binaries and one primary spelling changes on `mlxcel serve`. **Parse-level assertions** in `src/main_tests.rs` and the `mlx_server.rs` test module pin that each spelling resolves to the identical field value, mirroring the existing issue #464 alias tests. Both ends of the table are covered, including `--adapter` / `--lora` on `mlxcel serve`, which was already symmetric. ## The new assertion is not vacuous A green contract test proves nothing unless it fails on a real regression, so this was checked three ways: 1. Five guard tests pin the helpers against synthetic input: the four signature shapes clap renders plus the two it must reject, the optional-value `[=<VALUE>]` form, the flattened-subcommand cut (and four inputs it must NOT cut), the surface collector, and the mirrored-entry case where two binaries with opposite primaries and different value names must still compare equal. 2. `dropping_a_shared_flag_alias_would_fail_the_spelling_parity_assertion` splices the live `mlxcel-server --help`, cuts the rendered `[alias: --n-parallel]` annotation out of the `--parallel` entry, and asserts the accepted-spelling set then collapses to `["--parallel"]`. This is the same idiom the existing `removing_the_rendered_alias_annotation_leaves_the_alias_undocumented` test uses. 3. Two one-off live mutations against the real binaries, each restored and the suite re-run green. Removing `visible_alias = "n-parallel"` from `src/bin/mlx_server.rs` fails the named-list assertion with `left: ["--parallel"], right: ["--n-parallel", "--parallel"]`. Removing `--force` from `SERVE_ONLY_FLAGS` fails the whole-surface assertion with `left: {"--estimate-memory", "--force"}, right: {"--estimate-memory"}`, which is the shape a genuinely new one-sided flag would produce. 4. The whole-surface assertion carries its own floor (`serve.len() > 100 && server.len() > 100`), so it cannot pass by comparing two empty sets if the matcher ever stops resolving entries. ## Test plan - [x] `cargo fmt --all -- --check` clean - [x] `cargo clippy --profile test-fast --features cuda --lib --bins --tests -- -D warnings` clean (exit 0). `--bins` is included because this PR changes both binary crates. The template's `--workspace --all-targets` form was not run: this host builds MLX from source and that invocation does not finish inside the available window. - [x] `cargo test --profile test-fast --features cuda --bin mlxcel tests::serve_` (13 passed) - [x] `cargo test --profile test-fast --features cuda --bin mlxcel-server tests::` (13 passed) - [x] `cargo test --profile test-fast --features cuda --test cli_help_consistency` (25 passed, up from 17 on the base) - [x] Both live mutation checks described above, each restored and re-run green - [ ] `cargo test --workspace --profile test-fast` and `cargo deny check` not run here. No dependency, type, or signature changes; the blast radius is two clap structs, their test modules, and one integration test. - [ ] No real-checkpoint validation. Nothing on the inference path changes; the diff is clap attributes, help text, tests, and one docs line. ## Known gap, deliberately not closed here Short forms still diverge: `mlxcel-server` carries `-c` for `--ctx-size` and `-n` for `--predict`, and `mlxcel serve` has neither, so `mlxcel-server -m X -c 4096 -n 256` does not copy across. Both letters are free on `mlxcel serve`, so closing it is cheap, but it is a different table from the one #1109 enumerated and the contract compares long names only. The shipped help sentence is narrowed accordingly: it now says "so this flag parses on either binary" rather than claiming any copied command line parses unchanged.

inureyes added type:docs Documentation improvements or additions priority:low Low priority status:done Completed labels Aug 13, 2026

inureyes merged commit 33322d6 into main Aug 13, 2026
8 checks passed

inureyes mentioned this pull request Aug 13, 2026

docs: add technical reports for PRs #1118, #1119 and #1121 #1123

Merged

3 tasks done

inureyes mentioned this pull request Aug 13, 2026

fix(server): thread dry_sequence_breakers through the greedy branch #1124

Merged

8 of 10 tasks

inureyes mentioned this pull request Aug 13, 2026

fix(cli): make the drifted server flag spellings work on both binaries #1125

Merged

7 of 9 tasks

inureyes deleted the update/issue-1108-cli-dry-breakers branch August 19, 2026 10:49

inureyes self-assigned this Aug 27, 2026

inureyes mentioned this pull request Oct 6, 2026

epic: one batch-native engine for CLI and server decode #2166

Open

9 of 14 tasks

Navigation Menu

Repository navigation

docs: explain CLI DRY runs without sequence breakers - #1118

docs: explain CLI DRY runs without sequence breakers#1118
inureyes merged 1 commit into
mainfrom
update/issue-1108-cli-dry-breakers

inureyes commented Aug 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

inureyes commented Aug 13, 2026

Summary

What changed

Notes

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant