feat(sc): gate-authoritative token capture via an external TransferQueue sink - #3837
Conversation
fd8314c to
fc6a551
Compare
9496ff6 to
b3e70a0
Compare
|
/ok to test 14dde62 |
|
/ok to test 82689d1 |
ZhiyuLi-Nvidia
left a comment
There was a problem hiding this comment.
Review finding: advantage-estimator valid_mask is only honored by GRPO.
|
|
/ok to test 4e50901 |
Rollout token deltas are staged into TransferQueue by the vLLM worker (external sink), custody is tracked by Gym's capture ledger, and a finalizer actor pool assembles receipts into training rows — no token echo, no re-tokenization. Rebased onto main: the TQ runtime-env pin and the mtp refit fix were dropped (both superseded upstream), and uv.lock was regenerated against main's resolution. Signed-off-by: Prashant Thombre <pthombre@nvidia.com>
|
Thanks for the thorough review — went through all 21 threads. Summary below, grouped by outcome. Applied
Acknowledged, not changed
All commits signed off, pushed to this branch. Running the full unit suite (default + |
Full-suite job 6920206 (5266+ tests, default + --nemo-gym-only lanes) surfaced 14 real failures caused by this PR's own changes; the other 26 (6 test_flops_counter + 21 test_policy_validation.py -S) are pre-existing HF Hub 401/gated-repo failures from missing HF_TOKEN in this sbatch env, unrelated to this PR. - test_payload.py: two tests asserted the exact tags dict without the new num_routed_experts_backfilled key (comment #12's fix); one of the fixture completions is a _failed_completion() that genuinely gets backfilled, so its count is 1, not 0. - test_rollout_manager.py: two TestGenerateForFinalizationFlow tests asserted request.truncated, which no longer exists on ReassemblyRequest (comment #20's fix moved truncated computation into the finalizer). - test_single_controller_actor.py: nine tests construct _step_log_dict by hand and were missing the sample_masks key the mask-weighted train/reward fix (comment #11) now appends to unconditionally. - test_rollout_pump.py: one test's fake _master_config was missing token_capture, which the finalizer-drop retry path (comment #13 AI-2) now reads. Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
Syncs the token-capture branch with main. Resolves conflicts by preferring main's KVBatchMeta.subset()-based sharding (preshard.py), main's _patch_scalar_field_schema() monkeypatch over the deleted PROMOTE_1D_FIELDS block (schema.py), main's generic make_actor_runtime_env() venv fix over the branch's manual VIRTUAL_ENV override (nemo_gym.py), and this branch's _with_route_fields() wrapper over main's direct fields_with_optional_routed_experts() calls (tq_policy.py), while keeping both sides' independent test additions in the touched test files. Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
|
/ok to test f6305dc |
… lint after main merge The origin/main merge (f6305dc) auto-merged nemo_gym.py without conflict but dropped the token_capture kwarg from the new spinup_nemo_gym_actor wrapper introduced by #3367, while setup.py still passed it. Restore the parameter and forward it to build_nemo_gym_config; assert in the unit test that it reaches the actor config. Also fix the Lint CI job: point pyrefly.toml at the renamed rollout_reassembler*.py modules (the stale blackbox_finalizer / finalizer_actor entries made pyrefly abort), and apply ruff format/isort to the files the merge left unformatted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
|
/ok to test 37cbd31 |
…test setup_single_controller now indexes master_config.logger["wandb_enabled"] and ["wandb"] (main's full-result-table gating), so replacing the fixture's logger dict wholesale raised KeyError before the assertions ran. Extend the dict instead. Found while auditing the origin/main merge f6305dc. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
The pre-commit `ruff check --select I` hook is stricter than the default rule set, so `ruff check` alone missed this after the RolloutReassembler rename. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
|
/ok to test 30d300c |
…t tests Two unit tests drifted from code this branch added and went red in CI (run 33939587768 on 30d300c): - test_train_pump_e2e::_prepopulate_buffer inserts a ready slot into TQReplayBuffer by appending to its parallel lists by hand. The branch added _rollout_ids_list and _staging_keys_list to the buffer, so the first remove() indexed past the end of the empty staging list (IndexError in _remove_unlocked). Append None to both, matching what begin_group/commit and checkpoint restore do for finalized groups. - test_rollout_pump_writes_expected_tq_data pins the TQ tag schema. The branch added num_routed_experts_backfilled to VIOLATION_TAG_KEYS so every row's tags now carry it; add it to the expected set. The remaining red shard, L0_Unit_Tests_Megatron_Policy_1, is a main-side conflict between #3881 (test expects train_microbatch to forward model_slices_context_parallel_inputs) and #2957 (guard raises on it) and is inherited unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
|
/ok to test f54bbb1 |
Catches the telemetry branch up to main, which had moved 22 commits and tripped pr-branch-up-to-date-check (max 10 behind). Eleven files conflicted. Resolutions, in the order a reviewer would want them: - single_controller.py: main split _dispatch_one_prompt into a token-capture branch (generate_for_finalization + the finalizer actor pool, #3837) and moved the legacy generate_and_push loop into an else. Git interleaved our per-prompt span into the new branch, so the function was restored to main's exactly and the per_prompt_scope() + rl.sc.generate_and_push umbrella re-applied to the legacy loop only. The token-capture branch is left uninstrumented: it commits through the finalizer pool rather than here, so its attempt boundary is a different shape than this span describes. Recorded in the coverage-gap table in docs/observability/span-groups.md. - run_grpo_single_controller.py: keeps our startup_span / setup_span phases and adopts main's VLM processor (#4009), with the processor-aware tokenizer construction moved inside setup_span ("tokenizer") and processor= threaded to setup_single_controller. - run_grpo.py: adopts main's make_policy_factory() helper in place of our inline factory selection, keeping setup_span("workers"). - nemo_gym.py: keeps @accepts_trace_context and the run_rollouts / _stream_rollouts span split, alongside main's ledger control plane; both signatures widen to main's 4-tuple yield. - rollout_manager.py: keeps dispatch_with_trace_context and unpacks main's added resolved_agent_ref. - factory.py: adopts main's LocalDataPlaneConfig-aware observability lookup, keeping the telemetry_enabled_in_env() arm that installs the wrapper for its spans. - vllm_worker.py: keeps umbrella_trace_fn(U_MODEL_INIT) on _load_model beside main's _refit_with_reload_api_enabled. - config.py, worker_mixin.py, virtual_cluster.py, vllm_generation.py: both sides added adjacent imports or fields; all kept. The textually merged uv.lock was corrupt (a missing source field on opentelemetry-instrumentation-aiohttp-client, which matched more than one package), so it was regenerated from main's with uv 0.11.28 to match CI's revision 3. The result is 52 insertions: the nemo-lens rev bump and the three otel aiohttp packages, nothing else. Both uv lock --check runs pass. Verified: all 54 changed Python files compile, ruff check and format are clean, and the seven test_source_drift.py guards pass -- including the carrier guard, which confirms run_rollouts is still dispatched with trace context after the signature change. Signed-off-by: Raj Singh <rajsin@nvidia.com>
What this PR adds
Gate-authoritative token capture for NeMo-Gym rollouts on the SingleController path: instead of round-tripping token ids through the environment (token echo) or re-tokenizing text, the vLLM worker stages each model call's exact token delta into TransferQueue (the external sink), NeMo-Gym's policy model server custodies the lineage in a capture ledger, and a finalizer actor pool assembles verified receipts into training rows.
Enabled by
token_capture.enabled=true(NeMo-Gym + vLLM async engine only, validated loudly at setup). The legacy token-echo path is unchanged and remains the default.How it works (rollout → training row)
/ng-rollout/<id>) that rides the Gym run body — the TQ sample id is the capture key end to end (nemo_rl/data_plane/tq_token_sink.py,vllm_worker_async.py).nemo_rl/environments/nemo_gym.py,nemo_rl/experience/blackbox_finalizer.py).FinalizationRequest(group id, rollout ids, receipts, rewards — never token tensors) to a finalizer actor pool, which publishes canonical training rows into the TQ training partition and commits the replay-buffer slot (nemo_rl/experience/finalizer_actor.py,route_plan.py,single_controller.pyactor dispatch path).sample_mask=0, and the advantage estimator now takes a realvalid_maskinstead of hardwired all-ones (advantage_estimator.py).Supporting changes
TQReplayBuffertoken-capture mode:reserve(rollout_ids=…),commit_finalized,abort, staging-awareremove(nemo_rl/algorithms/async_utils/replay_buffer.py).nrlre1envelope decode in the sink, int16 fields broadcast as int32 (NCCL has no Short), all-placeholder groups dropped when routed dims are unknown (data_plane/schema.py,preshard.py,worker_mixin.py).token_captureconfig block with setup-time validation of the supported matrix; derived fields (control-plane auth token, capture dir) filled at setup (single_controller_utils/config.py,setup.py).vllmextra with intentionally different server stacks, and uv applies a requested extra to every workspace member, making the RL-vLLM + base-Gym worker environment unselectable otherwise (pyproject.toml, regenerateduv.lock).fd5e84d6(feat(token-id-capture): external-sink token capture (worker-owned staging, ledger, terminal attribution) Gym#2872, the tokidcap capture stack, merged into Gym main 2026-09-04).Tests
++token_capture.enabled=true.RAY_ADDRESS=localrun on a CPU node; final revalidation in flight).Not in this PR
The nano SWE launch recipe (configs, launchers) and the recipe guides are kept out to scope this PR to core logic. History note: this supersedes #3456 (same feature, rebased onto current main with a clean history; the TQ actor runtime-env pin and the mtp refit fix from that branch were dropped as both are superseded upstream).
🤖 Generated with Claude Code