Skip to content

Carry the rollout id on both agentic paths - #2536

Merged
fzyzcjy merged 2 commits into
mainfrom
tom/fix-agentic-rollout-id-parity
Aug 14, 2026
Merged

Carry the rollout id on both agentic paths#2536
fzyzcjy merged 2 commits into
mainfrom
tom/fix-agentic-rollout-id-parity

Conversation

@fzyzcjy

@fzyzcjy fzyzcjy commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

What breaks

  • tests/e2e/sglang/test_session_v1_v2_parity.py::test_qwen3_8b_h200_fa3_agentic_v2_drop_retries_matches_v1_training_payload_bitwise fails on main:
AssertionError: sample.rollout_id is not bitwise equal
left = None, right = 0, path = 'sample.rollout_id'

Root cause

  • miles/rollout/generate_hub/agentic_tool_call.py stamps the rollout id only on the v2 branch:
    samples = result.samples
    if use_v2:
        rollout_id = input.sample.rollout_id if input.sample.rollout_id is not None else input.sample.index
        assert rollout_id is not None, "v2 agentic samples require input Sample.rollout_id or Sample.index"
        for sample in samples:
            sample.rollout_id = rollout_id
  • So v1 leaves sample.rollout_id at None while v2 sets it to the index. The parity test compares every field of the training payload, so the two paths are structurally unable to agree on this one.
  • The two halves landed independently and were never reconciled:
Change Date Effect
#2129 test(session): validate H200 v1/v2 agentic parity 2026-08-05 Adds the bitwise parity test over all payload fields
#2368 fix(rollout): group session v2 leaf samples 2026-08-11 Adds the v2-only sample.rollout_id stamping
  • The rollout id is sample identity, not a v2 concept — nothing about it is specific to the session-server path.

The fix

  • Hoist the derivation out of the branch, keep the assertion where it was, and stamp whenever a value is derivable:
    samples = result.samples
    rollout_id = input.sample.rollout_id if input.sample.rollout_id is not None else input.sample.index
    if use_v2:
        assert rollout_id is not None, "v2 agentic samples require input Sample.rollout_id or Sample.index"
    if rollout_id is not None:
        for sample in samples:
            sample.rollout_id = rollout_id
  • v2 is byte-identical in behaviour: the same assertion still guards only that path, and rollout_id is non-None there by construction, so the new if is always taken.
  • v1 gains the stamp only when derivable: if neither rollout_id nor index is set, v1 keeps its current behaviour rather than acquiring a new failure mode.

Evidence

  • The same change, applied on a chain based on 6033ac0eaf, turns that lane green: stage-c-2-gpu-h200 (0) went from Enabled 5 test(s)4/5 passed to Enabled 5 test(s)5/5 passed, running 51 minutes, so it is a real pass rather than a skipped or no-op lane.
  • ruff check miles/rollout/generate_hub/agentic_tool_call.py passes.

Note for reviewers

  • [AMD] Enable amd pr ci #2347 edits the same guard to if use_v2 and len(samples) > 1: for an unrelated reason. Whichever lands second will need a trivial rebase; the two intents are compatible, since the condition being narrowed there is the stamping loop rather than the derivation.

The rollout id is stamped on the v2 branch only, so a v1 sample keeps
None where its v2 counterpart gets the index. tests/e2e/sglang/
test_session_v1_v2_parity.py compares every field of the training
payload, so the two paths can never agree on sample.rollout_id.

Hoist the derivation out of the branch. v2 behaviour is unchanged --
the assertion still guards only that path -- and v1 now stamps the same
value whenever it is derivable, keeping the old behaviour when it is not.
@guapisolo
guapisolo force-pushed the tom/fix-agentic-rollout-id-parity branch from 8853230 to 20721de Compare August 14, 2026 03:14
Only multi-sample v2 results need a shared rollout ID. Keeping singleton IDs unset prevents postprocessing from treating ordinary rollouts as compact and preserves normal trimming and dynamic batch metadata.
@guapisolo

Copy link
Copy Markdown
Collaborator

Rebased fix direction after #2347: this branch now keeps rollout_id=None for v1 and v2 singleton results, and assigns a shared rollout ID only to v2 multi-leaf results via if use_v2 and len(samples) > 1.

This preserves ordinary postprocessing, including sample-count trimming and dynamic-global-batch metadata, while retaining v2 leaf grouping. Focused verification passed: tests/fast/rollout/generate_hub/test_agentic_v2.py (7 passed) and Ruff on agentic_tool_call.py.

The PR description still describes the superseded v1-stamping approach; this comment records the final behavior instead.

@guapisolo guapisolo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do changes to align with session server v2 design

@fzyzcjy
fzyzcjy merged commit 20b18e2 into main Aug 14, 2026
15 of 20 checks passed
@fzyzcjy
fzyzcjy deleted the tom/fix-agentic-rollout-id-parity branch August 14, 2026 03:47
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.

2 participants