feat(sft): require renderer-based tokenization#2988
Conversation
SFT tokenization now goes exclusively through the renderers library: - build_training_sample with a hand-coded renderer replaces the incremental apply_chat_template masking path, which corrupted loss masks under position-dependent chat templates (e.g. Qwen3 stripping past thinking blocks across user turns). DefaultRenderer is rejected in setup_renderer; fake data skips renderer construction entirely. - [renderer] config defaults to auto-resolution from the tokenizer; template controls (e.g. enable_thinking) are set run-wide via the typed renderer config. - Non-assistant roles opt into the loss via the renderer's body-only path (content_sft_roles). - _drop_null_fields strips Arrow phantom nulls before tool-call deserialization so genuine nulls inside argument strings survive. - Null-check rather than key-check for messages vs prompt/completion resolution (Arrow schema union adds messages: null to prompt/ completion rows). - seq_len default 256 (FakeDataConfig keeps 128); loss_impl defaults to liger_fused; CI reverse-text configs pin [renderer] name=qwen3. Split from #2942 (part 1/7 of #2485), renderer-only without data_files.
…own chat template PrimeIntellect/Qwen3-0.6B and -1.7B ship a chat_template.jinja distinct from Qwen3's, so pinning [renderer] name="qwen3" trained them with the wrong template. setup_renderer now honors an explicit name="default" (apply_chat_template with the checkpoint's own template) and only rejects the silent auto-resolution fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Renderer pairing is checked once in __init__ (tokenizer=None stays the tests' passthrough mode) instead of raising mid-processing; the chat_template_kwargs backward-compat warning is dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirrors the OrchestratorConfig validator: renderer.name='auto' with a model outside MODEL_RENDERER_MAP now fails config validation (and --dry-run) instead of at trainer startup. The redundant runtime guard in sft/train.py is removed; fake-data runs without validation stay exempt since they build no renderer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hubert-marek
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36bc39b6f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if model_id in MODEL_RENDERER_MAP: | ||
| return self |
There was a problem hiding this comment.
When an SFT config sets tokenizer.chat_template for a model that is in MODEL_RENDERER_MAP, this validator now accepts renderer.name='auto', but training will instantiate the hand-coded renderer instead of the tokenizer's apply_chat_template path. That silently ignores the custom template that setup_tokenizer loaded, whereas the previous default SFT tokenization path honored it; users need to be forced to choose [renderer] name='default' or an explicit matching renderer in this case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d97cf5f0ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
hubert-marek
force-pushed
the
feat/sft-renderer-only-nodatafiles
branch
from
9eeee0c to
d97cf5f
Compare
Bump renderers to the PrimeIntellect Qwen3 typed renderer, migrate the affected configs off default/qwen3 selections, and reject DefaultRenderer whenever SFT renders real samples. Co-authored-by: Cursor <cursoragent@cursor.com>
The base PrimeIntellect Qwen3 checkpoints now auto-resolve to the typed renderer; only renamed fine-tunes keep an explicit selection. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 839a543be4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Defer to the renderer's sampled_mask by default so turn-closing role markers attributed to the next message stay trainable, and append the canonical stop token when a final assistant turn renders without one. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 592743f. Configure here.
| return self | ||
|
|
||
| model_id = self.tokenizer.name or self.model.name | ||
| if isinstance(self.renderer, AutoRendererConfig): |
There was a problem hiding this comment.
in which case is it not instance of AutoRenderConfig ? shouldn;t it be just if self.renderer is not None ?
There was a problem hiding this comment.
It needs to resolve later by acutally checking if there is an renderer to be selected by auto
Bump renderers to main with ensure_final_stop and drop the trainer-side append. Co-authored-by: Cursor <cursoragent@cursor.com>
These changes were unrelated to the renderer-only migration. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 041a1e5598
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| role_to_mask=role_to_mask, | ||
| tools=tools, | ||
| content_sft_roles=content_sft_roles or None, | ||
| ensure_final_stop=True, |
There was a problem hiding this comment.
This call relies on a newer renderers.build_training_sample keyword, but the project dependency still permits an unconstrained renderers install (and the configs package only requires renderers>=0.1.8.dev28). In an install path that does not use the checked-out submodule, a satisfying renderers release whose signature lacks ensure_final_stop will raise TypeError on the first real SFT example; I verified the installed 0.1.8.dev57 API in this environment has no such parameter. Please bump/pin the required renderers version alongside this new call.
Useful? React with 👍 / 👎.
2 participants
Summary
Split from #2942 (part 1/7 of #2485) — renderer-only SFT tokenization, without the local
data_filesingestion that ships separately.Makes SFT tokenization renderer-only and deletes the broken incremental masking path:
build_training_samplewith a hand-coded renderer replaces the incrementalapply_chat_templatemasking path, which corrupted loss masks under position-dependent chat templates (e.g. Qwen3 stripping past thinking blocks across user turns).DefaultRendereris rejected insetup_renderer(it falls back to incremental tokenization and doesn't fix the problem); fake data skips renderer construction so models without a hand-coded renderer can still benchmark step time. Non-assistant roles opt into the loss via the renderer's body-only path (content_sft_roles).[renderer]config defaults to auto-resolution from the tokenizer name; template controls (e.g.enable_thinking) are set run-wide via the typed renderer config. Per-examplechat_template_kwargscolumns are ignored with a warning.messagestakes precedence overprompt/completiononly when non-null — Arrow schema union addsmessages: nullto prompt/completion rows whenever other rows have amessagescolumn, and the previous key-presence check silently resolved those rows to empty message lists._drop_null_fieldsstrips Arrow phantom nulls before tool-call deserialization so genuine nulls inside argument strings survive.seq_len256 (FakeDataConfig keeps 128),loss_implliger_fused; CI + example reverse-text/wordle configs pin[renderer] name = "qwen3"(PrimeIntellect/* fine-tunes miss the auto-renderer's exact-match lookup).[val]uses real data (fake training data + validation would otherwise crash at the first val step), and the now-dead incremental-tokenization helpers (build_incremental_token_maskand friends) are deleted fromutils/chat_template.py.VLM SFT remains rejected at config here (
validate_renderer_vs_vlm) — multimodal support lands later in the stack.Tests
All 18 tests in
test_sft_dataset.pypass:Note
High Risk
This changes the core SFT data path and loss-mask semantics; misconfigured or unmapped renderers will fail validation or alter what tokens are trained, affecting training correctness for existing runs.
Overview
SFT now tokenizes exclusively through the
rendererspackage — the incrementalapply_chat_template/build_incremental_token_maskpath is removed fromSFTDatasetandchat_template.py.Training samples are built with
build_training_sample(sampled-mask loss by default, optionalcontent_sft_rolesfor non-assistant roles,ensure_final_stop=True).[renderer]defaults toautoand config validation rejectsDefaultRendereror unmapped models whenever real train/val data is used; fake-only runs can skip constructing a renderer. Dataset handling adds_drop_null_fieldsfor Arrow phantom nulls, null-safemessagesvsprompt/completion, and requires a renderer insetup_datasetfortype = "sft".Docs/examples shift to typed renderer config (e.g. Wordle RL uses
prime-qwen3); new unit tests cover renderer validation and dataset behavior.Reviewed by Cursor Bugbot for commit 041a1e5. Bugbot is set up for automated code reviews on this repo. Configure here.