Repository navigation
[OPSD] Request last-position logits only in HybridEngine rollout prefill - #8839
Conversation
Prefill samples one token, but the static-cache and graph-capture paths called the model without logits_to_keep, so the lm_head produced [batch, prompt_len, vocab] logits. Pass logits_to_keep=1 when the model's forward declares it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Hi, Hi, @jinyouzhi Thanks for the optimization! The changes look good to me, and I didn't find any blocking issues. One small, non-blocking suggestion: could the new test use position-dependent logits? Since every position currently predicts token 7, it would also pass if the wrong position were selected. |
Every position used to predict the same token, so the test would also pass if the wrong position were sampled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
Thank you for your remindering, I have updated the cases. |
Related to #8197.
Summary
Rollout prefill only needs the last position's logits to pick the first token. Two prefill call sites in
HybridEngineRolloutcalled the model withoutlogits_to_keep, so the lm_head produced full[batch, prompt_len, vocab]logits:_generate_graph(graph-capture path)_continuous_prefill_cache, explicit static-cache branch (models that still expose_supports_cache_class)This PR passes
logits_to_keep=1when the model'sforwarddeclares the argument, mirroring how Transformers'generate()decides. Models without it are unchanged: their**kwargsmay be forwarded to submodules, so the argument is not passed blindly. Themodule.generate()branch already does this internally and is untouched.Scope
_supports_cache_classand supportlogits_to_keep(>= 4.50). On Transformers 5.x this path goes throughmodule.generate(), which already keeps only the last logits, so behavior is unchanged there.Measurements
Prefill forward with
StaticCache. Random-weight Llama with vocab 151936, hidden 1024, 4 layers, bf16. 1× Intel Arc Pro B70.logits_to_keep=1The selected first token is identical in all cases, and it matches the argmax of a plain HF forward. Small models gain the most because the lm_head dominates; for larger models the time saving shrinks, but the full-vocab activation is still avoided.
Tests
test_continuous_prefill_requests_last_logits_only: a model following the Transformerslogits_to_keepcontract getslogits_to_keep=1at prefill, and the generated tokens are unchanged. The test fails without the change.tests/unit/runtime/rollout/: 63 passed, 1 skipped (CPU).Contribution checklist
Related work. This relates to #8197 (rollout profiling). A search for
logits_to_keepand rollout prefill found no other PR.Sibling paths checked.
HybridEngineRollouthas three prefill entry points:module.generate(): already keeps only the last logits, so it is untouched.Commands and results.
At head
fba8f9184: 63 passed, 1 skipped, both withDS_ACCELERATOR=cpuand on 1× Intel Arc Pro B70 (XPU), Level Zero driver 1.15.39122 (compute runtime 26.27.39122.11), torch 2.14.0.dev20260804+xpu, transformers 5.17.0. The measurement table above was taken on the same B70 setup.Numerics, performance, memory. The selected first token is unchanged. Time and peak-memory results are in the Measurements table above.
Compatibility matrix.
_supports_cache_classbranch on <5.x is covered only by the unit testAI assistance. This PR was developed with AI assistance (GitHub Copilot CLI). I reviewed every changed line and ran the tests above.