Skip to content

Add unit test to prevent future regression of preset reward function - #6182

Open
mujtaba1747 wants to merge 3 commits into
aws:masterfrom
mujtaba1747:master-rlvr-hyperparam-bugfix
Open

Add unit test to prevent future regression of preset reward function#6182
mujtaba1747 wants to merge 3 commits into
aws:masterfrom
mujtaba1747:master-rlvr-hyperparam-bugfix

Conversation

@mujtaba1747

Copy link
Copy Markdown
Collaborator

Thanks to Claude review bot for the callout!

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

@mujtaba1747
mujtaba1747 deployed to auto-approve August 14, 2026 17:16 — with GitHub Actions Active
@mujtaba1747
mujtaba1747 deployed to auto-approve August 14, 2026 17:16 — with GitHub Actions Active
@mujtaba1747
mujtaba1747 deployed to auto-approve August 14, 2026 17:16 — with GitHub Actions Active
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @mujtaba1747's task in 49s —— View job


Code Review

  • Read PR diff (/tmp/pr.diff)
  • Review the new test against rlvr_trainer.py
  • Post review feedback

Verdict: LGTM ✅ — this is a clean, test-only change and I found no issues.

The new test_train_raises_when_no_reward_signal correctly guards the regression. I verified it against the source:

  • The guard in RLVRTrainer.train() (rlvr_trainer.py:417-424) is the very first statement in train(), so with wait=False the test reaches it deterministically without needing the heavier session/config mocks the other tests use.
  • getattr(self.hyperparameters, "preset_reward_function", None) is exactly the line under test, and Mock(spec=["to_dict"]) is the right technique here — a bare Mock() would auto-create a truthy preset_reward_function attribute and mask the guard, so spec=[...] genuinely forces the None/falsy path. The inline docstring explaining this is a nice touch for future maintainers.
  • match="requires a reward signal" matches the raised message, and the custom_reward_function default (unset) satisfies the other half of the and condition.

Minor, non-blocking observations (no action needed):

  • This test proves the "neither set" → raise case. If you want fuller coverage of the same guard, a complementary positive test (e.g. setting preset_reward_function and asserting train() gets past the guard) would pin down both branches — but that's optional and the existing test_peft_*/custom_reward_function tests already exercise the pass-through path indirectly.
  • mock_finetuning_options / mock_validate_group are set up but the assertion only depends on the guard firing first; that's fine and consistent with the surrounding tests.

Nothing to change from my side.

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.

3 participants