perf: batch trajectory generation in a single model.generate call (issue #6 item 4) - #17
hai-pilgrim wants to merge 3 commits into
Conversation
ReviewThe core batched generation logic is sound — left-padding for Issues to address: 1. Scope creep. This PR bundles ~160 lines of unrelated test additions (deps tests, data tests, KL tests) that overlap with dedicated test PRs (#18, #24, #29 — now merged). Please rebase onto main and drop the test additions that are already covered. The PR should only contain the batched generation changes + its own 5 trainer tests. 2. Misleading variable name. 3. Conflicts with #26. PR #26 tests Please rebase to resolve the overlapping test additions, and rename the variable. The core trainer changes look good. |
…sue marksverdhei#6 item 4) Previously training_step called _generate_trajectory once per (message, trajectory) pair, issuing N*T separate model.generate calls. This adds _generate_trajectories_batched which: 1. Repeats each user message num_trajectories times 2. Tokenizes all prompts with left-padding in one batch 3. Issues a single model.generate call for the whole batch 4. Decodes and pairs outputs back to their source messages Reduces GPU-CPU round trips from B*T to 1 per training step, which is meaningful when batch_size > 1 or num_trajectories > 1. Also adds 5 tests covering: pair types, message identity, num_trajectories repeat count, empty input, and empty-response filtering. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The variable holds the padded sequence length (input_ids.shape[1]), not individual prompt lengths. Rename to avoid confusion with the _get_prompt_lengths method which returns per-message token counts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f138681 to
6838ae9
Compare
PR marksverdhei#17 replaces the _generate_trajectory loop in training_step with a single _generate_trajectories_batched call returning (msg, response) tuples. Update all test mocks accordingly so tests remain valid after marksverdhei#17 merges. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
marksverdhei
left a comment
There was a problem hiding this comment.
The batched generation logic is solid and the variable rename looks good. However, this now conflicts with #20 (just merged) which refactored compute_loss/prediction_step into shared helpers (_compute_batched_kl, _prepare_pairs, etc.).
The conflict is in trainer.py — #20 restructured the methods that #17's training_step delegates to. Please rebase onto main. The _generate_trajectories_batched method and training_step changes themselves should apply cleanly; only the context around them shifted.
Will approve and merge once rebased.
|
Closing — we're pivoting away from on-the-fly trajectory generation toward SFT datasets. This optimization targets code that's slated for removal. Thanks for the contribution. |
Summary
Addresses item 4 from issue #6 (Performance: training pipeline optimizations).
Previously
training_stepcalled_generate_trajectoryin a Python loop, issuing onemodel.generatecall per (message × trajectory) pair —B × TGPU round trips per step. This PR replaces that loop with a single batched call.Changes
_generate_trajectories_batched(user_messages, num_trajectories)toPromptBakingTrainernum_trajectoriestimesmodel.generatecall for the entire batchtraining_stepto call_generate_trajectories_batchedinstead of the sequential loopImpact
The speedup is most significant when
batch_size > 1ornum_trajectories > 1, which are the common configurations.Test plan
🤖 Generated with Claude Code