Fix Prime-RL custom trainer SFT staging#866
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e53897198
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| def test_prime_rl_custom_trainer_pretokenized_row_masks_last_assistant_only() -> None: | ||
| """The compatibility row matches the old shifted-label custom trainer path.""" |
There was a problem hiding this comment.
Name the guarded commit in the regression test
The repo's AGENTS.md says regression tests must name the PR/commit they guard. This newly added test exercises the shifted-label pretokenized compatibility fix, but its docstring only describes the behavior and does not name 1c0bf71 or a PR, so it violates that review rule and loses the traceability the convention is meant to provide.
Useful? React with 👍 / 👎.
Additional E2E validation after padding fixValidated commit
Conclusion: this PR fixes the BenchFlow-to-Prime-RL staging/trainer compatibility issue and removes the zero-tool collapse symptom seen in the previous sample-mean/token-suffix canary. It does not by itself recover pass rate on the 3-task behavioral canary, so the remaining regression should be investigated as model behavior/eval-runtime parity rather than a |
Summary
input_ids/target_ids/loss_mask/position_idsdirectly toSFTDataset._processcustom-trainer-token-suffixspelling tocustom-trainer-pretokenized, and update the CLI/help/testsWhy
The previous BenchFlow wrapper preserved the historical custom trainer's token tail, but then decoded it into a fake assistant message. With Prime-RL assistant-only labeling, that changed the old full-label custom trainer objective and could train on the wrong role/message semantics. The compatibility profile now stages the same shifted tensor targets the historical trainer exposed to HF CausalLM loss, while leaving Prime-RL optimizer/scheduler/checkpointing intact.
Validation
Local:
uv run pytest tests/test_train_cli.py -q->30 passeduv run ruff check src/benchflow/training/backends/prime_rl.py src/benchflow/cli/train.py tests/test_train_cli.pyuv run ruff format --check src/benchflow/training/backends/prime_rl.py src/benchflow/cli/train.py tests/test_train_cli.pyuv run ty check src/benchflow/training/backends/prime_rl.py src/benchflow/cli/train.py tests/test_train_cli.pygit diff --checkH100 remote smoke:
30 passedbench train run sft --compat-profile env0-mobile300-pr828 --data /home/ubuntu/work/mobile300-fixed/datasets/local_mobile300 --prime-rl-dir /home/ubuntu/work/prime-rl --dry-runoverall_status=succeeded,custom_trainer_pretokenized_rows=300,custom_trainer_pretokenized_trainable_tokens=937095,derived_max_steps=37,tool_defs_removed_rows=300messages/tools/tool_defs, tensor keys only,sum_trainable=937095sitecustomize.pyenabled both sample-mean and pre-tokenized data shims;SFTDataset._processreturned the staged row tensors unchanged;prime_rl.trainer.sft.trainimported successfully under the sample-mean shim