Skip to content

feat: opt-in renderer-based tokenization for SFT - #2471

Closed
ercbot wants to merge 6 commits into
PrimeIntellect-ai:mainfrom
ercbot:feat/sft-renderers-support
Closed

feat: opt-in renderer-based tokenization for SFT#2471
ercbot wants to merge 6 commits into
PrimeIntellect-ai:mainfrom
ercbot:feat/sft-renderers-support

Conversation

@ercbot

@ercbot ercbot commented May 10, 2026

Copy link
Copy Markdown
Contributor

Closes #2470

Adds use_renderer flag to SFTConfig, mirroring the RL path added in #2278. When enabled, SFTDataset tokenizes via renderers.base.build_training_sample (single render() + message_indices mask) instead of the incremental Jinja template path.

Using renderers for tokenization fixes multiturn sample drops and the Qwen3.5 system-only TemplateError crash for chat templates that render position-dependently.


Note

Medium Risk
Changes the SFT data/tokenization path and adds new config validation around renderer usage; misconfiguration could cause training to fail early or change which tokens are masked for loss.

Overview
Adds use_renderer + renderer configuration to SFT, including validation to forbid unsupported combos (e.g. VLMs) and to error when renderer-only knobs are set without enabling the feature.

When enabled, SFT now creates a renderers-based tokenizer (create_renderer) and routes dataset tokenization through build_training_sample, passing the renderer through setup_dataset for both train and validation; it also warns once when per-example chat_template_kwargs would be ignored and rejects falling back to DefaultRenderer.

Reviewed by Cursor Bugbot for commit 9bb1180. Bugbot is set up for automated code reviews on this repo. Configure here.

ercbot and others added 3 commits May 10, 2026 14:11
Adds `use_renderer` flag to SFTConfig, mirroring the RL path added in PrimeIntellect-ai#2278.
When enabled, SFTDataset tokenizes via `renderers.base.build_training_sample`
(single render() + message_indices mask) instead of the incremental Jinja
template path. Fixes silent multiturn drops and the Qwen3.5 system-only
TemplateError crash for chat templates that render position-dependently.

Default path is unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ercbot

ercbot commented May 10, 2026

Copy link
Copy Markdown
Contributor Author

Adding renderer arg validation across orch and sft configs causes minor code duplication code. But there doesn't seem to be an existing pattern to make that common easily.

@ercbot

ercbot commented May 10, 2026

Copy link
Copy Markdown
Contributor Author

Ran existing unit tests:

image

Also ran checked the two multiturn sft training issues I mentioned in #2470 can now be avoided by with --use_renderer

uv run sft @ examples/wordle/sft.toml --model.name Qwen/Qwen3-0.6B --tokenizer.name Qwen/Qwen3-0.6B --max_steps 1 --use_renderer  --renderer.name qwen3 --clean-output-dir
uv run sft @ examples/wordle/sft.toml --model.name Qwen/Qwen3.5-0.8B --tokenizer.name Qwen/Qwen3.5-0.8B --max_steps 1  --use_renderer --renderer.name qwen3.5 --clean-output-dir

@ercbot
ercbot marked this pull request as ready for review May 10, 2026 19:38

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8104505. Configure here.

Comment thread src/prime_rl/trainer/sft/train.py
…allback, reject pool_size

- Type `renderer: Renderer | None` in `SFTDataset` / `setup_dataset`; lift
  `renderers` imports to module scope.
- Raise in `train()` when `use_renderer=True` resolves to `DefaultRenderer`:
  the fallback uses incremental `apply_chat_template` and does NOT fix the
  position-dependent template bug `use_renderer` is meant to solve.
- Warn once per dataset when an example carries `chat_template_kwargs` while
  the renderer path is on (renderers don't forward template kwargs).
- Reject `renderer.pool_size` unconditionally in `SFTConfig`. SFT tokenizes
  synchronously (`num_workers=0`) and already gets one renderer per DP rank
  — an in-process pool adds nothing. Comment explains the implicit-pool
  framing.
- Drop the new renderer test (`assert sum(loss_mask) > 0` was too weak to
  earn its keep).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@hallerite

Copy link
Copy Markdown
Member

thank you @ercbot !

@hallerite hallerite left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — renderer-based SFT tokenization fixes the multi-turn / Qwen3.5 TemplateError bug as advertised, validators correctly gate VLM and renderer-args misconfig, DefaultRenderer fallback is refused with a helpful error, and renderer.pool_size is rejected for SFT (synchronous tokenization, one renderer per DP rank — no in-process pool needed).

Verified end-to-end on Qwen/Qwen3-0.6B + PrimeIntellect/Reverse-Text-SFT for 50 steps: training loop completes cleanly, loss descends monotonically, grad norms track the baseline path.

One follow-up tracked separately: the renderer path currently trains on <|im_start|>assistant\n scaffolding because Qwen3Renderer._render_assistant attributes the role opener to the assistant message via message_indices. RL avoids this via its structural prompt/completion split; SFT (single-render + build_training_sample) doesn't. PrimeIntellect-ai/renderers#33 just merged adding RenderedTokens.sampled_mask to fix this at the right layer; tracking the prime-rl-side bump as a follow-up issue. Doesn't block this PR — it lands the bug-fix that's actually requested.

@hallerite

Copy link
Copy Markdown
Member

@ercbot quick heads-up: CI on #2471 fails because GitHub doesn't expose org secrets to PRs from forks, so wandb crashes the trainer process. I pushed the same head SHA to a different branch and opened #2493 as a mirror so CI can run properly.

I'll make sure to merge that one with --author="Eric Botti <ericjbotti@gmail.com>" so you stay the primary author on the resulting commit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extend renderers support to SFT training to solve incremental tokenization issues

2 participants