Skip to content

fix(engine): generate waits on own request_id + chat enable_thinking - #163

Closed
bakon11 wants to merge 1 commit into
mudler:mainfrom
bakon11:fix/serve-generate-own-request
Closed

fix(engine): generate waits on own request_id + chat enable_thinking#163
bakon11 wants to merge 1 commit into
mudler:mainfrom
bakon11:fix/serve-generate-own-request

Conversation

@bakon11

@bakon11 bakon11 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Generic OpenAI-server / engine fixes found while serving Gemma-4 for Hermes. Not Gemma-specific — helps any multi-request serve path.

1. LLMEngine::generate hang under concurrent load

generate() looped on has_unfinished_requests() globally. A second unfinished request (e.g. long streaming chat) blocked every blocking generate() forever after prefill.

Fix: wait only until this request_id finishes (with idle-step safety abort).

2. Chat template enable_thinking

HF/vLLM parity: pass enable_thinking into minja (default false). Server flags:

  • --enable-thinking / --no-enable-thinking

Gemma-4 jinja emits an empty thought block when thinking is off (correct HF behavior).

3. chat_template.jinja sidecar load

(from earlier small commit on this branch) Load sibling chat_template.jinja when tokenizer_config.json has no inline template.

Test plan

  • Exclusive server: short chat + longer max_tokens completes
  • Concurrent-style hang no longer pins short requests behind another unfinished id
  • CI cpu/vulkan/sanitize

Relation to #154

Split out of the Gemma4 MoE ROCm campaign so this can land without the ROCm device-leakage rework. #154 will stay focused on ops/Backend registration per review.

FOLLOWING_AGENTS_PROTOCOL
Assisted-by: Hermes:grok-4.5 [Hermes]

@bakon11
bakon11 force-pushed the fix/serve-generate-own-request branch 8 times, most recently from efaf703 to 494584f Compare August 8, 2026 21:24
@bakon11

bakon11 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Pushed head with VT_CHAT_ENABLE_THINKING + VT_SERVER_VERBOSE documented in docs/ENVIRONMENT.md. Local gates green: doc-checkpoint, public-doc-tables, readme-structure, pr-size, env-doc, device-leakage. Please re-run CI if the previous run was on a mid-force-push SHA.

FOLLOWING_AGENTS_PROTOCOL
Assisted-by: Hermes:grok-4.5 [Hermes]

@bakon11
bakon11 force-pushed the fix/serve-generate-own-request branch 2 times, most recently from 4ff9887 to a43b442 Compare August 8, 2026 21:45
@bakon11 bakon11 closed this Aug 8, 2026
@bakon11 bakon11 reopened this Aug 8, 2026
@bakon11
bakon11 force-pushed the fix/serve-generate-own-request branch 3 times, most recently from 76a1ca2 to 4807d41 Compare August 8, 2026 21:57
LLMEngine::generate waits only until this request_id finishes.
Chat enable_thinking via VT_CHAT_ENABLE_THINKING; chat_template.jinja sidecar.
Docs: STATUS, BENCHMARKS, USAGE, ENVIRONMENT.

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: Hermes:grok-4.5 [Hermes]
@bakon11
bakon11 force-pushed the fix/serve-generate-own-request branch from 4807d41 to e769cb9 Compare August 8, 2026 22:10
@bakon11

bakon11 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

CI status note

Local gates on this PR head are green:

  • check-doc-checkpoint, check-public-doc-tables, check-readme-structure
  • check-env-doc, check-pr-size, check-device-leakage, check-commit-trailers

agent-record / test_audit_live_rows.py::test_shipped_record_has_no_abandoned_active_row fails on current main itself (fresh clone of mudler/vllm.cpp@main fails with ~55 stale ACTIVE rows). That gate is not introduced by this PR; it will red every PR until main’s live-row reconciliation is fixed.

Build jobs (cpu/vulkan/sanitize) should still be the signal for this change; please don’t block merge solely on that abandoned-ACTIVE assertion while main is in the same state.

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: Hermes:grok-4.5 [Hermes]

@bakon11

bakon11 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Dependency

#163 is green on every job except agent-record, which fails on clean main (abandoned ACTIVE rows).

Fix is in #173 (fix/audit-abandon-stale-active). Once #173 lands, re-run agent-record on this PR (or rebase).

@bakon11

bakon11 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Agent-record unblock PR is now #177 (supersedes stuck #173).

@localai-bot localai-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. This is a real bug with the right fix.

LLMEngine::generate() looped on has_unfinished_requests() — a GLOBAL predicate — so a blocking offline generate could not return while any other request was still in flight. Your comment names the failure exactly: a concurrent chat/async job pins every blocking generate indefinitely. Filtering step_outputs by out.request_id != request_id and breaking on this request's own finished is the correct scope, and it matches what upstream's per-request driver semantics imply.

The idle_steps / kMaxIdleSteps guard reads as belt-and-braces rather than the mechanism, which is the right instinct for a loop that used to be able to hang — max_tokens should always trip first, and aborting the request rather than spinning is the safe failure. Worth a follow-up only if it ever actually fires.

CI: everything green except agent-record, which is a process gate. Per operator instruction I am merging and carrying that myself.

@localai-bot

Copy link
Copy Markdown
Collaborator

Closing as already landed — with thanks, because the fix in here was real and it is now on main.

The core change, LLMEngine::generate() waiting on its OWN request_id instead of the global has_unfinished_requests(), is on main verbatim today, comment included:

// Offline driver for ONE request. CRITICAL: wait only until *this* request_id
// finishes — not has_unfinished_requests() globally. Otherwise a concurrent
// chat/async job (e.g. huge Hermes SOUL) pins every blocking generate forever.

It rode in with your other merged work. That was a genuine bug — a blocking offline generate could not return while any unrelated request was in flight — and the scoping fix is the correct one.

What made me close rather than merge: this branch is now behind main, so git diff origin/main <head> is +520 / -4868, and those deletions include files that only exist on newer main (for example include/vllm/entrypoints/openai/request_logger.h and tests/vllm/models/test_qwen3_5_lm_head_dtypes.cpp). Merging it as-is would revert a substantial amount of main to get a change main already has.

If any part of this PR did not make it across — the enable_thinking chat-template handling in particular is the piece I am least sure about, since main and this branch differ only by a variable rename there — please open a fresh PR off current main with just that delta and I will take it straight away.

#154 is merged, and your portable vt::fused_ops seam there was exactly the right fix.

@localai-bot localai-bot closed this Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants