Skip to content

feat(core): model-architecture seam, chat templates, text antiprompt - #667

Open
khalilswdp wants to merge 1 commit into
JustVugg:devfrom
khalilswdp:feat/arch-seam
Open

feat(core): model-architecture seam, chat templates, text antiprompt#667
khalilswdp wants to merge 1 commit into
JustVugg:devfrom
khalilswdp:feat/arch-seam

Conversation

@khalilswdp

@khalilswdp khalilswdp commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

The engine is hardcoded to GLM-5.2: chat template, eos token, and forward-pass dispatch are all baked into colibri.c, so a second model family currently means a second binary (like olmoe.c). The smallest change that fixes this is a load-time seam:

  • arch.h + arch_glm.c β€” a ModelArch descriptor registry selected from config.json "architectures"[0] (falling back to "model_type"). Unknown architectures are refused up front with the supported list, instead of silently mis-reading another model's weights. A config with neither field predates the registry and keeps loading as GLM with a notice, so existing snapshots and oracle fixtures are unaffected.

  • engine.h β€” the shared engine types (Cfg, QT, Layer, Model, …) move verbatim out of colibri.c so a future arch module can compile as a standalone .c. The forward pass dispatches attention_rows/moe through a two-pointer vtable bound at load; GLM binds the existing built-ins. No speculative fields β€” a new family adds its fields in the PR that consumes them.

  • Chat template/eos from the descriptor β€” serve/run build turns from chat_prefix/chat_turn/chat_think/chat_eos instead of hardcoded GLM strings; the GLM descriptor is byte-identical to the previous literals. router_bias gains NULL guards for families without e_score_correction_bias.

  • antiprompt.h β€” a quantized model can emit a role delimiter like <|user|> as ordinary text tokens, so the id stop never fires (and serve drops role-marker ids from the stop set, [Bug]: Tool Calling Fail in serve APIΒ #401) and the model hallucinates the next turn. This matches the decoded text, streamed with a (longest-needle βˆ’ 1) hold-back so delimiters split across tokens are caught and never printed. COLI_ANTIPROMPT overrides or disables it.

Also: COLI_TPS_EVERY makes the tok/s heartbeat cadence configurable (default 8; the hardcoded 16 meant short replies never produced a reading; 0 silences).

Validation

  • make -C c check β€” clean build, 0 new warnings, C unit tests + Python
    stdlib tests pass (the two -Wformat-truncation notes gcc 15 emits are
    pre-existing on dev; a separate small PR fixes them)
  • CUDA changes β€” n/a (no kernel changes; arch_glm.o is pure C linked into
    every build)
  • No performance claims in this PR

Compatibility

  • The default CPU build remains dependency-free β€” the registry is pure C,
    two new headers + one .c, no new deps
  • No model files, generated binaries, or benchmark artifacts included
  • Defaults changed: none. All new behavior is selected by the model's own
    config.json or opt-in env vars (COLI_ANTIPROMPT, COLI_TPS_EVERY).
  • .gitignore gains pattern rules for build outputs (*.o, *.obj, …) so new
    sources' objects don't leak into git status.

@JustVugg

Copy link
Copy Markdown
Owner

Thanks for this β€” the architecture seam is a nice direction. It now conflicts on dev (colibri.c / Makefile moved with recent merges). Could you rebase onto latest dev? CI is green; once it's clean we can land it, then #670 on top of it.

@JustVugg JustVugg added the enhancement New feature or request label Jul 28, 2026
@khalilswdp
khalilswdp force-pushed the feat/arch-seam branch 2 times, most recently from 4843afe to 074b9a8 Compare July 28, 2026 21:24
@khalilswdp

Copy link
Copy Markdown
Contributor Author

Rebased onto latest dev (a3a5a75) β€” applied cleanly, no conflicts. The conflicts you saw were from an accidental fork-sync merge that briefly polluted the branch; it's been removed and the branch is back to the single seam commit, now based on current dev. Ready to land β€” and the windows suite is already re-stacked on top for the follow-up.

@khalilswdp

Copy link
Copy Markdown
Contributor Author

Rebased onto latest dev (c67dbab). One real conflict with #679: both changes guard the spec-decode emit loop β€” your g_mux_stop/g_mux_cancel (client STOP/CANCEL mid-turn) and this PR's g_astop (text antiprompt) β€” resolved as the union of the stop conditions so both features coexist.

Why the antiprompt guard exists at all: a heavily-quantized GLM can emit a role delimiter like <|user|> as ordinary text tokens (<,|,user,|,>) instead of the atomic control-token id β€” the id stop never fires and the model runs on, hallucinating the next turn. And in serve mode the role-marker ids are deliberately dropped from the stop set anyway (#401, protecting <tool_call> blocks from int4 argmax noise), so even the atomic token wouldn't stop there. The antiprompt matches the decoded text instead, streaming with a short hold-back (longest needle βˆ’ 1) so a delimiter split across tokens is caught and never leaked to the client.

Full local gate green at the rebased tip: build + C/Python suites, token-exact oracle 32/32 TF / 20/20 greedy, no new warnings. Ready to land β€” the windows suite is re-stacked on top and ready to follow.

@JustVugg

Copy link
Copy Markdown
Owner

dev has moved a fair amount (v1.3.0: Kimi K3 and Inkling engines, the ragged-attention grouped-scale fix, CUDA_RELEASE_HOST default, the Inkling expert-cache fix). This PR now conflicts β€” could you rebase onto latest dev? Nothing is wrong with the change itself; CI was green where it ran. Ping me once it's clean and I'll review/merge.

@terrizoaguimor

Copy link
Copy Markdown
Contributor

Read this properly because it overlaps my #716, and it turns out the overlap is almost nothing
textually and something real semantically. Both are worth knowing before either lands.

The design reads well to me. Refusing an unknown architectures[0] up front with the supported
list, rather than letting it mis-read another model's weights, is the right default β€” it is the
same reasoning behind the identity check in #700, and for the same reason: a wrong answer that
looks healthy is worse than a refusal. Keeping a config with neither field loading as GLM with a
notice is a good call too; that is exactly the tiny oracle fixture, and breaking it would have cost
someone an afternoon.

Textual overlap with #716: only the Makefile

I diffed it rather than guessing. Of the functions #716 touches:

overlap
model_init, pin_load, stats_dump_q, usage_load, the ROUTE_TRACE block none
moe none β€” you change the call site to g_ops.moe(...), #716 changes the inside
eusage moved verbatim into engine.h; #716 aliases it in model_init, which stays
Makefile dependency lines conflicts β€” you add arch.h engine.h arch_glm.o, #716 adds route_trace.h, to the same ~17 lines

So the only conflict is mechanical and both sides of it are wanted. Whichever lands second takes
five minutes. No reason to sequence these on my account.

The interaction that does matter

Today arch_glm.c registers one architecture with .model_type = "glm_moe_dsa", and #716
hardcodes that same string as the identity written into .coli_usage:

rt_init("glm_moe_dsa", c->n_layers, c->n_experts);

They agree by coincidence, and the coincidence ends the moment this seam does its job. With a
second architecture registered β€” the Qwen3.6 work in #712/#713 is the obvious next one β€” one
binary loads two model families and stamps both histories with the same identity. Two
consequences, both bad:

This is a problem with my PR, not yours; your seam is what makes it visible. The fix is small and
belongs on my side: derive the identity from the descriptor rather than hardcoding it, so it tracks
whatever model_arch_select returned:

rt_init(arch->model_type, c->n_layers, c->n_experts);

route_trace.h only needs a string it can hash, so it does not care where it comes from. I will
make that change in #716 if this lands first, or note it here if mine does β€” either order works,
it just must not be forgotten. If you would rather I hold #716 until this is in so the seam is
available to use, say so and I will.

One question about the descriptor

chat_turn is documented as "per-turn printf fmt, exactly two %s: (user, think)". Since the
strings are compile-time constants in a static registry that is fine today, but it is a format
string driving printf from a table β€” if an architecture is ever registered from anything but a
literal in this file, that becomes a formatted-output injection point. Worth a comment on the
struct field saying it must remain a literal, so the constraint outlives the current registry.

@terrizoaguimor

Copy link
Copy Markdown
Contributor

Correcting myself before this misleads anyone, including me.

I said above that the history identity in #716 would become wrong once this seam lands, and
committed to changing rt_init("glm_moe_dsa", …) to derive from the descriptor. I checked properly
before writing that patch, and it would have been a no-op: REGISTRY[] holds exactly one entry
today, whose .model_type is the same "glm_moe_dsa" string I hardcoded. Same output, plus a new
dependency from merged code onto an unmerged PR.

More to the point, I assumed the direction was one binary serving several families. The evidence
says otherwise β€” #712/#713 add c/qwen36.c as its own binary, the way kimi_k3.c and olmoe.c
already are. With one engine per family, an identity per binary is correct rather than fragile,
which is what #716 does.

So the hazard is real but conditional, and the condition is not this PR: it arrives the day a
second ModelArch is registered here and one binary can load two families. At that moment
rt_init has to take its identity from g_arch->model_type instead of a literal, or the two
families write histories claiming to be each other and #700's cross-engine refusal silently stops
firing. load_cfg already runs before rt_init, so the descriptor is available when that day
comes β€” it is a one-line change, just not one worth making before it means anything.

Sorry for the noise. The rest of the review stands, and #716/#719 have since merged, so the
Makefile conflict I flagged is gone too β€” @JustVugg's TEST_BINS rework in #731/#733 removed the
shared line entirely.

@JustVugg

JustVugg commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Rebase request β€” and an apology for how many of these you have had.

dev moved a lot in the last day: 19 PRs landed, including the Vulkan backend (#418), the Kimi K3 GPU tier (#705), Metal grouped-int4 (#457), the shared routing telemetry (#716/#719), and several CUDA and launcher fixes. This PR now conflicts.

Before asking, three things changed on our side so that this is the last one of these you should need for a while.

1. The main source of these conflicts is closed. c/Makefile had a single hand-written TEST_BINS line listing every test gate. Every PR that added a test appended to that same line, so any two such PRs conflicted by construction, even when they touched entirely unrelated code β€” c/Makefile appeared in 26 of 40 open PRs. #386 hit it twice while being rebased, and said so, which is what sent us looking.

Gates are now derived from the build rules (#733). Adding a test means adding your .c and its own rule, which land in different places in the file. There is no shared list left to conflict on.

2. We resolved what we could ourselves instead of asking. Eight PRs were unblocked by a maintainer pushing the merge to the contributor's branch rather than requesting a rebase β€” including three where both sides carried a real change and had to be merged rather than picked. No commits were rewritten. Yours is here because its conflict is in engine code (c/colibri.c or a backend), where guessing your intent would be worse than asking.

3. Merge order is oldest-clean-first from now on. A PR that is green and unconflicted merges ahead of anything opened after it. The reason some of you rebased many times is that newer, smaller PRs kept jumping the queue and resetting you β€” a starvation loop we built, not bad luck on your side.

What we need: one rebase onto current dev. If your only conflict was the TEST_BINS line, just drop your entry β€” your test is picked up by its own rule now.

If you would rather not, say so and we will close it with thanks and the branch stays yours to reopen. No pressure either way, and no hard feelings β€” several of these have been open a while through no fault of the author.

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