Conversation
Signed-off-by: Laura Dang <laurad@nvidia.com>
…very.sh Signed-off-by: Laura Dang <lauradang.2000@gmail.com>
Signed-off-by: Laura Dang <lauradang.2000@gmail.com>
Signed-off-by: Laura Dang <lauradang.2000@gmail.com>
Signed-off-by: Laura Dang <laurad@nvidia.com>
Signed-off-by: Laura Dang <laurad@nvidia.com>
Signed-off-by: Laura Dang <laurad@nvidia.com>
Signed-off-by: Laura Dang <laurad@nvidia.com>
… cache Extract the vLLM worker's _fetch_chain_prefix and _resolve_admission_prefix into tq_token_sink.py as ChainPrefixCache and resolve_admission_prefix, and make the worker methods one-line delegates. TQMegatronPromptPreparer now resolves staging chains through the same pair, so both backends share one cached TQ read (256 entries keyed by the chain's last staging key, deepest cached key bounds the fetch to the uncached suffix). The preparer reads the splice boundary from the request-metadata keys the Megatron chat endpoint writes (prefix_splice_suffix_token_ids, prefix_splice_boundary_token_id). The key strings are spelled out here rather than imported so this module stays importable in the vLLM worker and finalizer environments; a test asserts parity with Megatron's constants when Megatron is importable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
Signed-off-by: Laura Dang <laurad@nvidia.com>
033f7a7 to
5bf90aa
Compare
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
❌ Submodule Fast-Forward Check FailedCheck based on commit: 5bf90aa (PR #4129 from ❌ Submodules that need attention:Gym: ❌ Commits have DIVERGED from a common ancestor Please ensure all submodule commits are fast-forwards of the main branch before merging. |
lauradang
left a comment
There was a problem hiding this comment.
Team review of PR #4129 (7 agents: rl-expert, expert-gym, expert-megatron-lm, bug-finder, test-agent, design-reviewer, devil-advocate). Static review only: this host is macOS without GPUs and the lockfile is Linux-only, so no tests or linters were run; every claim about upstream behavior is linked to source at the pinned SHAs.
Not landable at HEAD — five deterministic blockers, all inline:
- The Megatron capture path programs against Megatron-LM hooks (
payload_stager,prompt_preparer,PREFIX_SPLICE_*) that exist only on unmerged Megatron-LM #7015; the Megatron-Bridge pointer is unchanged, sosetup_token_captureraises on every Megatron capture run (megatron_worker.py). - The Gym pointer
c3d7cf4fis not reachable from any NVIDIA-NeMo/Gym ref and Gymmainis wire-incompatible with this RL (3rdparty/Gym-workspace/Gym). PY_EXECUTABLES.MCORE_GYMfails the existingtest_every_extras_py_executable_is_wired_to_an_actor(virtual_cluster.py).- The runtime
ACTOR_ENVIRONMENT_REGISTRYswap is inert in the official container, whose prebuiltMegatronPolicyWorkervenv lacksnemo_gym(setup.py). test_prefix_splice_keys_match_megatron_constantshard-fails the Nemo_Gym shard, and the barecheckpointing.save_data_plane=trueoverride fails the streaming-recovery L1 test.
Lint: not run here; two findings (import order in megatron_generation.py, a stray blank line in test_checkpointing.py) would fail pre-commit run --all-files in CI.
The design itself is sound where it is checkable: the durability boundary is identical across backends, TQMegatronTokenStager.stage() fails closed, ChainPrefixCache / resolve_admission_prefix are a clean dedup, the set_generation_epoch rank-0 fan-out is correct, and all Gym API usage matches c3d7cf4f.
Informational (not staged as findings, below the confidence bar): at the #7015 tip both engine hooks run synchronously on the MP-coordinator engine loop, so each captured request costs one or two ray.get TQ round trips during which that DP replica does not step; vLLM offloads the same fetch via asyncio.to_thread. Worth a capture-on/off throughput number once the pin lands.
Devil's advocate: 24 confirmed, 1 disputed (a claimed isort collapse of the reassembler import), 7 downgraded below threshold.
Generated by Claude Code
| @@ -1 +1 @@ | |||
| Subproject commit fd5e84d6b1c485c80e7ae61553bbd485611c03b4 | |||
| Subproject commit c3d7cf4ff484d0f4f07898aedeaba05eb4821438 | |||
There was a problem hiding this comment.
1 action item.
TL;DR — c3d7cf4f is not reachable from any NVIDIA-NeMo/Gym ref, and Gym main is wire-incompatible with this RL, so the pointer cannot fast-forward until the Gym side lands.
PR-introduced. git merge-base --is-ancestor c3d7cf4f fd5e84d6 → 1 (the bot's fast-forward check already fails). git ls-remote on NVIDIA-NeMo/Gym has no ref reaching c3d7cf4f; it is 40 commits stacked on open Gym #2278 on the lauradang/Gym fork, and .gitmodules points at NVIDIA-NeMo/Gym.git branch=main shallow=true. Against Gym main today, begin_call has no weight_version kwarg (RL passes it in TQMegatronTokenStager) and TokenIdCaptureSettings is extra="forbid" with no external_staging_backend, so the Gym config RL builds at nemo_gym.py:537 fails validation at Gym startup.
The open Gym PR for this work appears to be #2823, whose head is neither an ancestor nor a descendant of c3d7cf4f and which nests the Megatron admission under request_metadata.ng_capture (pinned c3d7cf4f writes it top-level).
AI-1
Action: Land Gym #2278 / #2823 (or a consolidated equivalent) on Gym main, then point this submodule at a main SHA. Before that, reconcile the Megatron wire contract (top-level ng_capture vs request_metadata.ng_capture) between RL, the Gym PR, and the Megatron-LM endpoint, and link the Gym PR instead of a fork SHA in the description.
Context — no action (Gym-side). At c3d7cf4f, _strip_capture_transport_fields does not strip compact_prompt_token_ids, which the Megatron endpoint emits under return_tokenized_data (chat_completions.py), so a prompt-length token array leaks to the agent hop per call. Gym #2823 already strips it, which is one more reason to follow that lineage.
There was a problem hiding this comment.
partially fixed in a9b17f2 — the pointer now targets the head of Gym #2823 (fc08bf19), which is reachable from NVIDIA-NeMo/Gym, fast-forwards from Gym main, and nests ng_capture under request_metadata as the Megatron-LM #7015 endpoint requires. Still remaining: Gym #2823 must land on Gym main before this can merge, and the PR description should link Gym #2823 instead of the fork SHA.
- Declare the nemo_gym extra on MegatronPolicyWorker in actor_environments.py (the venv source of truth) instead of swapping ACTOR_ENVIRONMENT_REGISTRY at runtime, which the prebuilt container venv ignored; drop PY_EXECUTABLES.MCORE_GYM and the ModuleNotFoundError string-match remediation. - Gate backend=megatron token capture at setup on the MInf capture hook protocols (RequestPayloadStager / RequestPromptPreparer) from Megatron-LM PR #7015, failing with a NotImplementedError that names the dependency while the Megatron-Bridge pin predates it. - Point the Gym submodule at Gym PR NVIDIA-NeMo#2823 (fc08bf19), which is reachable from NVIDIA-NeMo/Gym, fast-forwards from Gym main, and nests ng_capture under request_metadata as the Megatron endpoint requires. - Skip test_prefix_splice_keys_match_megatron_constants when the pinned megatron-core lacks the constants, and importorskip megatron.core in the Megatron hosting test, so the Nemo_Gym shard skips instead of aborting. - Restore the ++ Hydra override for checkpointing.save_data_plane in the streaming recovery script (the key is absent from the Gym config chain). - Document the Megatron-LM #7015 dependency in the design doc. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
|
/ok to test a9b17f2 |
- Add design-docs/token-capture-ledger.md to the docs toctree and drop links to guides that do not exist yet (Sphinx treats both as errors). - Read vllm_cfg / mcore_generation_config through a dict cast in the token-capture validation so pyrefly does not reject the TypedDict keys. - Apply ruff formatting to megatron_worker.py and test_checkpointing.py. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
…anup Stamp a Megatron request that spans a refit with its admission (oldest) policy epoch instead of failing capture and masking the whole rollout. This matches vLLM, which freezes the version at begin_call, and the finalizer's min-over-calls group tag; spans are counted (epoch_span_count) and logged at WARNING. Also fold in the mechanical review items: drop the undefined rollout_max_attempts_to_avoid_lp_nan knob, remove the dead KeyError branch in receipt parsing, route fetch_for_finalization through TQStagingStore, collapse the duplicate prefix predicate, call set_generation_epoch directly, fix import order, revert diff churn, fix the FIFO docstring and the manifest control route in the design doc, and remove the unused logical_request_id test parameter. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
macandro96
left a comment
There was a problem hiding this comment.
Re-review: PR #4129 at d678aa7
Main ones are:
-
The targeted NeMo-Gym unit subset fails with
14 failed, 38 passed, 1 skipped. Two test fixtures have drifted from the pinned Gym schemas; see inline comments. -
uv lock --checkis currently failing. The Gym submodule changed package requirements, but the rootuv.lockstill records the previous Gym dependency metadata. The check currently fails during re-resolution on a missing flash-attention wheel hash. -
Megatron capture/recovery still lacks an end-to-end functional test.
-
The external dependencies remain open: Gym #2823 and Megatron-LM #7015.
Generated by Claude Code
…dapter TQMegatronTokenStager now hands the offloaded payload to RolloutTokenCapture.complete_call_from_response with Gym's MegatronCaptureAdapter instead of extracting fields by hand. Malformed payloads poison the call with capture_failed coordinates, which Gym records as worker_capture_failed (matching vLLM) rather than worker_response_missing_commit_coordinates. Requires the Gym-side adapter from NVIDIA-NeMo/Gym#2823. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
There was a problem hiding this comment.
Reminder that before merging, you will have to first merge the Gym side, bump the Gym commit hash, and re-generate uv.lock.
- add parent_chain_hash to the inline-prefix token_in admission - drop logical_request_id from _manifest_record (no such CallRecord field) Signed-off-by: Laura Dang <laurad@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
/ok to test eadf9e5 |
Signed-off-by: Laura Dang <laurad@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- reword _assemble_receipt docstring so the named failure reasons are examples, not an exhaustive list; add unresolved_parent and the capture_failed fallback for reason-less rows - terminal_selection is left unchanged: the pinned Gym RolloutReceipt (fc08bf19) types it Literal["declared","response_id","content", "heuristic"] with no None, so the invalid_manifest_row path cannot report "no heuristic ran" without a Gym-side schema change Signed-off-by: Laura Dang <laurad@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rics code Signed-off-by: Laura Dang <laurad@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…wiring Signed-off-by: Laura Dang <laurad@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
/ok to test 2c9baf1 |
…fload_params Megatron Inference renamed the opaque per-request dict it forwards to the payload stager and prompt preparer (NVIDIA/Megatron-LM PR #7015); follow it on the TQMegatronPromptPreparer / TQMegatronTokenStager side. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
…_prefix_tokens The Megatron chat endpoint now ships the chat-template render through the last assistant message (template_prefix_token_ids) and the EOS id in offload_params instead of a precomputed boundary (NVIDIA/Megatron-LM PR #7015). TQMegatronPromptPreparer feeds those, plus the prefix it resolved from the staging chain, to the same replace_prefix_tokens the vLLM worker uses, so one splicing algorithm serves both backends. replace_prefix_tokens gains a keyword-only eos_token_id override so callers that hold only token ids can use it without a tokenizer; the boundary failure message skips the detokenized reprs in that case. Existing callers are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
Signed-off-by: Laura Dang <laurad@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- bump Gym to 37dc751f (RolloutReceipt.terminal_selection is now Optional) - _assemble_receipt no longer pre-labels parse failures as heuristic - finalizer metric loop skips the None member of the Optional annotation Signed-off-by: Laura Dang <laurad@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
/ok to test 5a13a7c |
Signed-off-by: Laura Dang <laurad@nvidia.com>
Summary
CaptureAdmission, token-free lineage ledger, and the shared TQ staging contractOffloadedRequestPayloadthroughRolloutTokenCaptureandTQTokenSinkCallRecordonly after that acknowledgementRolloutReceiptat rollout end, then fetch, verify, and linearize staged records into the canonical GRPO sample during finalizationToken capture flow
The two serving backends use different prompt-preparation and generation hooks, but converge at the same durability boundary. A model call becomes eligible for lineage resolution only after its canonical token delta is staged in TQ. Capture failures write a failure row instead of a committed call record, causing finalization to reject or mask the rollout rather than train on incomplete lineage.
The source diagram is tracked in
docs/assets/token-capture-ledger-queue-data-flow.dot; the full invariants, terminal-selection rules, cleanup behavior, and fail-closed semantics are documented indocs/design-docs/token-capture-ledger.md.Dependencies
amahishi/partial-rollout-telemetry-v3OptionalRolloutReceipt.terminal_selection: NVIDIA-NeMo/Gym#2823 (submodule currently pinned to its head37dc751f; must land on Gymmainbefore this merges, then re-pin and regenerateuv.lock)RequestPayloadStager/RequestPromptPreparer, typedRequestPromptPreparationResult, andoffload_paramscarryingng_capture,template_prefix_token_ids,eos_token_id): NVIDIA/Megatron-LM#7015, pinned transitively through NVIDIA-NeMo/Megatron-Bridge#6101. Both dependencies must land before this PR merges, then the Bridge submodule must be re-pinned to its merged commit anduv.lockregenerated.Testing
Local test limitation
The RL lockfile supports Linux x86_64/aarch64 only, so the focused RL pytest files cannot run natively on this macOS host. An ephemeral dependency run progressed through PyTorch, Transformers, and pyzmq before reaching the NVIDIA-only
pynvmlruntime dependency; Linux CI provides the authoritative RL test coverage.