Repository navigation
fix(train): hand off to the engine before an off-cadence final eval - #3615
Merged
Merged
Conversation
#3182 skips the last rollout's engine handoff (weight onload, update_weights, KV onload) unless a final eval follows, but its guard called should_run_periodic_action without args.num_rollout. That helper only treats the last rollout as due when num_rollout is passed, so whenever num_rollout is not a multiple of eval_interval the guard skipped the handoff while the eval gate below, which does pass num_rollout, still ran the final eval. With --colocate the eval then reached an engine left in the released state: the 2026-09-22 nightly lost test_qwen3_vl_4B_fsdp and test_qwen3_4B_fsdp_true_on_policy (--num-rollout 3 --eval-interval 20) to "Pointer argument cannot be accessed from Triton (cpu tensor?)" in write_req_to_token_pool_triton. Without colocation the final eval would score the previous rollout's weights. Compute eval_due once and use it for both the handoff guard and the eval gate, so the handoff runs exactly when a final eval needs it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Collaborator
Author
|
@claude review always |
Collaborator
Author
|
/rerun-test tests/e2e/fsdp/test_qwen3_vl_4B_fsdp.py |
Collaborator
Author
|
/rerun-test tests/e2e/fsdp/test_qwen3_4B_fsdp_true_on_policy.py |
Contributor
|
✅ |
Contributor
|
✅ |
yueming-yuan
approved these changes
Sep 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Runs the last rollout's engine handoff whenever a final eval follows, including off-cadence.
Symptom & Reproduction
main(366f614b8b, run 35745409894, attempt 1) failedtest_qwen3_vl_4B_fsdpinstage-c-2-gpu-h200 (0)andtest_qwen3_4B_fsdp_true_on_policyinstage-c-2-gpu-h200 (1)withValueError: Pointer argument cannot be accessed from Triton (cpu tensor?)inwrite_req_to_token_pool_tritonatEval ... 0%.release_memory_occupationbut noresume_memory_occupationorupdate_weights_from_tensorbefore the eval.tests/fast/test_train.py::TestFinalEval::test_an_off_cadence_final_eval_follows_the_last_handoff(num_rollout=3,eval_interval=2) fails onmain: noonload_weights/update_weights:2precedeeval:2.Root Cause
train.pyhandoff guard callsshould_run_periodic_actionwithoutargs.num_rollout.should_run_periodic_actionforces the last rollout only whennum_rolloutis passed.--num-rollout 3 --eval-interval 20, the guard is false on rollout 2; the handoff is skipped.args.num_rollout, so the final eval still dispatches.release_memory_occupationleaves the colocated engine released; eval requests crash its scheduler.Fix
train()computeseval_dueonce and uses it for both the handoff guard and the eval gate, so the two cannot disagree. The last rollout still skips the handoff when no final eval follows, which is #3182's intent. Every earlier rollout is unchanged. Without colocation the old code did not crash, but its final eval scored the previous rollout's weights; that also changes.Verification
tests/fast/test_train.py,tests/fast/test_train_async.py: 16 passed locally (CPU). The new off-cadence test fails onmain(8cdf3794d9) and passes here;test_the_last_rollout_skips_the_handoff_without_an_evalpasses on both.run-ci-fsdp.Review Focus
eval_dueintrain.py: the handoff must run for every rollout whose eval runs, and only then on the last one.🤖 Generated with Claude Code