Repository navigation
Conversation
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
|
/review-pr-team |
Signed-off-by: seonjinn <sna@nvidia.com>
seonjinn
left a comment
There was a problem hiding this comment.
Self-review before requesting reviewers.
Method: six review agents over the PR8a-only diff (8 files vs the #3703+#3706 base), then an adversarial pass that re-verified citations, ran two empirical checks on CPU torch, and re-checked CI/PR state live.
The core math held up well under adversarial verification: the multi-pass staircase visibility and the RoPE-relative shift alignment in _merge_branch were re-derived against an independently written rotate-half oracle and match to machine precision (4.4e-16, fp64); label alignment (teacher_offset = pass_index + 1) was re-worked by hand; the cross-pass detach asymmetry is pinned by an explicit test; and the TP/SP integration follows the clean pattern -- EagleTTTCoreAttention owns zero parameters, so the sharded-checkpoint bug classes from earlier PRs in this series are structurally inapplicable. The SP test (real NCCL SelfAttention with sequence_parallel=True, output + input grads + parameter grads vs a global dense oracle) is the strongest distributed test in the series so far.
The one substantive finding is the activation-recompute incompatibility (inline, empirically reproduced): with recompute_granularity=full or selective, backward re-enters the armed forward and crashes on step 1.
Process, before merge:
- No
/ok to testhas ever been issued on this PR -- zero substantive CI on any commit. Needs a fresh/ok to test 292e29d31a6276c0e4823c37d94f7c6e917f2fc1(after the rebase below, on the new head). - Rebase required: both stacked dependencies have diverged (not just advanced) -- #3706's head (
40badeea) and #3703's head (5133c28d) no longer contain this PR's base660ee3c6, so the rebase may not be clean. - Body accuracy: the Ruff/pyrefly/4xGB200 numbers are self-reported (no check-runs exist); "139 passed" spans the full 13-file stacked suite, not this PR's 5 test files (~91 collected) -- worth stating the basis.
- Below inline threshold:
MCoreEagleTTTSessionis never driven pastpass_count=2off-GPU, and the "pinned_modelopt" test name pins an internal formula, not an external ModelOpt oracle -- consider a rename or a small modelopt-driven geometry diff test later.
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
…erge Signed-off-by: seonjinn <sna@nvidia.com>
The FP32 vocab tile size was a constructor default that no config path could reach, even though it is what bounds the activation footprint this PR exists to cap. Thread it from policy.draft.token_chunk_size through DraftLossWrapper into DraftCrossEntropyLossFn, declare it on DraftCrossEntropyLossConfig and DraftConfig, and document the default in the exemplar config. Also drop the unused ctx.tp_group assignment: backward rebuilds both distributions from the already TP-reduced log-normalizers and never communicates. Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
…erge Signed-off-by: seonjinn <sna@nvidia.com> # Conflicts: # pyrefly.toml
Signed-off-by: seonjinn <sna@nvidia.com>
…erge Signed-off-by: seonjinn <sna@nvidia.com> # Conflicts: # nemo_rl/models/megatron/train.py # nemo_rl/models/policy/workers/megatron_policy_worker.py
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com> # Conflicts: # nemo_rl/algorithms/loss/draft.py # nemo_rl/algorithms/loss/loss_functions.py # nemo_rl/models/megatron/train.py # tests/unit/algorithms/test_draft_loss_wrapper.py # tests/unit/algorithms/test_draft_soft_ce.py
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
|
/ok to test 95e639b |
Signed-off-by: seonjinn <sna@nvidia.com>
…-main-merge Signed-off-by: seonjinn <sna@nvidia.com> # Conflicts: # nemo_rl/models/policy/__init__.py # pyrefly.toml
|
/ok to test be80ab6 |
|
Merged latest
1. 2. 3. self.cfg["draft"].get("token_chunk_size", DEFAULT_DRAFT_TOKEN_CHUNK_SIZE)which git merged clean, passes lint, and raises Known gap, deliberately not fixed here. |
Summary
Scope
Covers the provider, pass state, attention, loss, and split-step training primitives. Higher-level configuration and recipes remain out of scope.
Validation
fdf2a971be0b396ed7282a9a99c79a6af24a5e7bPrior GPU evidence
Dependencies / Remaining
Depends on #3703 and #3706. CUDA memory cases remain GPU-only validation.