feat(core): model-architecture seam, chat templates, text antiprompt - #667
feat(core): model-architecture seam, chat templates, text antiprompt#667khalilswdp wants to merge 1 commit into
Conversation
|
Thanks for this β the architecture seam is a nice direction. It now conflicts on |
4843afe to
074b9a8
Compare
|
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. |
074b9a8 to
810c437
Compare
|
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. |
|
|
810c437 to
182dcf4
Compare
|
Read this properly because it overlaps my #716, and it turns out the overlap is almost nothing The design reads well to me. Refusing an unknown Textual overlap with #716: only the MakefileI diffed it rather than guessing. Of the functions #716 touches:
So the only conflict is mechanical and both sides of it are wanted. Whichever lands second takes The interaction that does matterToday 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
This is a problem with my PR, not yours; your seam is what makes it visible. The fix is small and rt_init(arch->model_type, c->n_layers, c->n_experts);
One question about the descriptor
|
|
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 More to the point, I assumed the direction was one binary serving several families. The evidence So the hazard is real but conditional, and the condition is not this PR: it arrives the day a Sorry for the noise. The rest of the review stands, and #716/#719 have since merged, so the |
|
Rebase request β and an apology for how many of these you have had.
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. Gates are now derived from the build rules (#733). Adding a test means adding your 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 ( 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 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. |
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 + Pythonstdlib tests pass (the two
-Wformat-truncationnotes gcc 15 emits arepre-existing on
dev; a separate small PR fixes them)arch_glm.ois pure C linked intoevery build)
Compatibility
two new headers + one
.c, no new depsconfig.jsonor opt-in env vars (COLI_ANTIPROMPT,COLI_TPS_EVERY)..gitignoregains pattern rules for build outputs (*.o,*.obj, β¦) so newsources' objects don't leak into
git status.