|
| 1 | +# `SAMPLE-LOGPROBS` — `logprobs=-1` widens at admission |
| 2 | + |
| 3 | +*(Live spec, 2026-08-09. Base `origin/main` `58f43f66`. Pin vLLM 0.26.0.dev0 |
| 4 | +`555967922`. Issue [#231](https://github.com/mudler/vllm.cpp/issues/231). Row |
| 5 | +`SAMPLE-LOGPROBS` (`.agents/engine-matrix.md:131`, `DONE`) — a bugfix to a |
| 6 | +closed row, not a lifecycle move.)* |
| 7 | + |
| 8 | +## Scope |
| 9 | + |
| 10 | +`logprobs=-1` ("give me every vocab entry") crashes the engine. Mirror upstream's |
| 11 | +handling — widen the sentinel to `vocab_size` at admission — so one gathered |
| 12 | +shape reaches every consumer and the sampler's raw-vocab branch becomes as |
| 13 | +unreachable here as it is upstream. |
| 14 | + |
| 15 | +In scope: `InputBatch::add_request`, `InputBatch::max_num_logprobs`, the comment |
| 16 | +on the sampler branch that stays, and the tests. Out of scope: `logprobs_mode` |
| 17 | +variants and `logprob_token_ids` generative scoring (`SAMPLE-LOGPROB-TOKEN-IDS`, |
| 18 | +`INVENTORIED`); the OpenAI `logprobs` request field, whose valid range is 0..5 |
| 19 | +and never carries `-1` — this is the library/`SamplingParams` surface. |
| 20 | + |
| 21 | +## Upstream chain |
| 22 | + |
| 23 | +- `vllm/v1/worker/gpu_input_batch.py:434-440` — the widening being ported: |
| 24 | + `self.num_logprobs[req_id] = self.vocab_size if sampling_params.logprobs == -1 |
| 25 | + else sampling_params.logprobs`. |
| 26 | +- `vllm/v1/sample/sampler.py:120-131` — the three-way branch, including the |
| 27 | + `num_logprobs == -1` raw-vocab arm. Reachable only from a hand-built |
| 28 | + `SamplingMetadata`, because `max_num_logprobs` is fed from the widened map. |
| 29 | +- `vllm/sampling_params.py:588-592` — `-1` is a legal value, validated. |
| 30 | +- `vllm/v1/engine/logprobs.py:69-119` — the consumer, which reads the gathered |
| 31 | + three-array shape unconditionally. |
| 32 | + |
| 33 | +## Our baseline |
| 34 | + |
| 35 | +The port is faithful at the sampler (`src/vllm/v1/sample/sampler.cpp:343-352` |
| 36 | +matches `sampler.py:122-125` arm for arm). The divergence is one layer up: |
| 37 | +`src/vllm/v1/worker/gpu/input_batch.cpp:292-297` deliberately PRESERVED the |
| 38 | +sentinel, and `max_num_logprobs()` at `:481-497` propagated it, both recorded as |
| 39 | +an intentional deviation in `input_batch.h`. That routes live requests into the |
| 40 | +branch upstream cannot reach. |
| 41 | + |
| 42 | +`src/vllm/v1/engine/logprobs.cpp:51-73` then indexes `logprob_token_ids` and |
| 43 | +`selected_token_ranks`, which that shape leaves empty. Its guard is `width <= 0`; |
| 44 | +the raw-vocab shape sets `num_tokens_per_position = vocab`, so the guard passes |
| 45 | +and the reads run off the end of two empty vectors. |
| 46 | + |
| 47 | +## Port map |
| 48 | + |
| 49 | +| Upstream (`555967922`) | Local anchor | |
| 50 | +|---|---| |
| 51 | +| `gpu_input_batch.py:434-440` (widen `-1` → `vocab_size`) | `InputBatch::add_request`, `src/vllm/v1/worker/gpu/input_batch.cpp` | |
| 52 | +| `gpu_input_batch.py:1150-1151` (`max(num_logprobs.values())`) | `InputBatch::max_num_logprobs`, same file — plain max once every value is concrete | |
| 53 | +| `sampler.py:122-125` (the raw-vocab arm, unreachable on the V1 path) | `src/vllm/v1/sample/sampler.cpp` — kept, and its unreachability + differing shape now stated where a future reader will meet it | |
| 54 | + |
| 55 | +## Design |
| 56 | + |
| 57 | +One line at admission. `num_logprobs[req_id] = *sp.logprobs == -1 ? vocab_size : |
| 58 | +*sp.logprobs`, exactly as upstream. `max_num_logprobs()` loses its sentinel |
| 59 | +special case and becomes the plain max upstream's `max(...)` already was: a |
| 60 | +request asking for "all" now carries the largest possible count and wins that max |
| 61 | +on its own. |
| 62 | + |
| 63 | +Nothing downstream changes. `GatherLogprobs` with `k == vocab` produces the |
| 64 | +ordinary `[n, vocab+1]` shape; `AppendLogprobsForNextPosition` already handles |
| 65 | +`num_logprobs == -1` on the *engine* side by deriving `k` from the row width |
| 66 | +(`logprobs.h:82`), so the `LogprobsProcessor` reads it correctly without change. |
| 67 | + |
| 68 | +**Why not teach the consumer the second shape.** It is the other available fix |
| 69 | +and it is worse: it keeps a deviation whose only effect is to make our engine |
| 70 | +carry two logprob shapes where upstream carries one, and every future consumer |
| 71 | +would have to know that. Removing the deviation deletes the class of bug. |
| 72 | + |
| 73 | +## Tests to port |
| 74 | + |
| 75 | +Upstream has no test for this (the value cannot reach the branch there), so these |
| 76 | +are written, not ported, and recorded as such. |
| 77 | + |
| 78 | +1. `tests/vllm/v1/test_llm_engine.cpp` — a `logprobs=-1` request through the |
| 79 | + engine returns one entry per generated token, each carrying every vocab id |
| 80 | + exactly once, the sampled token at rank 1, and a row that exponentiates to |
| 81 | + 1.0. **RED: SIGSEGV** inside `UpdateSampleLogprobs`. |
| 82 | +2. Same file — a finite `logprobs=2` request still returns at most `k+1` entries, |
| 83 | + guarding the ordinary path against a regression in the same edit. |
| 84 | +3. `tests/vllm/v1/worker/test_input_batch.cpp` — `-1` is widened at admission |
| 85 | + (the map holds `vocab_size`, never the sentinel), both alongside a finite |
| 86 | + request and alone. |
| 87 | + |
| 88 | +The existing case `C7 wiring: -1 logprobs sentinel dominates max_num_logprobs` |
| 89 | +asserted the deviation, so it is REPLACED, not relaxed: its assertion |
| 90 | +(`max_num_logprobs == -1`) is exactly the behaviour that crashes, and the |
| 91 | +replacement asserts the mirrored value with the reason written beside it. |
| 92 | + |
| 93 | +## Gates |
| 94 | + |
| 95 | +CPU reference backend. |
| 96 | + |
| 97 | +```sh |
| 98 | +cmake -S . -B build-cpu -G Ninja -DCMAKE_BUILD_TYPE=Release \ |
| 99 | + -DVLLM_CPP_CUDA=OFF -DVLLM_CPP_VULKAN=OFF -DVLLM_CPP_METAL=OFF |
| 100 | +cmake --build build-cpu -j 18 |
| 101 | +./build-cpu/tests/test_llm_engine |
| 102 | +./build-cpu/tests/test_input_batch |
| 103 | +ctest --test-dir build-cpu -j 6 --output-on-failure |
| 104 | +``` |
| 105 | + |
| 106 | +A failure under `-j` is re-run serially before it is called a regression. |
| 107 | + |
| 108 | +## Dependencies |
| 109 | + |
| 110 | +None. No kernel, no vt op, no ABI, no model file, no GPU. Independent of |
| 111 | +`SAMPLE-PROMPT-LOGPROBS` (#223): that row's `-1` path was already correct, |
| 112 | +because the runner widens to `vocab_size` for prompt logprobs the way this |
| 113 | +change now does for sampled ones. |
| 114 | + |
| 115 | +## Work breakdown |
| 116 | + |
| 117 | +Single change. There is no W2. |
| 118 | + |
| 119 | +## Risks/decisions |
| 120 | + |
| 121 | +1. **Replacing an existing assertion.** Mitigated by stating in the test itself |
| 122 | + why the old one encoded the defect, and by the engine-level RED that shows |
| 123 | + what the old behaviour actually did. |
| 124 | +2. **A hand-built `SamplingMetadata` can still reach the raw-vocab branch.** True |
| 125 | + upstream too. The branch stays (mirroring), and the comment now says the shape |
| 126 | + differs so a new consumer branches instead of indexing blindly. |
| 127 | +3. **`vocab_size` columns is a large allocation.** `GatherLogprobs` at |
| 128 | + `k == vocab` does a full sort per row. That is what "all logprobs" costs, and |
| 129 | + what upstream costs; no speed claim is made or owed. |
| 130 | + |
| 131 | +## Evidence |
| 132 | + |
| 133 | +In the PR body: the RED crash, the GREEN runs, the full `ctest` summary. |
| 134 | + |
| 135 | +## Stop conditions |
| 136 | + |
| 137 | +- If the `-1` sentinel turns out to be load-bearing anywhere else, stop and |
| 138 | + re-spec rather than widening the fix. |
| 139 | +- Never make the consumer's `width <= 0` guard broader to swallow the shape — |
| 140 | + that hides the defect instead of removing it. |
| 141 | + |
| 142 | +## Outcome |
| 143 | + |
| 144 | +*(2026-08-09. Row stays `DONE`; the fix removes a recorded deviation.)* |
| 145 | + |
| 146 | +**What the bug actually was.** Not a bad port. `sampler.cpp:343-352` matches |
| 147 | +`sampler.py:122-125` arm for arm, and reading only those two files makes the |
| 148 | +crash look like the sampler's fault. The defect was a DELIBERATE choice one layer |
| 149 | +up — preserving the `-1` sentinel instead of widening it — written down in |
| 150 | +`input_batch.h` as an intentional deviation, with the reasoning "our Sampler |
| 151 | +reads it directly". That was true. What it missed is that the branch it routes |
| 152 | +into produces a DIFFERENT shape (empty ids and ranks), and upstream can only |
| 153 | +afford that branch because its own input batch can never reach it. We adopted the |
| 154 | +branch without adopting the widening that makes it dead. |
| 155 | + |
| 156 | +The first version of issue #231 said upstream has no such branch. That was wrong |
| 157 | +and is corrected in a comment on the issue rather than silently: the branch |
| 158 | +exists at the pin, it is simply unreachable there. |
| 159 | + |
| 160 | +**Measured.** RED: `SIGSEGV` inside `UpdateSampleLogprobs` for the engine case, |
| 161 | +and `-1 == 1024` for the admission cases. GREEN: `test_llm_engine` 13/13 (228 |
| 162 | +assertions), `test_input_batch` 26/26 (190), clean CPU Release build with zero |
| 163 | +warnings under `-Werror`, full `ctest` **360/360** (729 s, no flake, no serial |
| 164 | +re-run needed). |
| 165 | + |
| 166 | +**Rejected: teaching `UpdateSampleLogprobs` the raw-vocab shape.** It fixes the |
| 167 | +crash and keeps the deviation, so our engine would carry two logprob shapes where |
| 168 | +upstream carries one, and every future consumer would need to know that. The |
| 169 | +widening deletes the class of bug instead of the instance. |
| 170 | + |
| 171 | +**Kept deliberately:** the sampler's `-1` arm. Upstream keeps it, and a caller |
| 172 | +that hand-builds `SamplingMetadata` can still reach it, so the comment there now |
| 173 | +states both that it is unreachable from the input batch and that its shape |
| 174 | +differs — which is the fact whose absence caused this. |
| 175 | + |
| 176 | +**Default.** No flag. `logprobs=-1` was already a validated, legal value; it now |
| 177 | +returns what it says. |
0 commit comments