Repository navigation
feat: max_off_policy_steps masks stale tokens instead of cancelling episodes - #4143
Merged
Merged
Conversation
faresobeid
force-pushed
the
feat/token-off-policy-mask
branch
from
October 18, 2026 17:03
61c2c52 to
d9f415b
Compare
faresobeid
marked this pull request as ready for review
October 19, 2026 00:07
# Conflicts: # src/prime_rl/orchestrator/clients.py
…mask # Conflicts: # src/prime_rl/orchestrator/train_sink.py
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3f05442. Configure here.
## Summary - `node_policy_versions` counts sampled tokens with `np.count_nonzero(node.mask)` instead of `sum(node.mask)`. `MessageNode.mask` is a numpy bool array since PrimeIntellect-ai/verifiers#2961, which #4226 pulled into main. Python's `sum()` walks that array one token at a time and returns an `np.int64`. `np.count_nonzero` is vectorized, accepts lists too, and the `int()` cast keeps the metric values as plain ints. - `test_token_policy_ages` builds its masks as numpy arrays, which is the type main now produces. Targets #4141. Checks: `ruff check` and `ruff format --check` pass. I also ran `token_policy_ages` on the test case with list masks and with numpy masks. Both returned `(20, 8, 3)` as plain ints. I did not run pytest, because the import chain needs `torch` and the test machine (macOS) does not have it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Small metric-calculation fix for numpy masks; behavior unchanged aside from correct int types and performance. > > **Overview** > **Fixes sampled-token counting** now that `MessageNode.mask` is a numpy bool array (from verifiers): `node_policy_versions` uses `int(np.count_nonzero(node.mask))` instead of `sum(node.mask)`, avoiding slow element-wise summation and keeping metrics as plain Python `int`s rather than `np.int64`. > > The unit test builds trace masks with `np.array(...)` to match production types. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5a96878. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
… feat/token-off-policy-mask
faresobeid
added a commit
that referenced
this pull request
Oct 24, 2026
…icy age (vLLM 0.32 weight_versions) (#4141) Engines label each sampled token with the policy step that produced it, and the orchestrator logs per-token policy age. - `/update_weights` takes `weight_version` and calls vLLM's `update_weight_version` after loading. `/load_lora_adapter` takes a `weight_version` query parameter and does the same. The orchestrator sends the step on both (filesystem, NCCL and NIXL transports). - vLLM 0.32 (on main) then returns `weight_versions` spans on every `/inference/v1/generate` choice; renderers/verifiers on main already pass them through to the trace. - `token_policy_ages` turns the spans on a trace's assistant nodes into per-token ages against the batch's training version. The orchestrator logs `off_policy/token/{mean,max}` next to the episode-level `off_policy/*`. - The startup sync goes through `/update_weights` before the dispatcher starts, so the boot step is labelled too. Only Dynamo engines (weight updates via `/collective_rpc`) keep the `"default"` label; those tokens are not counted. **Validation** - `pytest tests/unit/orchestrator tests/unit/test_configs.py tests/unit/inference -m "not gpu"` (445 passed), ruff, `uv lock --check`. - GPU (1×H200, Qwen3-0.6B, vLLM 0.32): a 4000-token generation paused mid-way, `/update_weights` with `weight_version=5`, resumed. It returned `[{"version": "default", "start": 0, "end": 1390}, {"version": "5", "start": 1390, "end": 4000}]`; the next request returned `[{"version": "5", "start": 0, "end": 8}]`. Upstream: vllm-project/vllm#59582 (in 0.32). #4143 is stacked on this PR. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Extends the weight-update and LoRA reload admin APIs to require `weight_version` and invoke vLLM's `update_weight_version`; a mismatch or missing field would break sync, but behavior is additive for training. > > **Overview** > Wires **vLLM 0.32 `weight_versions`** through the weight-sync path so each sampled token is tagged with the policy step that produced it, and the orchestrator can report finer-grained off-policy signal than episode-level staleness. > > After full weight loads (`/update_weights`) and in-place LoRA reloads (`/load_lora_adapter`), engines call **`update_weight_version`** with the training step the orchestrator already passes on those admin calls (filesystem / NCCL / NIXL and LoRA filesystem receive). Generate responses then carry per-token version spans on traces. > > New helpers **`node_policy_versions`** and **`token_policy_ages`** turn those spans into per-token ages vs the batch’s training policy; at ship time the orchestrator logs **`off_policy/token/mean`** and **`off_policy/token/max`** alongside existing episode **`off_policy/*`** metrics. Tokens still labelled **`default`** (e.g. Dynamo engines that skip this path) are excluded from the token metrics. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 0025738. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Mika Senghaas <mail@mikasenghaas.de> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
mikasenghaas
approved these changes
Oct 24, 2026
faresobeid
added a commit
that referenced
this pull request
Oct 25, 2026
Main now masks stale tokens instead of dropping rollouts (#4143) and names the bound max_staleness (#4253). Resolve the conflicts by following it: - Queued traces are saved with their episode, so the staleness sweep masks restored ones like any queued trace and they count in rollout metrics. - Saved group members are all kept and stay in their group's scoring; the resume-time drop and the scoring filter are gone. - Episodes are saved as python dumps with raw-bytes token streams: a JSON dump rounds logprobs and cannot round-trip routed_experts.
This was referenced Oct 25, 2026
Open
samsja
pushed a commit
that referenced
this pull request
Oct 26, 2026
`TrainSink.process_batch` crashes with `KeyError` on the stale-token metric path (#4143, renamed in #4253) as soon as a queued trace has tokens past `max_staleness`. Seen at step 18 of an async debug RL run. `vf.Trace.branches` is a property that builds new `Branch` objects on every access. The trainable masks were keyed by `id(branch)` from one `iter_trainable_branches(trace)` call, then looked up with a branch from a second `trace.branches` call. The ids never match, unless CPython happens to reuse the freed address. When it does reuse one, the lookup can silently return a different branch's mask. Fix: key by `branch.index` and take the branch and mask from the same `iter_trainable_branches` pass. Only the `staleness/token/masked` metric was affected. Masking in `_drop_stale` already indexes a single `branches` list. Regression test: a partially stale trace ships its fresh token. On main it fails with the `KeyError`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Narrows a metrics-only bug in batch assembly; actual stale-token masking in `_drop_stale` was already correct. > > **Overview** > Fixes a **`KeyError`** when `TrainSink.process_batch` records **`staleness/token/masked`** for queued traces with tokens past **`max_staleness`**. > > The metric path used **`id(branch)`** from one **`iter_trainable_branches`** pass but looked up branches via **`trace.branches[sample.branch_index]`**, which rebuilds new **`Branch`** instances on each access so keys rarely matched (and could silently map to the wrong mask). It now keys trainable masks by **`branch.index`** and uses the **branch and mask from the same iteration**, matching how **`_drop_stale`** already indexes a single branch list. > > Adds **`test_partially_stale_trace_ships_fresh_tokens`** so a partially stale trace still ships fresh tokens and reports the masked-token rate without crashing. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 33d1543. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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.

max_off_policy_stepsnow bounds the age of each trained token instead of each episode.On main, the dispatcher cancels every train group whose dispatch version is older than
(step-1) - max_off_policy_steps, including finished members, and the sink drops queued traces by the same rule. A group ages out as a whole even when most of its tokens came from recent weights (in-flightkeep-mode updates).With this PR:
weight_versionsspans from feat: label sampled tokens with the policy step and log per-token policy age (vLLM 0.32 weight_versions) #4141, on main). Unlabelled tokens (Dynamo only) take their episode's dispatch version, which is the old rule for them.(step-1) - max_off_policy_stepsout ofmask,rl_weights,ce_weightsandref_kl_weights, at the queue sweep and at batch ship. Fresher tokens of the same sample keep training with the sample's advantage and their own inference logprobs.off_policy/dropped).stalecancel reason and the sink's whole-group void are removed. On Dynamo, a stale group now runs to completion and is dropped at the sink.off_policy/token/masked: fraction of the batch's sampled tokens masked for staleness.trainer/rl/loss.pyis token-level and reads the loss mask.Validation:
pytest tests/unit/orchestrator tests/unit/test_configs.py tests/unit/inference tests/unit/eval -m "not gpu"(512 passed), ruff. Not run end-to-end on GPU.Note
Medium Risk
Changes off-policy enforcement in the orchestrator training path (masking vs cancellation), which can alter throughput, batch composition, and train–inference mismatch; loss code is unchanged but what reaches the trainer does.
Overview
max_off_policy_stepsnow limits staleness per sampled token, not per rollout or group. The train sink masks tokens older than(step-1) - max_off_policy_stepsout ofmask,rl_weights,ce_weights, andref_kl_weights(using vLLMweight_versions; unlabeled Dynamo tokens use the episode dispatch version). Fresher tokens in the same sample still train; traces drop only when every sample has nothing left to train.Rollouts are no longer cancelled for staleness. The dispatcher’s pre–weight-update stale group drops, the
staleCancelReason, and whole-group voiding in the sink are removed;on_version_pendingonly blocks scheduling across the swap. Queue sweeps and batch shipment apply the token masks instead of episode-level drops.Observability: new
off_policy/token/maskedrate; docs/config describe token-level behavior; cancellation metrics treat pipeline voids as overload/shutdown rather than stale.Reviewed by Cursor Bugbot for commit 24b8b51. Bugbot is set up for automated code reviews on this repo. Configure here.