Skip to content

feat(token-id-capture): deliver rebuilt trajectories safely - #2126

Open
ananthsub wants to merge 7 commits into
ananthsub/tokidcap/builderfrom
ananthsub/tokidcap/delivery
Open

feat(token-id-capture): deliver rebuilt trajectories safely#2126
ananthsub wants to merge 7 commits into
ananthsub/tokidcap/builderfrom
ananthsub/tokidcap/delivery

Conversation

@ananthsub

@ananthsub ananthsub commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Attaches a safely rebuilt token trajectory to each participating rollout and retires capture evidence only after durable handoff.

Snapshot and retirement flow

sequenceDiagram
    participant C as Rollout collector or framework
    participant S as TokenSource
    participant B as Trajectory builder
    participant D as Durable downstream

    C->>C: select static agent capture or all_agents
    C->>S: freeze(rollout_id)
    S-->>C: TokenCaptureSnapshot(entries, incomplete, snapshot_id, version)
    C->>B: build frozen snapshot
    alt incomplete, missing, ambiguous, or unsafe
        B-->>C: mask_sample=true and diagnostics
        Note over C,S: Retain frozen evidence
    else one safe trainable chain
        B-->>C: rebuilt response and metrics
        C->>D: write rebuilt rollout
        D-->>C: durable acknowledgement
        C->>S: drop(rollout_id, snapshot_id, version)
        S-->>C: conditional result - frozen tombstone remains
    end
Loading

Retirement is outside reconstruction. A delayed consumer cannot delete a newer attempt, and a late writer cannot recreate a retired attempt.

Summary

  • Clears and finalizes records only for agents selected by static token_id_capture configuration or all_agents; tokenless nonparticipants remain ordinary evaluation rollouts.
  • Freezes the source before reconstruction so entries and incomplete state come from one atomic snapshot.
  • Masks missing, malformed, incomplete, ambiguous, multi-root, multi-chain, and empty-token builds.
  • Separates finalization from retirement and conditionally drops only the matching snapshot_id and version after durability.
  • Fsyncs Gym's JSONL result before retirement and retains failed or masked evidence.
  • Uses an installed caller-owned source without closing it, or constructs and closes Gym's default file source.
  • Fails before rollout dispatch when selected rows require rebuilding and the collector process has no source.

Depends on #2125. Documented in #2341.

@copy-pr-bot

copy-pr-bot Bot commented Jul 23, 2026

Copy link
Copy Markdown

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.

@ananthsub ananthsub changed the title ananthsub/tokidcap/delivery feat(token-id-capture): uniform delivery, per-agent scoping, retention Jul 23, 2026
@ananthsub
ananthsub marked this pull request as ready for review July 23, 2026 16:18
@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Jul 24, 2026
@ananthsub
ananthsub force-pushed the ananthsub/tokidcap/delivery branch from 74a48d6 to 71de7c5 Compare July 29, 2026 06:55
@copy-pr-bot

copy-pr-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Comment thread nemo_gym/token_id_capture/delivery.py Outdated
cmunley1
cmunley1 previously approved these changes Aug 6, 2026

@cmunley1 cmunley1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a question above, but i think its unlikely in normal cases and we could follow up unless you want to reply there now.

@ananthsub

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

SHIP — no reliability concerns.

Reviewed the token-capture delivery step and its wiring into run_rollouts. The correctness-critical properties hold:

  • Durable-before-delete ordering is correct. finalize_rollout_token_capture mutates result["response"]["output"] before orjson.dumps(result), so the persisted record carries the rebuilt trajectory; os.fsync(results_file.fileno()) runs before retire_rollout_token_capture, so source records are dropped only after the rebuilt trajectory is durable. No crash-window data loss.
  • Reward/scorer untouched — only response.output is replaced; reward and components are left alone (asserted in tests).
  • Never-raises holdstrajectories_from_source and source.drop are both wrapped; a failing transport masks one rollout, not the batch.
  • Retirement is safe against deterministic-id reusedrop is conditioned on seal_id+version; a stale build cannot delete a newer attempt's records. Masked/failed builds are retained as evidence, not retired.
  • Ownership/lifecycle correctowned_token_source is closed only when Gym constructed it (configured source or file store), never a process-installed source; second finalize call is a no-op.
  • Async hygiene clean — no httpx, no ray.get, all coroutines awaited; no undeclared imports; all symbols exported from the package __init__.

Test coverage is real and exercises masking, no-capture-key, nothing-recorded, mixed native/external batches, retention, and stale-record clearing.

NOTE (non-blocking, author's call): finalize_rollout_token_capture is invoked for every result including no_persist (kill_shaped) and failure_class rows that are never persisted, so a rebuild + possible masking warning is computed and discarded for those. Harmless — no reward/output impact and results aren't written — just minor wasted work and log noise on failure-heavy runs.

@ananthsub

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Review incomplete: the PR revision changed during review. The captured base (970603d) and head (84755ea) that this review analyzed no longer match the live PR, which now points at base b9746c7 and head 21e7ab0. The final pre-publish revision check therefore failed, so inline findings are withheld to avoid anchoring comments to a stale commit. Please re-run the review against the current head.

@ananthsub

Copy link
Copy Markdown
Contributor Author

/claude review

An unusable build has no rebuilt response and sets ``mask_sample``.
"""
if source is None or rollout_carries_token_ids(result):
return None

@cmunley1 cmunley1 Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With all_agents=True, if capture records are written, this early return skips the snapshot retirement https://github.com/NVIDIA-NeMo/Gym/pull/2126/changes#diff-4ed3026c5b1fe1967854548a34299c33ba36c8fd55c7cd2249324108dd0a550cR915, so those redundant records may accumulate. Should we avoid capture for agents who provide the token ids or still retire their captured snapshot?

cmunley1
cmunley1 previously approved these changes Aug 20, 2026
cmunley1
cmunley1 previously approved these changes Aug 21, 2026
A rollout's records are read, built into one contiguous response, and swapped in
for response.output, so a trainer reads response.output the same way for a native
agent and an external harness.

The step takes the record and a TokenSource, and nothing else. Both belong to the
caller, so every configuration question stays on the side that owns it: Gym's
rollout collection resolves its source from Gym's config, and a training framework
staging records through its own data plane passes its own source and never reads
Gym's config. That is what makes the step shared rather than Gym's alone; a caller
driving run_examples gets the same behaviour without reimplementing it.

What a rollout needs is read from the rollout. An item already carrying token ids
holds what the policy sampled, so that rollout is left alone; without them, the
recorded ids are attached. A batch can therefore mix native agents and external
harnesses, and a native agent moving onto capture simply stops arriving with ids
and starts being rebuilt, with no config change anywhere. It also makes the step
idempotent, since a rebuilt rollout carries ids.

Records are cleared before dispatch, because rollout ids are deterministic and a
store appends, and retired through the source once consumed. A rollout that lost a
call is masked rather than trained on with a hole, reported as one field,
mask_sample, at the top of the record: a consumer decides by reading it and
nothing else. The metrics dict keeps the reasons, not the verdict.

token_id_capture.rebuild_response says whether Gym reads records back at all. Off,
Gym stops after the write and the caller reads through its own source; without it,
a run capturing to its own transport has Gym reading a store nothing wrote to.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Resolve paired sources for readback and delay conditional capture retirement until the rebuilt rollout is fsynced, preserving masked evidence and newer attempts.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Use lifecycle terminology that states when rollout records become immutable without implying that durable delivery or retirement already completed.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
State the freeze, rebuild, handoff, and conditional retirement boundaries directly. Keep retention and masking comments aligned with the implementation.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Use the same static agent selection as request routing when clearing and rebuilding capture records. Tokenless agents that did not opt in remain ordinary evaluation rollouts.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Use caller-owned installed sources without closing them and fail before dispatch when selected rollouts cannot be rebuilt.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants