Skip to content

Fix RLVR integ test - #6183

Merged
zhaoqizqwang merged 1 commit into
aws:masterfrom
zhaoqizqwang:master
Aug 14, 2026
Merged

Fix RLVR integ test#6183
zhaoqizqwang merged 1 commit into
aws:masterfrom
zhaoqizqwang:master

Conversation

@zhaoqizqwang

Copy link
Copy Markdown
Collaborator

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

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @zhaoqizqwang's task in 43s —— View job


PR Review: Fix RLVR integ test

  • Read the PR diff (/tmp/pr.diff)
  • Review changed test file and surrounding context
  • Verify against the code being tested

Review

This is a correct, minimal fix. ✅

The two RLVR dry-run integ tests (test_rlvr_dry_run_returns_none and test_rlvr_serverful_dry_run_returns_none) construct an RLVRTrainer without any reward signal. Since RLVRTrainer.train() now validates that a reward signal is present (rlvr_trainer.py:417-424), calling train(dry_run=True) raises ValueError before reaching the return None path, so both tests would fail.

Adding trainer.hyperparameters.preset_reward_function = "prime_code" satisfies that validation and lets the dry run complete as expected.

Consistency check: the change matches the exact pattern already used in test_rlvr_trainer_integration.py:96 and the value/attribute suggested by the trainer's own error message (rlvr_trainer.py:423). "prime_code" is a documented preset value, and the attribute name is correct.

Minor observations (non-blocking):

  • The reward-signal requirement is intrinsic to RLVRTrainer, so these tests would be more robust setting preset_reward_function at (or immediately after) construction is fine as-is — mutating trainer.hyperparameters post-init is already the established convention in this suite, so no change needed.

No correctness, security, or performance concerns. LGTM.

@mujtaba1747 mujtaba1747 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nice

@zhaoqizqwang
zhaoqizqwang merged commit ce42023 into aws:master Aug 14, 2026
12 of 16 checks passed
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.

2 participants