fix(train): surface required hyperparameters instead of dropping them

[rsareddy0329]

FineTuningOptions.to_dict() builds the hyperparameters dict from self._specs but omits any None value. A spec marked required=True with no default that the user never set was therefore silently dropped from the training request, so a job could launch missing a required hyperparameter and only surface the problem via bad/failed results.

Add FineTuningOptions.required_keys() and have the shared _validate_hyperparameter_values() raise a clear error listing any required hyperparameter missing from the final (post recipe/override merge) request. The check is opt-in via an options arg passed only at the post-merge leaf-trainer call sites (SFT/DPO/RLVR/RLAIF/MultiTurnRL); the pre-merge base_trainer call is left unchanged to avoid false positives. Guarded on the concrete type so mocks are ignored.

Adds unit tests for required_keys() and the surfacing behavior.

Issue #, if available:

Description of changes:

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]

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

---

PR Review: surface required hyperparameters instead of dropping them

• Read PR diff
• Examine source context (FineTuningOptions, call sites)
• Assess correctness, tests, best practices
• Post review

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

Summary

The change is well-targeted and correct for what it covers. required_keys() reads _specs (set from options_dict in FineTuningOptions.__init__, common.py:26-37), and the opt-in options guard in _validate_hyperparameter_values (finetune_utils.py:1341) is defensively type-checked with isinstance(options, FineTuningOptions), so mock-based trainer tests keep passing. Because defaults are materialized as attributes at init (common.py:34-36), a required spec with a default is included by to_dict() and correctly not flagged; only required-with-no-default that nothing supplied gets surfaced. Placement of the check after the recipe/override merge in each leaf trainer is right — a recipe-supplied value is counted as present. Tests are thorough (both required_keys() and the surfacing/back-compat behavior). Imports (Optional, FineTuningOptions) are already present in finetune_utils.py. Nicely done.

A few things worth considering:

1. Gap: the serverful SMTJ path is not covered (base_trainer.py:1094)

The required-key check was added to the 5 serverless leaf trainers, but BaseTrainer._train_serverful_smtj also builds a final, post-merge hyperparameters dict and never runs the required check on it:

# base_trainer.py
final_hyperparameters = self.hyperparameters.to_dict()
_validate_hyperparameter_values(final_hyperparameters)          # line 1094 — PRE-merge, no options
...
final_hyperparameters = self._apply_recipe_to_hyperparameters(final_hyperparameters)  # line 1102 — merge
# no post-merge required-key validation here

Leaving line 1094 pre-merge (no options) is correct to avoid false positives, as the PR describes. But there's a natural post-merge point after line 1102 where the same _validate_hyperparameter_values(final_hyperparameters, self.hyperparameters) call could go. As written, any model that trains through _train_serverful_smtj — notably CPTTrainer (cpt_trainer.py), which does not define its own train() with the check and isn't in the modified set — can still silently drop a required hyperparameter, i.e. the exact bug this PR fixes. Is that intentionally out of scope? If so, a one-line comment at line 1102 noting why the post-merge check is omitted here would prevent the gap from being "fixed" incorrectly later; otherwise consider adding the check there too.

2. Empty-string is treated as missing — confirm that's intended (finetune_utils.py:1358-1362)

if hyperparameters.get(key) in (None, "")

to_dict() stringifies values, so a legitimately empty required string value ("") is reported as missing, and there's a test asserting this (test_required_empty_string_is_treated_as_missing). That's a reasonable default, but note it means a required param whose valid value is the empty string cannot be satisfied. Given these are fine-tuning hyperparameters this is almost certainly fine — just flagging the semantic so it's a conscious choice. Note also 0/False become "0"/"False" and are correctly not flagged.

3. Minor: type annotation and pre-existing import re

• required_keys(self) -> set (common.py:88) could be -> Set[str] for consistency, but common.py currently only imports Dict, Any from typing, so the bare set avoids adding an import — acceptable as-is.
• import re inside _validate_hyperparameter_values (finetune_utils.py) is redundant with the module-level import re (line 4), but that's pre-existing and untouched by this PR — not a blocker.

Verdict

Correct and well-tested for the serverless leaf paths it targets. The one substantive item is the serverful SMTJ / CPT gap (#1) — worth either closing or explicitly documenting so the fix isn't misread as complete. Items #2 and #3 are informational.