Skip to content

fix(rlaif): accept preset reward_prompt template names - #6192

Open
rsareddy0329 wants to merge 6 commits into
aws:masterfrom
rsareddy0329:master-canary-test-fixes
Open

fix(rlaif): accept preset reward_prompt template names#6192
rsareddy0329 wants to merge 6 commits into
aws:masterfrom
rsareddy0329:master-canary-test-fixes

Conversation

@rsareddy0329

Copy link
Copy Markdown
Contributor

Issue #, if available:
reward_prompt did not accept preset template names via the Python SDK. Passing "summarize.jinja", "summarize", or any plain string (including the example from the SDK docs, "Rate the helpfulness of this response on a scale of 1-10") caused the SDK to treat the value as a HubContent name and call DescribeHubContent, failing with a validation error.

Only strings with an exact "Builtin" prefix were routed to preset resolution; everything else fell through to a HubContent lookup.

Description of changes:
Resolve preset template names directly against the recipe's judge_prompt_template enum (which is already loaded in memory as hyperparameters._specs — no API
call), matching RLVR's preset behavior. New resolution order:

  1. Preset template name → resolved locally against the enum. Accepts "Builtin.Summarize", "summarize", and "summarize.jinja".
  2. Evaluator ARN (arn:aws:sagemaker:...) → validated/assigned as-is.
  3. Otherwise → HubContent name lookup (registered custom prompt), raising a clear error if not found.

Details:

  • Added _normalize_template_name — strips an optional Builtin. prefix, takes the basename, strips an optional .jinja suffix, lowercases. So "Builtin.Summarize",
    "summarize", "summarize.jinja", and enum values like /opt/ml/code/verl/summarize.jinja all normalize to summarize.
  • Added _is_preset_reward_prompt and _get_judge_prompt_template_enum helpers; reordered routing to preset → ARN → HubContent.
  • Rewrote _update_judge_prompt_template_direct to use normalized matching (no longer assumes the Builtin. prefix) with a clearer error listing valid presets.
  • Preset detection/resolution is local; the HubContent API call only happens for genuine custom-prompt names.
  • Updated the SDK docstring examples from the invalid raw string to reward_prompt="summarize".

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 1m 11s —— View job


Code Review: fix(rlaif): accept preset reward_prompt template names

  • Read PR diff and changed source
  • Analyze correctness, backward-compat, and tests
  • Post review

Overall this is a solid, well-motivated fix. Routing preset names through the in-memory judge_prompt_template enum (no DescribeHubContent call) is the right approach, the normalization helper is clean, and the new tests cover the important cases (Builtin., plain, .jinja, mixed case, path-prefixed enum values, and the "plain preset must not call Hub" path). Comments below are mostly about edge cases and a documented behavior change — none are blockers.

1. Backward-incompatibility: custom HubContent names can now be shadowed

rlaif_trainer.py:444-453_is_preset_reward_prompt now intercepts any plain string whose normalized basename matches an enum entry. If a user has previously registered a custom HubContent prompt whose name collides with a preset (e.g. a custom prompt literally named summarize), it will now resolve to the preset template instead of doing the Hub lookup it did before — the custom prompt becomes unreachable by that name. This is an intentional precedence change (documented in the PR body, matches RLVR), but it is a behavior change for existing SDK consumers and worth calling out explicitly in release notes.

2. startswith("Builtin") is broader than the "Builtin." contract

rlaif_trainer.py:450_is_preset_reward_prompt routes on reward_prompt.startswith("Builtin") (no dot), while _normalize_template_name only strips a "builtin." prefix (with dot). Consequences:

  • A HubContent name like BuiltinCustomPrompt (no dot) is now forced down the preset path and will raise "not an available preset" instead of being looked up in Hub.
  • "Builtin" / "Builtinsummarize" normalize to "builtin" / "builtinsummarize", never match the enum, and error out.

Consider tightening the guard to startswith("Builtin.") so only the documented Builtin.<name> form is force-routed, keeping the plain-name enum match as the other preset path. Low severity, but it makes the two helpers consistent.

3. Explicit Builtin.* can be silently ignored when no enum exists

rlaif_trainer.py:444-453 + 495-501_is_preset_reward_prompt("Builtin.anything") returns True unconditionally. If the recipe has no judge_prompt_template enum and no current value, _update_judge_prompt_template_direct hits the return at line 501 and silently does nothing — the user's explicit preset request is dropped with no error and no Hub fallback. Given the goal is "clear error instead of a confusing Hub failure," an explicit preset that can't be resolved arguably deserves a raised error rather than a silent no-op. Edge case (requires an enum-less recipe), so minor.

Minor / nits

  • rlaif_trainer.py:481raise ValueError(...) inside the except swallows the original traceback; consider raise ValueError(...) from e. Pre-existing, not introduced here.
  • rlaif_trainer.py:466-477 — indentation of the session = TrainDefaults.get_sagemaker_session(...) block is misaligned (pre-existing, not touched by this PR, but nearby).

Tests

Good coverage. Two additions worth considering:

Note: the inline-comment tool wasn't available in this run, so findings are consolidated here with file:line references.

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.

1 participant