Skip to content

Consult ignore_eos when classifying a finish reason at the output limit - #102

Merged
lukealonso merged 1 commit into
NVIDIA:mainfrom
mark-yong:fix/ignore-eos-finish-reason
Nov 5, 2026
Merged

lukealonso merged 1 commit into
NVIDIA:mainfrom
mark-yong:fix/ignore-eos-finish-reason

Conversation

@mark-yong

@mark-yong mark-yong commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Request

ignore_eos requests finish on their output limit, but the terminal event classified the finish reason as if an end-of-sequence token had ended the generation. docs/TODO.txt carries the gap as a roadmap item: "Respect ignore_eos when classifying EOS at the output limit; see helios/src/server/runtime.rs."

ignore_eos suppresses end-of-sequence termination: the admission flag is packed into the sampling graph as EosCount = 0 (helios/src/model/packing.rs), so the request keeps generating past EOS tokens and stops at its output limit. The terminal event still decided the reason from the last sampled token alone:

// helios/src/server/runtime.rs, before
let reason = if user_stop
    || request
        .last_token
        .is_some_and(|token| self.config.eos_token_ids.contains(&token))
{
    "stop"
} else {
    "length"
};

The two neighbouring EOS checks in the same function already consult the setting (the terminating-EOS check and the text-stop finish), so this site was the inconsistent one. When an ignore_eos request's last sampled token happens to be an EOS id, the client is told the generation ended normally although it was cut at its output limit. That reaches every encoder: OpenAI finish_reason: "stop", Anthropic stop_reason: "end_turn", and Responses incomplete: false, and Termination::Eos on the terminal event. A harness that trusts those fields treats truncated output as a complete answer.

It fires when the sampling draw lands on an EOS token in the final position of a request that carried ignore_eos: intermittent by sampling, deterministic once the draw occurs.

Change

  • helios/src/server/runtime.rs: the finish-reason classification uses the request's ignore_eos setting, the same way the emitting and logprob paths already do. An ignored EOS no longer counts as a stop, so the request reports length and Termination::OutputLimit. User stops are unchanged, and so is every non-ignore_eos request. The changed termination also reaches the decoders before encoding, which changes three behaviors for an ignore_eos request that ends at its output limit on an end-of-sequence token: a completed tool call now reports the output-limit reason instead of tool_calls (Chat Completions; raw Completions rejects tool parameters) or tool_use (Messages), so a client that dispatches only on those reasons stops dispatching; an open tagged, DSML, or XTML tool envelope is now dropped silently where end-of-sequence termination raised an "incomplete tool call" error; and the recipient parser no longer closes a final complete tool frame that end-of-sequence termination would have closed, so with required or named tool choice and no earlier completed call the request now ends in an error. All three follow from end-of-sequence being non-terminating and match the documented rule that truncated envelopes are omitted (docs/api.md).
  • docs/api.md: maps each termination cause to each protocol's finish reason. An output limit reports length (Chat Completions and Completions), max_tokens (Messages), and incomplete with max_output_tokens (Responses) even when parsed calls completed the answer; otherwise completed calls report tool_calls and tool_use (raw Completions rejects tool parameters, so tool_calls is Chat Completions only); any other cause reports stop (Chat Completions and Completions), with Messages returning stop_sequence for a matched string and end_turn for every other cause, including an honored end-of-sequence token and an explicit token stop. The cause is decided at the terminal token, so an honored end-of-sequence token or a matched string stop landing on the final allowed token reports the stop reason. A request admitted with ignore_eos reports the limit reason when it exhausts its output limit even when its last generated token is end-of-sequence, and a page-capacity early finish reports the limit reason before the declared limit.
  • docs/TODO.txt: drops the roadmap item this closes.
  • Regression test ignored_eos_at_the_output_limit_finishes_for_length (helios/src/server/runtime/phase_tests.rs): a one-token prompt with ignore_eos: true and a two-token limit, EOS sampled on both generated tokens, driving Init → Prefill → Snapshot → Decode → Snapshot and asserting the terminal event is (generated tokens 2, reason "length", Termination::OutputLimit).

Runtime-only: no package format, prepared contract, graph, kernel, or recipe change, so existing packages need no re-export.

Validation

CPU, release Helios library suite on the build machine (no GPU), commit 449b8708:

  • The regression phase test fails on c65024db with only the test applied, at the terminal-event assertion (left: [(2, "stop", Eos)], right: [(2, "length", OutputLimit)]), and passes with the fix.
  • Targeted run with the fix (--lib ignored_eos): 3 passed, the new case plus the two existing server::logprobs ignore_eos cases.
  • Full library suite: 1276 passed, 9 failed, 25 ignored with the fix; 1275 passed and 10 failed on c65024db with only the test added, the new case being the only difference. The nine shared failures are environmental on both trees: four engram::reader staging allocation cases (the tests request 8 MiB RLIMIT_MEMLOCK, the host caps it lower), four missing FLARE_TEST_TEMPLATE_DIR fixture cases, and one package::cache staging case.
  • Integration targets and doc-tests: identical counts on both trees. rustfmt --check on the touched file: no new drift.

Separately, builds at 0ce6de86 served one prepared package side by side on one GPU (unsloth/gemma-4-E4B-it-NVFP4 TP1 on SM120, no MTP draft, --max-concurrency 2) with the same request to both: /v1/chat/completions, prompt "Reply with exactly the word: ok", which stops naturally after two tokens (ok + end-of-sequence). Requests carried an explicit seed rather than temperature: 0. Flare has no greedy mode to request: the decoder clamps client sampling values to the package's declared range before validation (helios/src/server/request.rs, sampling_number), so temperature: 0 is served as the declared minimum or the prepared default, and the model-layer rejection in SamplingParameters::validate is never reached over HTTP, as docs/api.md (Clamping and rejection) documents. Six requests per cell, cells interleaved between the servers, except the defect cell at thirty:

max_tokens ignore_eos unmodified build this build
48 false stop / 2 tok ×6 stop / 2 tok ×6
2 true stop / 2 tok ×30 length / 2 tok ×30
10 true stop / 10 tok ×6 length / 10 tok ×6
2 false stop / 2 tok ×6 stop / 2 tok ×6
1 true length / 1 tok ×6 length / 1 tok ×6
1 false length / 1 tok ×6 length / 1 tok ×6

Row 2 is the defect: the limit sits exactly at the natural stop, so the last sampled token is the end-of-sequence token, and the unmodified build reports stop for a request that ran to its output limit. Row 3 is the same request with more room. Rows 1, 4, 5 and 6 show the unchanged paths: honouring EOS, and truncation before the EOS. Token counts and returned text matched between the builds in every cell.

Limitations

  • The served check covers one prepared package, one GPU generation (SM120), sampling at the package's prepared defaults with an explicit seed, no MTP, and the /v1/chat/completions path. The Anthropic and Responses surfaces derive from the same Termination, but were not exercised over the wire.
  • A pinned seed makes a cell repeat, and every cell above produced a single distinct token stream on each build, but it is not a bit-identical guarantee across requests: in one pass at max_tokens=32 the unmodified build produced two different streams in six requests. The classification rule held in every request observed on both builds: an ignore_eos request must not report stop because its last token was an ignored end-of-sequence token.
  • Sampled-EOS suppression (EosCount = 0) is unchanged and was not re-validated. The runtime change is limited to termination classification, but the resulting Termination also affects decoder finalization and tool-call handling as disclosed above.
  • The reason value is a contract choice: length matches vLLM for the same request.
  • Only requests whose final sampled token is an EOS id under ignore_eos change behavior; the other termination paths are untouched, and their encoders read the same Termination values as before.

Written with AI assistance; reviewed before opening.

@mark-yong mark-yong changed the title fix(server): report the output limit when an ignored EOS ends a request Consult ignore_eos when classifying a finish reason at the output limit Oct 26, 2026
@mark-yong
mark-yong force-pushed the fix/ignore-eos-finish-reason branch from 5dba4f1 to ea7e971 Compare October 26, 2026 21:17
@original-el8 original-el8 added area:api Serving API, protocol encoding, tool parsing, and client-visible behavior. bug Something isn't working priority:P1 High attention: unresolved correctness, serving failure, or model-data deletion risk. triage:review Proposed change needs code review; this does not imply merge readiness. labels Nov 5, 2026
@original-el8

Copy link
Copy Markdown
Collaborator

@mark-yong GitHub reports that this PR has merge conflicts with main. A local git merge-tree check also confirms conflicts for PR head ea7e9716c309 against main at c65024dba32b (2026-11-05).

Conflicting files:

  • helios/src/server/runtime.rs

Please update the branch against main (rebase or merge), resolve these conflicts, and rerun the affected checks.

@mark-yong
mark-yong force-pushed the fix/ignore-eos-finish-reason branch from ea7e971 to f1cccd6 Compare November 5, 2026 18:29
@original-el8

Copy link
Copy Markdown
Collaborator

The runtime fix is correct and still needed. The branch head sits on main c65024d, so the merge is a fast-forward, and the finish-reason classification in Worker::publish has not changed on main since 99cfcd5. On CPU (--features runtime-package,server), server::runtime::phase_tests passes 86/86 on the branch head. On main with only the new test added, that test fails with left: [(2, "stop", Eos)], right: [(2, "length", OutputLimit)]. A speculative round that emits [5, 1, 5] against a limit of 4 also moves from (4, "stop", Eos) to (4, "length", OutputLimit). Four points before merging:

1. The new termination also changes parser behavior, which the description does not state. The body says "only the reporting moves". The changed termination (helios/src/server/runtime.rs:1869-1875) is also passed to decoders[index].finish(termination) (helios/src/server/http.rs:494-510) before the encoder runs. For an ignore_eos request whose last token at the output limit is EOS:

  • A completed tool call stays in the message, but finish_reason changes from tool_calls to length and Anthropic stop_reason from tool_use to max_tokens, because the output limit takes precedence (helios/src/server/protocol/encode.rs:550-556). A client that dispatches tools only on tool_calls stops dispatching them.
  • An open tool envelope in the tagged, DSML and XTML parsers is now dropped silently, with length reported, where it was an "incomplete … tool call" error (helios/src/output/tagged.rs:261-271, dsml.rs:326, xtml.rs:242-247).
  • The recipient parser drops a complete final tool frame that EOS would have closed (helios/src/output/recipient.rs:264-282): on such a frame finish(Eos) emits ToolEnd and finish(OutputLimit) does not.

All three follow from EOS being non-terminating, and they match the documented rule that truncated envelopes are omitted (docs/api.md:359-361). They are behavior changes nonetheless, the recipient-format drop above all, so the description should state them and a maintainer should accept them explicitly.

2. Commit text.

  • The f1cccd6 message ends with a rebase-history paragraph ("Rebased onto c65024d after upstream changed…"). Commit messages should state the resulting behavior and validation, not the merge history.
  • It reports suite counts for two trees and does not name the first (0ce6de8).
  • It describes the served check as "greedy sampling", while the PR body says the requests used an explicit seed at clamped prepared defaults; Flare has no greedy mode.
  • It gives the defect cell as 6/6, while the body gives 30 requests.
  • Both commits are signed Design-Signed-Off-By: Hermes Agent. docs/design-review.md asks for the reviewing agent's name and model, and the model is missing. The digest a38f is correct for c65024d.

3. The first commit fails its own test. 8d61d89 adds only the test, which fails on that tree, so a rebase-merge would put a failing commit on main. A squash merge, or folding the test into the fix commit, avoids that.

4. The docs/api.md:373-378 paragraph is imprecise.

  • It names only the Chat and Completions values. Messages reports max_tokens, tool_use, stop_sequence and end_turn; Responses reports incomplete with max_output_tokens.
  • It does not say that the output limit takes precedence over tool_calls.
  • "length at the output limit" has two exceptions. A string stop matched at the final flush reports stop. A page-capacity early finish (helios/src/scheduler.rs:1321) reports length before the limit is reached.

Tests run, CPU only: the full lib suite gives 1281 passed, 4 failed and 25 ignored on the branch head, and 1280 passed and 5 failed on main plus the new test. The four common failures, and three in the integration targets, need FLARE_TEST_TEMPLATE_DIR fixtures and fail the same way on both trees. Not run: served requests and HTTP wire output for Messages and Responses.

@mark-yong
mark-yong force-pushed the fix/ignore-eos-finish-reason branch from f1cccd6 to 8aaf734 Compare November 5, 2026 19:00
@mark-yong
mark-yong marked this pull request as draft November 5, 2026 19:13
@mark-yong
mark-yong force-pushed the fix/ignore-eos-finish-reason branch from 8aaf734 to 01db7c7 Compare November 5, 2026 19:41
A request with ignore_eos suppresses end-of-sequence termination, so its last
sampled token can be an EOS id while it finishes on the output limit. The
terminal event classified that as a stop with Eos termination, which tells
clients a truncated generation completed normally.

Use the request's ignore_eos setting in the finish-reason classification, the
same way the emitting and logprob paths already do, and document the finish
reason each protocol reports for each termination cause. The new termination
also reaches the decoders before encoding: at the output limit a completed
tool call now reports the limit reason instead of tool_calls, open tagged,
DSML, and XTML envelopes are dropped instead of raising an incomplete-call
error, and the recipient parser no longer closes a final complete tool frame
that end-of-sequence termination would have closed, so with required or named
tool choice and no earlier completed call the request now ends in an error.
These follow from end-of-sequence being non-terminating and match the
documented truncated-envelope rule.

Validation: the regression phase test fails on c65024d with only the test
applied, at the terminal-event assertion (reason "stop", termination Eos), and
passes with the fix. Release Helios library suite on the build host (CPU):
1276 passed, 9 failed, 25 ignored with the fix; the same suite on c65024d with
only the test added is 1275 passed and 10 failed, the new case being the only
difference. The nine shared failures are environmental: four engram staging
memory-limit cases, four missing FLARE_TEST_TEMPLATE_DIR fixtures, and one
package-cache staging case. Separately, builds at 0ce6de8 served one prepared
single-GPU package side by side with an explicit seed at the clamped prepared
defaults: a request that reaches its output limit on an EOS token reported stop
on the unmodified build and length on this one in all 30 requests, while
requests that honour EOS, stop before it, or ignore it without ending on it
reported the same reason on both builds. rustfmt drift on the touched file is
unchanged. Material limits: the regression drives the simulated worker phases,
the served check used /v1/chat/completions on one prepared package, and the
sampled-EOS suppression itself is unchanged.

Design-Signed-Off-By: Hermes Agent (GLM-5.3-Flash); design-review=a38f
Signed-off-by: Mark Yong <mark-yong@users.noreply.github.com>
@mark-yong
mark-yong force-pushed the fix/ignore-eos-finish-reason branch from 01db7c7 to 449b870 Compare November 5, 2026 19:49
@mark-yong

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. All four points have been addressed on the branch, now a single commit (449b8708) on c65024db.

  1. Parser behavior: The PR description now explicitly discloses all three downstream changes: completed tool calls report the output-limit reason, incomplete tagged/DSML/XTML envelopes are discarded rather than raising EOS errors, and the recipient parser no longer implicitly closes a final tool frame. The last case also causes required or named tool choice to fail when no earlier call was completed. These follow from treating ignored EOS as non-terminating. Requesting explicit maintainer acceptance or rejection of these semantics.

  2. Commit text: Removed merge history, distinguished the earlier served A/B at 0ce6de86 from the final CPU comparison against c65024db, corrected the sampling description to an explicit seed at clamped prepared defaults, and recorded 30 requests in the defect cell. Added the DCO Signed-off-by trailer and corrected the design-review acknowledgment to identify Hermes Agent (GLM-5.3-Flash).

  3. Commit structure: Folded the regression test and fix into one commit, so no intermediate commit fails its own test.

  4. Documentation: Updated docs/api.md with the per-protocol finish-reason mappings, output-limit precedence over completed tool calls, and exceptions for coincident stops and page-capacity exhaustion. Raw Completions is distinguished from Chat Completions.

The speculative-round case is also covered by the same classification in Worker::publish. With ignore_eos enabled, a final committed EOS token at the output limit, and no overriding user stop, both plain and speculative decoding now report (length, OutputLimit) instead of (stop, Eos).

Final-tree validation: 1,276 passed, 9 failed, 25 ignored. The same nine environmental failures occur on c65024db with only the regression test added (four missing FLARE_TEST_TEMPLATE_DIR fixtures, four engram memory-limit failures, and one package-cache staging failure). The new regression fails without the fix and passes with it.

The served A/B was not rerun after the rebase; the runtime fix is unchanged. The earlier comparison covered Chat Completions only. Messages and Responses have not been exercised over the wire.

@original-el8

Copy link
Copy Markdown
Collaborator

thanks for the updates. this looks good to me now. @lukealonso good to go.

@lukealonso
lukealonso marked this pull request as ready for review November 5, 2026 19:57
@lukealonso
lukealonso merged commit 4a570b0 into NVIDIA:main Nov 5, 2026
tunamitom added a commit to tunamitom/flare-local-inference that referenced this pull request Nov 11, 2026
…op-16 (numerics change on re-export)

Above eight rows the recipe left the route exchange entirely: the general
path's latent all-reduce (rows x 3584 BF16), router all-gather, two
copies and the native route top-k. MAX_ROWS goes 8 -> 20 so decode rounds
of nine to twenty rows (two concurrent speculative streams verifying up
to 2 x (depth + 1) rows, or four-stream rounds up to twenty) stay on the
route exchange: the paired gather plus the standalone top-16 selects
their experts; the fused single-exchange select stays at <= 8 rows. The
decoder routes every 9-20-row round onto this path unconditionally; the
recipe prepares 1-20 rows or raises at preparation, with no pin
detection and no fallback to the general path.

Transport and speed: the route path is not push-gated -- it runs on
whatever paired-gather transport the b12x plan selects. The per-layer
speed evidence (30.75 / 38.35 / 48.57 / 59.63 us saved at 9 / 12 / 16 /
20 rows; slowest-rank medians, >= 200 graph replays x 5; general path
47.05-80.52 us vs route path 16.30-20.89 us per MoE layer) was measured
on a push-planned exchange. The pull transport at nine to twenty rows is
UNMEASURED -- that is the path a default upstream deployment takes at
these widths. The push path carries the evidence, no speed claim is made
for the pull transport, and measuring it at 9/12/16/20 rows is a
follow-up before this path is relied on by default.

Numerics change on re-export at 9-20-row rounds (flagged in the
subject): the latent arrives by gather instead of the BF16 all-reduce,
so the hidden-shard sums regroup -- 2.63e-3 relative RMS against the
general path at every width, one BF16 unit roundoff of the general
path's partial sums. The route path is strictly closer to the FP32
product (1.66e-3 vs 2.36e-3 rel RMS). Selected ids are identical to
the general path at 9/10/12/16 rows over 6 banks x 8 ranks (weights
within 5 fp32 ULP), with one near-tie 16th-slot swap row in 960 at 20
rows (near-tie gate re-run: 3.2M rows at 9/12/16/20, every set
difference a 1-for-1 swap within 2 fp32 ULP, set rate <= 5e-5, 0
violations). Existing captured packages are unaffected; every
re-exported Kimi package changes arithmetic at those widths on two-
and four-stream decode, and the maintainer must accept that trade.

Pin dependency: rows 17-20 need the FlashInfer standalone top-16 that
accepts the full range (the pin declares KIMI_TOPK16_MAX_ROWS = 20).
Their route-gather pull request NVIDIA#101 covers 9-16 rows; this change
extends the recipe to 20 rows and needs NVIDIA#102's standalone select. The
FlashInfer pin bumps when NVIDIA#102 merges; until then, preparation for
9-20-row rounds requires it -- on an older pin the top-16's row check
raises at preparation of those widths, loudly, by design.

Tests: tests/recipes/test_kimi31.py 33 passed (1/2/8/9/12/16/20-row
route-gather surface selections -- one row fuses the select, two to
twenty rows pair the exchange with the standalone top-16 -- plus the
21-row envelope rejection and the 25 existing tests); with
test_kimi3_preparation.py 57 passed. The Flare test mocks the b12x plan
surfaces: it proves which surfaces are planned, not results. The GPU
evidence is the flashinfer side's CUDA tests for the top-16 row-range
change; no Flare-level GPU route test was run on this tree. Earlier
end-to-end round numbers were measured with a full settings bundle and
are dropped rather than misattributed.

Validation: tests/recipes/test_kimi31.py + test_kimi3_preparation.py
57 passed on this branch against the release-lane base's 49 (the 8 new
items; the remaining tests are identical and pass at base -- base-failure
parity holds, no test that passes at base fails here). Material
limitations: no Flare-level GPU route test and no pull-transport
measurement (above); the route-vs-general arithmetic comparison is from
the recorded measurements (6 banks x 8 ranks), not re-run here.

Numerics class: NUMERICS-LIGHT, BF16-structural -- ids identical except
the quantified near-tie swaps; the latent regrouping above is the
documented one-BF16-roundoff difference, closer to FP32 than the
general path.

Design-Signed-Off-By: Hermes Agent (hermes-agent); design-review=a38f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:api Serving API, protocol encoding, tool parsing, and client-visible behavior. bug Something isn't working priority:P1 High attention: unresolved correctness, serving failure, or model-data deletion risk. triage:review Proposed change needs code review; this does not imply merge readiness.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants