[rl] RendererConfig silently drops the removed preserve_* renderer knobs

[andylizf]

RendererConfig.build forwards knobs to the renderers config by matching field
names, and drops anything that does not match:

args = {
    field.name: getattr(self, field.name)
    for field in fields(self)
    if field.name != "name"
    and getattr(self, field.name) is not None
    and field.name in config_type.model_fields
}

renderers replaced preserve_all_thinking and preserve_thinking_between_tool_calls
with a single thinking_retention enum in PrimeIntellect-ai/renderers#88 (2026-06-26).
Neither name is a field on a renderer config any more, so both are dropped. Both are
still declared on RendererConfig and documented in its docstring, and
components/training_sample_builder.py recommends setting preserve_all_thinking=True
to keep thinking from being stripped out of history. experiments/rl/requirements.txt
tracks renderers at main, so the knobs have been inert since that commit.

The drop is silent, and it defeats a guard the library added for this case: renderers
keeps a validator that raises "[...] were replaced by thinking_retention" when an old
name reaches it, and the typed configs use extra="forbid". The name filter removes the
field before either can see it. Startup logs the result without flagging it:

Using renderer qwen3.5, of type <class 'renderers.configs.Qwen35RendererConfig'>, with args {}

Where we hit it: a recipe setting preserve_all_thinking=True ran under the renderer's
implied tool_cycle policy instead. For an agent loop that returns tool output as plain
user messages, every turn reads as a new user query, so bridge_to_next_turn refuses to
continue and the trajectory is re-rendered from scratch — 40694 of 40697 turns in one
run, against a code comment that expects zero. On a hybrid model without prefix caching
that is a full recompute per turn.

This PR:

1. Adds thinking_retention to RendererConfig so callers have a working path, typed
   as the library's Literal["tool_cycle", "all"].
2. Raises when a removed knob is set to True and the resolved config cannot take it,
   naming the replacement. An explicit False is left alone: it meant "defer to the
   chat template", which is what dropping it already does.
3. Forwards thinking_retention on the auto path by constructing AutoRendererConfig,
   which the library provides precisely to carry it through tokenizer-based resolution.
   Other knobs set there still reach nothing, so they raise rather than being ignored,
   and create_renderer(tokenizer, None) is kept unchanged when nothing is set.

Verified against the installed renderers: thinking_retention through auto resolves to
the same config object as naming the renderer explicitly, across all 53 models in
MODEL_RENDERER_MAP; create_renderer(tokenizer, None) and
create_renderer(tokenizer, AutoRendererConfig()) are equivalent; a matrix over names
(None, "auto", "qwen3", unknown) x both legacy fields (None/False/True) x
retention set/unset behaves as described; and existing callers that set only name and
enable_thinking, or pass tool_parser alongside a model-specific renderer, are
unaffected.

One gap left alone: config_from_name() still runs before the removed-knob check, so an
unknown renderer name reports that first and masks the legacy message. I can reorder it
if you would rather the legacy message win.

[meta-cla]

Hi @andylizf!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[pytorch-bot]

The following ciflow label(s) have been added but CI has not been triggered yet because the workflows are awaiting approval:

• ciflow/rl

Once a maintainer approves the workflows (scroll to the bottom of the PR page), the corresponding CI jobs will be triggered automatically. Please ping one of the reviewers if you do not have access to approve and run workflows.

[felipemello1]

hey @andylizf , thanks for the PR! I am on PTO until Monday. I will try to review over the weekend. If not, i will do it on Tuesday

[felipemello1]

we dont really do backward compatibility. If this is a dead knob, lets remove it

[felipemello1]

same here, no need to keep comments about historical motivation

[felipemello1]

@claude please review. What is a robust and elegant/simple way of forwarding configs without silently allowing errors like this? please think of 3 ideas with code snippets. Dont be verbose.

[claude]

Claude finished @felipemello1's task in 46s —— View job

---

Forwarding configs without silent drops

The root cause is the field.name in config_type.model_fields filter: it turns "knob the target doesn't accept" into "silently dropped." renderers already ships the guard we want (extra="forbid" + a validator that names the thinking_retention replacement). So the most robust move is to stop filtering and let the target reject unknowns.

Idea 1 — Delegate to the library's own guard (simplest, my pick).
Drop the membership filter; pass every set knob through. extra="forbid" and the removed-knob validator do the work, and any future rename fails loudly for free.

args = {
    f.name: getattr(self, f.name)
    for f in fields(self)
    if f.name != "name" and getattr(self, f.name) is not None
}
# config_type has extra="forbid" + a validator naming thinking_retention,
# so a removed/typo'd knob raises here instead of vanishing.
return create_renderer(tokenizer, config_type(**args))

Idea 2 — Don't mirror the schema at all.
Mirroring renderer fields on RendererConfig is what created the drift; the library is the source of truth. Hold a pass-through dict (or the typed sub-config) instead.

name: str | None = None
options: dict[str, Any] = field(default_factory=dict)  # forwarded verbatim
...
config_type(**self.options)  # unknown keys -> pydantic raises

Trade-off: loses per-field typing/discoverability in TorchTitan's config. Good when the wrapped schema churns often.

Idea 3 — Explicit diff, if you want to keep the typed mirror.
Keep the named fields for discoverability, but assert the mirror stays in sync instead of filtering. This is the generalization of what the PR already does for the two removed bools — applied to all knobs, so it works even without extra="forbid" and names the target.

provided = {f.name for f in fields(self)
            if f.name != "name" and getattr(self, f.name) is not None}
unknown = provided - config_type.model_fields.keys()
if unknown:
    raise ValueError(f"{sorted(unknown)} not accepted by {config_type.__name__}")

Recommendation: Idea 1. It deletes code, removes the mirror-vs-library drift entirely, and reuses the guard renderers built for exactly this. Combined with @felipemello1's two review notes — since we don't do back-compat, delete preserve_all_thinking / preserve_thinking_between_tool_calls and their historical comments rather than special-casing them — the whole removed-knob block collapses into the one-liner above; the auto path still needs its explicit thinking_retention forwarding since AutoRendererConfig is the only thing that carries it through tokenizer resolution.

Want me to push this simplification to the branch?
· fix-renderer-knob-drop