Skip to content

fix(generate): sample each penalized step against the token just read - #2117

Merged
inureyes merged 3 commits into
mainfrom
fix/issue-2090-penalty-history-lag
Oct 5, 2026
Merged

inureyes merged 3 commits into
mainfrom
fix/issue-2090-penalty-history-lag

Conversation

@inureyes

@inureyes inureyes commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Summary

Every pipelined decode loop in CxxGenerator sampled step t+1 before pushing token t into token_history, so with a history-reading sampler (repetition, frequency or presence penalty, DRY) each token was sampled against a history missing the one just emitted, and the first decode token saw the prompt alone (#2090). mlx-lm's generate_step and llama.cpp's common_sampler_accept both include it.

What changed

  • sample_next_step (in generate.rs) does the ordering once for all four loops (generate_streaming, generate_streaming_with_embeddings, generate_with_stats_and_embeddings, generate_with_stats). On the history path it submits the step t+1 forward with async_eval, reads token t, pushes it, and only then builds the sample, returning the token so the loop neither reads nor pushes it twice. The history-free path keeps sampling from the lazy graph in the old order.
  • Server batch scheduler: unaffected. lookahead_params admits only rows that pass config_supports_fused_batch_except_bias, which rejects needs_token_history, so penalty rows never pipeline; the per-row paths read and push each token in the tick that sampled it (decode_tick.rs), and prefill pushes the first token before the first decode tick (prefill.rs).
  • Tests: test_support::induction now holds InductionModel, lcg_tokens and sequential_reference for both test files. generate_history_tests runs all four entry points against the sequential reference under a windowed repetition penalty, full-history counts (the incremental SamplerState path), frequency/presence, DRY, and no penalty; the four penalty cases fail on the old ordering. Prompt lookup's penalty test checks CxxGenerator against the reference again.

mlxcel generate, run and chat output under penalties changes, intentionally.

Validation

  • generate::history_tests and speculative::prompt_lookup: 44 passed; reverting the ordering fails 4 of the 5 history tests and prompt lookup's penalty test.
  • Real checkpoints on GB10: before/after release binaries built from one tree (only generate.rs swapped), greedy, median of 3, interleaved (load median 2.9):
Case no-penalty after/before no-penalty output --repetition-penalty 1.3 --repeat-last-n 3 after/before penalty output
Qwen3-1.7B write 1.004x identical 0.998x identical
Qwen3-1.7B story (800) 0.990x see below 1.005x differs
Qwen3-8B write 1.007x identical 1.010x identical
Qwen3-8B summary 1.003x identical 1.000x identical
Llama-3.1-8B story (800) 1.005x identical 0.997x differs

Throughput is unchanged on both paths. Penalty output changes only where a stale window would have chosen differently (the Llama story's before-penalty reply has a doubled word the after reply does not). The Qwen3-1.7B story differs without a penalty because plain greedy decoding of that reply is not reproducible run to run on this host: three runs of the unchanged binary diverge from each other at characters 1300 to 1351, the same range as the before/after divergence (1432). That is pre-existing and outside this PR.

  • Reviews: pr-reviewer and pr-security-checker found no CRITICAL, HIGH or MEDIUM issue in the change; their LOW notes on diagnostics are now documented at sample_next_step. A pre-existing gap they noted (mirostat and adaptive-p state is not carried by CxxGenerator) is unreachable today: generate and chat fix both off, and the server never uses CxxGenerator.

Closes #2090

Every pipelined decode loop in `CxxGenerator` built step t+1's sample before it pushed token t into `token_history`, so with a history-reading sampler (repetition, frequency or presence penalty, DRY) each token was sampled against a history missing the one just emitted, and the first decode token saw the prompt alone. mlx-lm's `generate_step` and llama.cpp's `common_sampler_accept` both include it.

A shared `sample_next_step` now does the ordering for all four loops (`generate_streaming`, `generate_streaming_with_embeddings`, `generate_with_stats_and_embeddings`, `generate_with_stats`): on the history path it submits the step t+1 forward, reads token t, records it, and only then samples, returning the token so the loop neither reads nor records it twice. The history-free path keeps sampling from the lazy graph in the old order, so its output and overlap are unchanged.

The server batch scheduler is unaffected: its lookahead pipeline admits only rows that pass `config_supports_fused_batch_except_bias`, which rejects `needs_token_history`, and the per-row paths read and push each token in the tick that sampled it.

Tests: the induction-model fixtures move to `test_support::induction`; new `generate_history_tests` run all four entry points against a sequential reference under a windowed repetition penalty, full-history counts (the incremental `SamplerState`), frequency/presence, DRY, and no penalty. The four penalty cases fail on the old ordering. Prompt lookup's penalty test checks `CxxGenerator` against the reference again.

CLI output under penalties changes, intentionally.

Closes #2090
@inureyes inureyes added type:bug Bug fixes, error corrections, or issue resolutions priority:medium Medium priority status:review Under review area:inference Generation, sampling, decoding (incl. speculative, DRY) labels Oct 5, 2026
With a history-reading sampler, sample_next_step submits the step t+1 forward before the host read, so the decode-graph hooks (MLXCEL_EXPORT_DECODE_DOT, MLXCEL_TRACE_ASTYPE, MLXCEL_CAPTURE_DECODE) cover only the sampler, the pipeline profiles count the wait for the token as sample time, and MLXCEL_FORCE_SYNC still submits the forward asynchronously before its synchronous eval. Document it at the helper and the astype hook, and fix the test file name in test_support::induction.

Refs #2090
@inureyes

inureyes commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Merging with WebUI installed artifact red, by maintainer decision.

  • The job failed five times on 19222848, every time at its pre-check (GPU compute processes are already present before Activity gate), including one rerun on an idle host (load 1.3). The check prints a count only; nvidia-smi on the host showed no compute process before or between the reruns, and the steps before the gate produced the same output as main's run of 680f64ee, which got past the pre-check and failed later on its activity-performance stage.
  • This PR cannot change what that job runs: it touches CxxGenerator's decode loops (generate, run, chat), and the server path the job exercises goes through BatchScheduler and never constructs a CxxGenerator.
  • The other 15 checks pass. Locally on GB10: 80 tests in generate::history_tests, speculative::prompt_lookup and generate::tests, clippy -D warnings, and the before/after release-binary comparison in the PR body.

The pre-check would be easier to diagnose if it printed the PIDs and process names it found.

@inureyes inureyes added status:done Completed and removed status:review Under review labels Oct 5, 2026
@inureyes
inureyes merged commit 8f20a58 into main Oct 5, 2026
121 of 126 checks passed
@inureyes
inureyes deleted the fix/issue-2090-penalty-history-lag branch October 5, 2026 22:52
inureyes added a commit that referenced this pull request Oct 5, 2026
…2119)

## Summary

The `WebUI installed artifact` job's Activity performance gate had two environmental faults on the GB10 runner (#1949). It leaked its server when the verifier was killed or its cleanup gave up, and the leaked server then failed every later run at the GPU precondition, which printed only a count (on 2026-10-05, five consecutive runs of PR #2117 after `main` 680f64e failed in scoring). It also scored while other work ran on the host and reported degradations inside its own noise. This PR closes both in the job itself.

## Changes

- **`scripts/webui/activity_gate_host.py precheck`** replaces the bare `nvidia-smi` count as the first command of the gate body (after the CI flock and `gpu-lock`):
  - A process carries the job's identity when its executable is in `$RUNNER_TEMP/mlxcel-webui-installed/`, or its working directory or `HOME` is a verifier run directory (`$RUNNER_TEMP/mlxcel-activity-performance-*`). The verifier gives the server, node and the browser `HOME=<run dir>/home`; Playwright launches the browser `detached`, in a process group of its own, so `HOME` is what reaches it.
  - Stops a process of that identity an earlier run leaked: orphaned (PPID 1), or with its directory or executable reading `(deleted)` or its run directory gone. A live process of that identity, or a leak owned by another uid, fails closed instead.
  - Fails closed if any GPU compute process remains, before the quiet wait (so a conflict is reported at once) and again after it, naming each with pid, process name, memory, parent and uid. Executable and directory paths are printed only for the job's own processes, since logs and artifacts are public.
  - Waits up to 600 s until the mean runnable-task count from `/proc/loadavg` over 10 one-second samples is at most a quarter of the CPUs (5 on GB10), and fails closed with a host snapshot otherwise. Load averages and `ps %cpu` lag a fresh compile, so neither is used, and the window must fill before the gate can refuse.
  - Writes the observed host state (snapshots at start and end with load averages and compiler pids, quiet-wait samples, anything reaped, any refusal) to `webui-activity-host-state.json`, uploaded with the diagnostics.
- **`activity_gate_host.py reap`**, a new `if: always()` step after the gate: stops the process groups the verifier's pid file names and anything else of the job's identity, prints what it found, and fails the job if anything survives or the pid file is unreadable (the identity sweep still runs). A missing pid file is a clean no-op.
- **Signalling safety:** processes with a controlling terminal (a developer's shell on the runner) are never touched; a process matched only by directory or `HOME` is signalled alone, never with its group; signals go through a pidfd checked against the recorded start time, so a reused pid is never signalled; pid-file records carry the boot id.
- **`verify_activity_performance.py`**:
  - `--pid-file` records every owned process (Xvfb, openbox, the server, the node activity script) outside the run directory as it starts; Xvfb and openbox now run in the run directory.
  - `--host-state` embeds the precheck's state into the full evidence as `host_state.gate_start`, plus a `host_state.after_activity` load sample taken before the output is validated, so a noisy verdict keeps it. The summary does not carry it and `summarize_activity_evidence.py` is unchanged.
  - `terminate_owned` no longer raises out of its SIGKILL wait or on a `PermissionError`, and it stops group members a quickly exiting leader left behind instead of only recording `process_group_empty: false`.
- Step timeout 65 to 75 minutes for the quiet wait, job timeout 90 to 120 for two cold builds plus that step; docs in `docs/webui-integration-matrix.md`; the new tests join `make verify-webui-helper-tests` and the job's helper test step.

The server spawns no child processes (model workers are in-process threads), so its process group is the whole server. The verdict rule and thresholds are untouched (#1925).

## Changes during review

The security review found that a directory-only match could SIGKILL a developer's shell and its whole group, that node and the browser had no reap handle, and that paths of unrelated GPU processes would reach public logs. All three are fixed above, together with pidfd signalling, tolerant pid-file parsing, the GPU check before the quiet wait, and the after-activity sample on the failure path from the implementation review.

## Verification

- `make verify-webui-helper-tests`: all suites pass (18 host-helper tests, 21 verifier tests, 10 summary tests). The two `terminate_owned` tests fail on the previous code: one raises `subprocess.TimeoutExpired` from the SIGKILL wait, the other ends with the group still populated. `actionlint` 1.7.7 is clean.
- **Runner, `lablup-dgxspark21` (machine spark-101):**
  - Run [37387059748](https://github.com/lablup/mlxcel/actions/runs/37387059748) (first head): the precheck found the real leaked server from the earlier failed run, pid 1453937 with executable and cwd both `(deleted)` and PPID 1, stopped it with SIGTERM, and the gate then passed. This is the leak that failed PR #2117's five runs.
  - Run [37388347876](https://github.com/lablup/mlxcel/actions/runs/37388347876) (final head 634444a): green. Host quiet at mean 1.4 runnable tasks (threshold 5), no GPU process, `server_shutdown` `exit_code 0`, `forced false`, `process_group_empty true`, full evidence carries `host_state.gate_start` and `after_activity`, and reap found nothing to do.
  - Run [37388422039](https://github.com/lablup/mlxcel/actions/runs/37388422039), job 112027483381 (throwaway probe PR #2120 on top of the final head, closed): under one busy loop per CPU the precheck refused with `host did not become quiet within 60s: mean runnable tasks 23.8 over the last 10 samples, threshold 5`; with the load removed it passed at 1.1. The verifier was then SIGKILLed mid-activity while the server held 4604 MiB, and the always() reap stopped Xvfb, openbox, the server, node and the browser (the last by `HOME`); the following check found `nvidia-smi --query-compute-apps` and `pgrep -af mlxcel-server-webui` both empty.
- **GB10 simulation on spark-102** (real `mlxcel-server` serving qwen3-0.6b-4bit, under `gpu-lock`): a leaked server (PPID 1, deleted run directory, 4116 MiB) was named and stopped; a foreign GPU process was refused at once, named without its path, and left running; a busy host (another session's compile) was refused after the wait; a server left by a SIGKILLed verifier was reaped by pid file.

Closes #1949
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:inference Generation, sampling, decoding (incl. speculative, DRY) priority:medium Medium priority status:done Completed type:bug Bug fixes, error corrections, or issue resolutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(generate): penalties in CxxGenerator see a history one token stale

1 participant