Repository navigation
Conversation
|
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. |
JF-D
marked this pull request as ready for review
October 23, 2026 06:27
Contributor
Author
|
/review mode=strict |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
Contributor
There was a problem hiding this comment.
LGTM. No actionable findings in the light review of all five changed files and their directly affected capture, prefetch, and gradient-reduction paths. Tests were inspected but not executed in this read-only environment.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
Open
JF-D
force-pushed
the
jiangfeid/moonep_fullcg_integration_pr
branch
from
October 26, 2026 20:57
ac54c56 to
52b93b1
Compare
JF-D
force-pushed
the
jiangfeid/moonep_fullcg_integration_pr
branch
from
November 2, 2026 20:53
b4e9571 to
796e583
Compare
Prepare virtual-expert forward pointer tables from warmed GTP storage before capture, without advancing prefetch. Run warmup on the shared capture stream so retained autograd state does not depend on an uncaptured stream. Test BF16 and MXFP8 eager/replay parity with virtual-expert transfers, persistent-ring sharing, repeated MTP, reset, and recapture. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
Select the reduction implementation from each bucket group configuration instead of changing the process-global default. Mixed DDP configurations must dispatch the matching implementation and retain its completion handle. Add a real-collective regression that constructs both configurations before reducing either. Without the fix, a model configured for native reduce-scatter can discard the FP32 accumulation handle and leave gradients unreduced. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
Use test-local GTP all-gather and reduce-scatter stream caches so gradient finalization does not join uncaptured streams retained by earlier test models. Keep the caches intact across warmup, capture, replay, reset, and recapture within the test. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
Register the mixed-configuration reduction regression in the kernel determinism manifest. Reset and reduce both models across three steps, checking output bytes against the exact expected gradient each time. This satisfies the CI determinism coverage gate while retaining a regression that fails with the original process-global dispatch. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
Reset process-global DDP reduce-scatter dispatch in the existing MoonEP test fixture and restore the previous state at teardown. Remove the per-bucket DDP dispatch change and its separate regression and manifest entry, keeping this PR focused on MoonEP full-iteration capture. Retain the shared capture-stream warmup required by training. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
Print the original exception before the result all-reduce, which can time out if peers are still executing model collectives. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
Scope the GTP cache to each lifecycle test and explicitly destroy its NCCL subgroups after model-parallel teardown. This prevents retained parameters and communicators from accumulating before later tests. Reuse the existing configuration helper, snapshot gradients outside capture, and reduce replay iterations while retaining BF16/MXFP8, MTP, and recapture coverage. Remove the duplicate ring-sharing assertion covered by GTP buffer tests. Validation: 22 tests passed on all four GPUs in Slurm job 1656052. Removing pointer-table preparation or capture-stream warmup reproduces the intended capture failure in both precisions. Black, isort, Pylint, and Ruff passed in job 1656309. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
Restore the upstream activation recompute test by removing the added stderr print and its comment. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
JF-D
force-pushed
the
jiangfeid/moonep_fullcg_integration_pr
branch
from
November 3, 2026 17:49
d923551 to
0e57bf5
Compare
fanshiqing
approved these changes
Nov 4, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fix MoonEP full-iteration CUDA graph capture after one warmup, including repeated MTP layers.
Main already contains the separate FC1/FC2 chains and GTP scratch-lifetime fixes (#8345, #7780, #8503). This PR adds no ring-reset mechanism.
The integration test compares eager loss and every parameter gradient with BF16/MXFP8 graph replays, with and without repeated MTP. It exercises virtual-expert transfers, shared persistent-ring slots, changing inputs, reset, and recapture. HSM is disabled for this fixed-input comparison; it does not test optimizer updates or convergence.
Issue tracking
Linked issue: Related to #7450, #8345, and #7780.
Code provenance
You should always make a best effort to avoid copying code from other
repositories. In exceptional cases in which this is unavoidable and you copied
or adapted code from an external source, provide the following details and
copy to the file header:
No external code was copied or adapted.
Contribution process
Pre-checks
jit_fuser/torch.compile, CUDA extension, TE or external-library dispatch, or a scatter/index accumulation), I have added or updated its bit-exact determinism test and registered it intests/unit_tests/determinism/kernels/manifest.py(guide)