Skip to content

Preserve model-declared EOS tokens in serve mode - #584

Draft
saskw2010 wants to merge 1 commit into
JustVugg:mainfrom
saskw2010:agent/preserve-declared-eos-in-serve
Draft

Preserve model-declared EOS tokens in serve mode#584
saskw2010 wants to merge 1 commit into
JustVugg:mainfrom
saskw2010:agent/preserve-declared-eos-in-serve

Conversation

@saskw2010

Copy link
Copy Markdown

What changed

  • Preserve every EOS token explicitly declared by config.json or
    generation_config.json when the engine runs in SERVE mode.
  • Continue filtering tokenizer-only special tokens, retaining the tool-call safety
    behavior introduced for [Bug]: Tool Calling Fail in serve APIΒ #401.
  • Add a cross-platform regression test covering the SERVE path.

Root cause

GLM-5.2 declares three EOS IDs:

  • 154820 β€” <|endoftext|>
  • 154827 β€” <|user|>
  • 154829 β€” <|observation|>

stops_arm_tok() initially loaded all three, but its SERVE branch subsequently
discarded every stop except the tokenizer's primary EOS. The Python CLI and
OpenAI-compatible server forward native output and do not own a streaming-safe role
boundary stop, so <|user|> was emitted as assistant content and generation continued
until NGEN.

User impact

Before:

GLM-5.2 is running.<|user|>...

After:

GLM-5.2 is running.

This affects private coli chat and the shared native path used by
/v1/chat/completions, including streaming.

Validation

  • Windows MinGW regression test: tests/test_stops.exe passes.
  • The test confirms that SERVE retains all three model-declared EOS tokens while
    filtering tokenizer-only <|assistant|> and <sop>.
  • Windows GLM-5.2 real-model smoke test through /v1/chat/completions:
    • HTTP 200
    • output exactly GLM-5.2 is running.
    • 9 completion tokens with max_tokens: 32
    • finish_reason: stop
    • no leaked role token

The model files were unchanged; the test used the existing validated 144-shard
checkpoint.

@ZacharyZcR

Copy link
Copy Markdown
Contributor

This is the right fix for the default, and it composes cleanly with the COLI_SERVE_ALL_STOPS escape hatch that just merged (#572) β€” worth stating how the two fit together, since we wrote #572 and diagnosed the underlying issue (#549).

The distinction is exactly the one #401 wanted and never drew. Three populations, not two:

  • tok_eos β€” always kept.
  • Model-declared EOS (config.json / generation_config.json β†’ c->stop_ids): <|user|> 154827, <|observation|> 154829. The model says these end a turn; discarding them is the v1.1.1 role-token leak. This PR keeps them.
  • Tokenizer-only specials (<sop>, <|assistant|>, …): kept out for tool-call safety. This PR still filters them.

The old serve branch collapsed the middle group into "discard," which is precisely the <|user|>-leaks-into-output / never-terminates bug from #549. Gating on "did the model declare it as EOS" is the correct predicate, and your regression test (config [100,101,102] β†’ 100/101/102 kept, 103/104 filtered) pins it.

Relationship to #572 (already on dev): they're complementary, not competing.

So the layering ends up: correct by default (#584), with a "stop on everything" lever for non-conforming containers (#572). No conflict β€” #584 just means the lever is rarely needed.

One thing worth a line in the code comment: #584 trusts config.json/generation_config.json to declare the EOS set. That's the right source of truth, but it's worth noting explicitly that a container with an incomplete eos_token_id is where COLI_SERVE_ALL_STOPS earns its keep β€” otherwise a future reader sees two overlapping stop-filtering mechanisms and wonders which is authoritative.

+1. Cleaner root-cause fix than the opt-in flag.

@JustVugg JustVugg added the enhancement New feature or request label Jul 28, 2026
@JustVugg

JustVugg commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Thanks for this β€” preserving the model-declared EOS tokens in serve mode is a real gap, and it is the kind of fix that only shows up when someone actually runs a model whose config declares more than one.

Two things before it can move, neither about the change itself.

It targets main. Everything here goes to dev β€” main is release-only and protected, so a PR opened against it cannot be merged without a bypass and gets skipped in every review pass. Retarget it to dev and it enters the normal queue. You can do that from the PR page ("Edit" next to the title, then change the base branch) without reopening anything or losing the discussion.

It conflicts with dev now. dev has moved a long way since this was opened β€” 162 commits, including the Vulkan backend, the shared routing telemetry, and a chat-template/EOS-adjacent change or two, so there is a real chance part of this is already handled and the rest wants rebasing on top rather than merging as-is.

Worth knowing before you spend time on the rebase: the main source of merge conflicts in this repo is gone. c/Makefile had a single hand-written TEST_BINS line that every test-adding PR appended to, so any two of them conflicted by construction β€” that file appeared in 26 of 40 open PRs. Gates now derive from the build rules (#733), so there is no shared list left to collide on. Merge order is also oldest-clean-first from here: a green, unconflicted PR merges ahead of anything opened after it.

It is also currently a draft, so if you are still working on it, no rush at all β€” this is a "when you pick it back up" note rather than a nudge.

If you would rather not carry it further, say so and we will close it with thanks; the branch stays yours, and the underlying issue (serve mode dropping declared EOS tokens) is worth an issue on its own even without the patch, so that whoever hits it next finds it written down.

@JustVugg JustVugg added the needs-rebase Confligge, serve rebase dell'autore label Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request needs-rebase Confligge, serve rebase dell'autore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants