feat(token-id-capture): chain a rollout's calls into one response - #2125
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
f2e23af to
873c11c
Compare
873c11c to
8e16358
Compare
8e16358 to
1b52f0b
Compare
1b52f0b to
e1825fe
Compare
a2efcc5 to
e5a9941
Compare
e5a9941 to
9b9ea4d
Compare
|
/claude review |
|
SHIP WITH CARE — trajectory builder + consumer for stitching captured token records into contiguous training trajectories. The correctness-critical parts hold up well. What I checked hardest (this feeds RLHF; wrong tokens/masks corrupt training silently):
Two non-blocking items posted inline:
Neither is a merge blocker. No BLOCKER or RISK findings — masking-on-uncertainty and the contiguity assertion make the failure mode "dropped sample," not "corrupted score." |
|
/claude review |
|
Review incomplete: the PR revision changed during review. The captured base ref ( |
|
/claude review |
| generated_tokens_captured: int = 0 | ||
| generated_tokens_delivered: int = 0 | ||
| # Only one chain is delivered per rollout. | ||
| # Sub-agent branches and post-compaction chains are dropped. |
There was a problem hiding this comment.
we should support returning the full tree / forest, or at least segments in a list with broadcasted reward.
Also would like to have a optional filter for certain prompts like title calls
| if not snapshot.entries: | ||
| built = _failed_build(rollout_id, builder, "capture contains no token records") | ||
| else: | ||
| built = _assemble(rollout_id, list(snapshot.entries), builder, model) |
There was a problem hiding this comment.
Should this be asyncio.to_thread(_assemble, ...)
| Identical cumulative sequences are ambiguous. | ||
| The caller quarantines an ambiguous subtree. | ||
| """ | ||
| matches = [n for n in candidates if _is_prefix(n.cumulative, prompt)] |
There was a problem hiding this comment.
With rollouts with 100s to 1000 turns, _infer_parent scans every prior node and compares full cumulative prefixes, which becomes O(turns³) as context grows, and the recursive walk() below might hit Python’s recursion limit 1k turn scale.
Should we make traversal iterative? Would a trie help here? We should test 1000 turn
A rollout's captured calls arrive unordered and have to be stitched back into the sequence the policy actually sampled. Call N+1's prompt begins with call N's prompt plus its generation, so the calls chain by token prefix, with the tokens between them (tool output, a new user turn) kept as interstitial context. That needs no cooperation from the harness, which is the point: the harness is opaque. When a rollout has more than one root, the delivered chain is the one whose root completed first. capture_tokens is awaited inside the model server's response path, so a call's record is durable before its response reaches the harness and the next call has not been made yet; for a sequential harness, completion order is dispatch order. Ordering on the record's created_at rather than on file order matters at num_workers > 1, where several processes append and their interleaving is lock order. Two shapes are not handled, both involving a second agent. An auxiliary call the harness makes on its own account can complete before the agent's first turn and be selected instead. Parallel sub-agents overlap, so completion order stops meaning dispatch order, and nothing in a record says which agent made a call. Both are follow-up work; until then a rollout that split reports chains > 1 and a delivered_fraction below 1 rather than failing quietly. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Seal capture snapshots before reconstruction and reject empty, ambiguous, multi-root, or multi-trajectory projections so uncertain policy output cannot receive rollout reward. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Keep the multi-trajectory builder available as a low-level primitive while making the single-response consumer fail clearly and safely. Cover external source freezing and its failure states directly. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Describe frozen snapshot input, fail-closed masking, and single-response delivery with short standalone comments and docstrings. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Builds one trainable Responses trajectory from a rollout's unordered token-capture records.
Reconstruction flow
flowchart TD E[Unordered TokenEntry records] --> Z[Exclude calls with no generated tokens] Z --> O[Order calls by prompt length] O --> P[Find the earlier call whose complete tokens are the longest strict prompt prefix] P --> A{Several candidates share the longest prefix?} A -->|yes| Q[Quarantine the ambiguous call] A -->|no| L[Attach the inferred predecessor] Q --> F[Build roots and chains] L --> F F --> S{Exactly one root and one trainable chain?} S -->|no| M[mask_sample = true] S -->|yes| R[Project contiguous Responses output] R --> C[Assert prefix continuity and report metrics]The predecessor relationship is inferred only during reconstruction: call B follows call A when A's complete token sequence is a strict prefix of B's prompt.
Summary
per_requestbuilder for multi-trajectory consumers, while single-response delivery rejects it explicitly.trajectories_from_sourcesuccess, incomplete state, source failures, and unsupported single-response modes.Depends on #2124. Consumed by #2126.