Port upstream #197 (ignore_eos) and #221 (MTP graph topology class) - #101
Conversation
ninfer-serve accepts the ignore_eos field and ignores it, so a request generates the checkpoint's own number of tokens rather than max_tokens. With max_tokens 512 and ignore_eos true, a chat completion that stops naturally after 267 tokens should run to 512, which is what SGLang returns for the same request; ninfer returned 267 with finish_reason "stop". Any cross-engine throughput comparison then measures decode rates over windows of different lengths. StopPolicy already carries include_model_defaults (include/ninfer/types.h:234), and merge_stop_policy (src/targets/qwen3_6/impl/frontend/frontend.cpp:389) appends the tokenizer's default stop tokens only when it is set. The serve layer had no way to clear it. Add a bool ignore_eos to GenerationRequest, parse it with the existing get_bool helper in parse_stop, and map it in to_request_options as options.stop.include_model_defaults = !request.ignore_eos. Caller-supplied stop strings and stop token ids are unaffected: only the checkpoint's own defaults are suppressed, which is the documented meaning of the field in vLLM, SGLang and llama.cpp. The default is false, so a request that does not send the field behaves exactly as before, and only the OpenAI Chat Completions path is touched (/v1/messages and /v1/responses are not). Verified against the same server and prompt: ignore_eos=true gives completion_tokens=512 and finish_reason length; ignore_eos=false gives 267 and stop, unchanged.
instantiate_graph_family builds one cudaGraphExec_t per topology class and installs every other profile of that class through cudaGraphExecUpdate, which cannot cross a change of node count. dflash_graph_profiles honours that twice: it breaks the frontier where the attention route flips, and derives topology_class from the same predicate. mtp_graph_profiles did neither, so at a draft window of six or more one executable served both the prompt and the chunked small-T route. Today the MTP draft cap of five hides it. With the cap raised - four constants, no other change - draft windows 6 through 15 fail at startup with cudaErrorGraphExecUpdateFailure; with this change all of them run. The shared predicate is renamed verify_uses_chunked_small_t because both planners now read it, and gains the width bound the real route has: causal_attention_resolve_route returns Prompt for every envelope past kMaximumVerifyTokens. verify_route_flip_target locates the boundary by bisecting that predicate rather than restating the route table, which is how the planner and the route could drift apart. At the shipped cap nothing moves: every width up to six resolves to SmallT, so no class changes and the added boundary sits behind a zero check. Adds tests/targets/qwen3_6_35b_a3b/test_mtp_graph_profiles.cpp, which asserts the contract over four capacities, draft windows one to sixteen and all five KV storages. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e_eos The upstream test linked CUDA::cudart, which on MSVC collides with the cudart_static that ninfer_core already brings in (LNK2005 on cudaMemGetInfo and friends). NINFER_CUDART_TARGET is how this tree picks the runtime; linking it also restores the CUDA include path the test needs for ninfer/ops/sampling.h. Upstream's ignore_eos change shipped without a test. Pin that an absent field keeps the checkpoint's stop tokens, that ignore_eos drops them while keeping caller stop strings, and that it parses on a request with no stop field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGPoSVoQSK7bMu5SdmNfBT
![codepulse-review[bot]](https://avatars.githubusercontent.com/in/3335482?s=60&v=4){kind=small}
There was a problem hiding this comment.
Jump to the
Correctness: The request translation and MTP topology logic are consistent; no correctness, security, concurrency, lifecycle, or performance defects found.
Readability: Names and comments clearly explain the route boundary.
Architecture: The new public request control is not documented in docs/serving.md; see the inline comment.
Tests: CI reports all four checks passed. Added schema and MTP contract coverage is relevant; local execution was not performed per the static-review constraint.
⚠️ Outstanding — 1 blocking item
Still unresolved on this PR. Any coding agent — CodePulse or your own AI tool — can work this list; check items off as they land.
-
src/serve/openai_chat_request.cpp:760— New chat-completions control is missing from serving docs (Important)
Reviewed by CodePulse · AI C++ review with a senior engineer's eye · codepulse.review →
| } | ||
|
|
||
| void parse_stop(const Json& body, GenerationRequest& output) { | ||
| output.ignore_eos = get_bool(body, "ignore_eos", false); |
There was a problem hiding this comment.
Fix: Add ignore_eos to docs/serving.md with its default and stop-policy semantics.
Caution: Keep src/serve/translate.cpp and tests/test_openai_schema.cpp aligned with the documented behavior.
The serving capability table documents stop strings but does not mention ignore_eos, and the repository currently has no other documentation for it. Because unknown top-level fields are otherwise ignored, users cannot discover this extension or understand that it suppresses only checkpoint defaults while preserving caller-supplied stops. The public serving contract should be updated alongside this parser change.
There was a problem hiding this comment.
Added ignore_eos to the Chat Completions list in docs/serving.md: boolean, default false, drops only the checkpoint's stop tokens (caller stop strings still apply), non-boolean rejected with 400, not honored by /v1/responses or /v1/messages. The 400 case is now pinned in tests/test_openai_schema.cpp.
Review of #101: the field was otherwise undiscoverable, since unknown top-level fields are ignored. State its default, that it drops only the checkpoint's stop tokens while caller stop strings still apply, the 400 on a non-boolean (now pinned in the schema test), and that the Responses and Messages endpoints do not honor it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGPoSVoQSK7bMu5SdmNfBT
There was a problem hiding this comment.
✅ No action needed — ship it.
Correctness: No actionable defects found. The stop-policy mapping is isolated to Chat Completions, and graph topology classes match the runtime route predicate and envelope boundaries.
Readability/Architecture: Clear and consistent with existing patterns.
Security/Performance: No issues identified.
Tests: Added coverage is appropriate; authoritative CI reports all four checks passing. Local execution was skipped per the static-review requirement.
Reviewed by CodePulse · AI C++ review with a senior engineer's eye · codepulse.review →
4 participants
Brings in two open upstream Neroued/ninfer PRs, keeping their original authorship, plus the changes our Windows build needed.
Neroued#197: honour
ignore_eoson Chat CompletionsUntil now
ninfer-serveacceptedignore_eosand ignored it. A request stopped at the checkpoint's own EOS instead of running tomax_tokens, so throughput comparisons against SGLang or vLLM measured decode runs of different lengths. It now setsstop.include_model_defaults = !ignore_eos. Stop strings and stop token ids from the caller still apply. Only/v1/chat/completionsis affected.Neroued#221: give MTP graph profiles a topology class
mtp_graph_profilesput every profile in class 0. At a draft window of 6 or more, one CUDA graph executable would then cover both the prompt and the chunked small-T attention routes, and startup fails withcudaErrorGraphExecUpdateFailure. Our MTP cap is 5, so we can't hit this today. At widths up to 5 the profiles are unchanged, because the route-flip boundary only applies from width 7. The 35B route table in our fork matches the predicate the fix mirrors.Fork-specific follow-up
CUDA::cudart, which collides on MSVC with thecudart_staticthatninfer_corealready links (LNK2005). It now links${NINFER_CUDART_TARGET}instead.ignore_eoscoverage totest_openai_schema.cpp, since upstream's PR had no test.Verification
build-ninjabuild passes on MSVC 14.44 / CUDA 12.8.ctest -R "qwen3_6_35b_a3b|openai|anthropic|serve_options" -E real: 6 passed, 1 skipped (the DFlash load-plan test needs the artifact).variant.cpp, it fails atk=6("class 0 carries both prompt and chunked_small_t"). With the fix it passes.ignore_eosrun against a live server on the 3090.🤖 Generated with Claude Code
https://claude.ai/code/session_01GGPoSVoQSK7bMu5SdmNfBT