Repository navigation
feat: by-handle payload segments: reader, packing, CP-window reads, GC - #3819
Closed
faresobeid wants to merge 6 commits into
Closed
faresobeid wants to merge 6 commits into
faresobeid wants to merge 6 commits into
Conversation
This was referenced Oct 3, 2026
faresobeid
marked this pull request as ready for review
October 3, 2026 23:06
This was referenced Oct 4, 2026
samsja
pushed a commit
that referenced
this pull request
Oct 4, 2026
Moves `BatchPacker` (35 lines) from `orchestrator/packing.py` into `orchestrator/batch.py` and deletes `packing.py`. The class only wraps `build_bin_cost` and `prepare_batch`, which already live in `batch.py`. Item 6 of the orchestrator review. Behavior change: none (pure move; the import in `orchestrator.py` is updated). ## Checks - `pytest tests/unit/orchestrator tests/unit/test_configs* -m "not gpu"`: 291 passed - `ruff check`, `ruff format --check`: clean - `python -c "import prime_rl.orchestrator.orchestrator"` OK - Equivalence: the class body is copied byte for byte (`git diff` shows only the move and the import lines). GPU checks: none needed. ## Conflicts - #3840 (FLOP estimator): textual, in the `batch.py` imports only. - #3652: its `Shipper` imports `BatchPacker` from `packing`, which becomes a one-line change to `from prime_rl.orchestrator.batch import BatchPacker`. - #3819, #3836, #3794 touch other parts of `batch.py`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Pure file move and import path update with no logic changes to training batch packing. > > **Overview** > **Colocates batch packing with the bin-packing helpers it already delegates to.** > > `BatchPacker` moves from `orchestrator/packing.py` into `orchestrator/batch.py` next to `build_bin_cost` and `prepare_batch`; `packing.py` is removed. The orchestrator now imports `BatchPacker` from `batch` instead of `packing`. Class behavior and `pack()` semantics are unchanged. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 986f5bf. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This was referenced Oct 6, 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.
Reworked onto vLLM 0.31 (stacked on #3888)
Branch
feat/payload-segmentsnow holds one commit1bcca4115on top of #3888's heade89019444(squashed from the six earlier commits; the content is the same as99fa8c814, with only an import conflict inbatch.pyresolved). This PR is closed, so GitHub still shows the old head and basemain. The intended base ischore/bump-vllm-0.31./home/fares/scratch/venv031, vLLM 0.31.0):pytest -m "not gpu" tests/unit/orchestrator tests/unit/test_configs.py tests/unit/inference: 307 passed on the stack top (feat: by-handle payload fields table (TRR weights, score-centering logprobs) #3836). Ruff: clean.prepare_batch+DataLoader._micro_batch_to_tensor(seq_len 16384/4096 with truncation, 2 DP, CP 1/2/4): 140/140 micro-batch comparisons bitwise equal to inline. The earlier None-vs-all(-1) mask difference no longer shows up.What
This is the consumer half of passing per-token side arrays (router-replay ids, sampling masks) by handle (design:
payload-by-handle-design.md). It adds no producer and changes no behavior on its own. #3820 (stacked) adds the vLLM-side writer, the switch and the GC.prime_rl/transports/payload.py:PayloadSegment(field, file, offset, pos, rows, dtype, shape)(msgspec, array-like):rowsfixed-width rows of one field in a shared file, starting at token positionpos.clip_segments(segments, lo, hi).read_field(segments, field, lo, hi, fill):preads only the rows in[lo, hi)into an int32 array filled withfill. Rows narrower than the field's widest row fill the leading entries (sampling masks are written at each request's own width). Later segments win.TrainingSample.payload/MicroBatch.payloadare appended last (wire-stable). They coexist with the inlinerouted_experts/sampling_mask.orchestrator/batch.py): truncation clips the segments, packing shifts them by the sample's offset in the micro batch, padding is "no segment", router-replay presence still gates bin compatibility (inline or by handle), dummy batches drop mask segments.trainer/rl/data.py,train.py):[lo, hi). The mask reads[lo+1, hi+1), so it is already on label positions (shift_tensor_left).train.pyskips the shift and shard for those tensors (payload_cp_window).Validation
test_payload_by_handle_matches_inline(one new test). 24 random samples carry router-replay ids and ragged sampling masks inline, and the same rows by handle: routing split across 2 files, masks one segment per position at its own width. Both go throughprepare_batch(truncation at 32, 2 DP ranks, padding to the CP degree, dummies) andDataLoader._micro_batch_to_tensor. The tensors are bitwise identical to inline for CP=1. For CP=2 they equalshard_for_cp(shift_tensor_left(inline))on every rank. The by-handle mask is narrower per window, and every dropped column is -1.pytest tests/unit/test_configs.py tests/unit/orchestrator tests/unit/train/rl -m "not gpu": 297 passed.ruff checkandruff format --checkpass.Not done here
GPU validation (2026-10-04)
Setup: 8x H200, vLLM 0.30.0, one integration branch (#3832 ⊃ #3831 ⊃ #3830, + #3794, #3793, #3820 ⊃ #3819) with router#61 + router#60. Exact comparisons use
VLLM_BATCH_INVARIANT=1(BI) with Qwen/Qwen3-30B-A3B (TP1, 48 layers, top-8) and the same token ids and seeds (temperature 1, top_p 0.95, top_k 50).payload_dir): tokens identical; file rows equal inline routing (uint8[rows, 48, 8]) and inline masks for 16/16. Real rows throughprepare_batch+DataLoader._micro_batch_to_tensor(seq_len 16384 and 4096 with truncation, 2 DP ranks, CP=1/2/4): routing equalsshard_for_cp(inline)and masks equalshard_for_cp(shift_left(inline))in 140 micro-batch/rank comparisons.Nonewhile inline gave an all -1 mask. Commits b1cee0f + 99fa8c8: when truncation cuts all mask rows,prepare_samplekeeps a zero-row, width-1sampling_maskplaceholder, so the trainer gets the same all -1 width-1 tensor as inline.test_payload_by_handle_matches_inlinenow includes such a sample (fails without the fix).Landing order
deps/renderers/deps/verifiersto the merged commits first)rlturn by-handle on for disaggregated P/DNote
Medium Risk
Changes how MoE router replay and sampling masks reach the trainer under context parallelism; incorrect windowing or sharding would silently skew training, though inline paths are unchanged and a parity test covers the new reader.
Overview
Adds by-handle transport for per-token side data (
routed_experts,sampling_mask): inference will write rows to shared files and pass lightweightPayloadSegmenthandles onTrainingSample/MicroBatch, alongside the existing inline fields.New
transports/payload.pydefines segments plusclip_segmentsandread_field(preadinto token windows). The orchestrator clips and repositions segments on truncation, shifts them when packing micro-batches, treats payload router replay like inline for bin compatibility, and strips mask segments from dummy padding batches. The trainerDataLoadertakes CP rank/size; whenpayloadis present it materializes experts/masks from files—for text batches only each rank’s CP chunk, with masks already on label positions—and setspayload_cp_windowsotrain.pyskips the usual mask shift and CP shard for those tensors. Multimodal batches still read the full sequence when CP may be deferred in the model.A unit test asserts by-handle tensors match the inline path through pack/truncate/pad and CP=1/2.
Reviewed by Cursor Bugbot for commit b1cee0f. Bugbot is set up for automated code reviews on this repo. Configure here.