Revert "[training, perf] fix: THD-aware FLOPS via cu_seqlens (Σᵢ sᵢ²) (#3839)" - #4363
Conversation
…#3839)" This reverts commit 7b1dc37 (PR #3839). #3839 was merged accidentally while its CI was still red: the `test_sft_example_runs_with_cp_and_packing` (CP=2) functional test hangs into an NCCL watchdog timeout (600s) on the GitHub Actions runners. The hang reproduces only on #3839 and is still under investigation. Reverting to restore main to a known-good state; #3839 will be re-opened with a fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chen Cui <chcui@nvidia.com>
|
Light Code Review -- Clean revert of PR 3839. The removed functions and their tests are consistently deleted across all call sites. The functional test test_seqpacking_cp_example is restored to its pre-3839 configuration. -- Observations -- VPP/dp_size ordering inconsistency for num_vision_patches (pre-existing, not introduced by this revert): train.py (line ~566) VPP-divides first then multiplies by dp_size, while train_utils.py (line ~535-541) multiplies by dp_size first then VPP-divides. With integer division these are not equivalent. Worth consolidating when 3839 is re-landed. -- .item() GPU-to-CPU sync in vlm_step.py (also pre-existing): the reverted grid.prod(dim=-1).sum().item() call forces a device sync per micro-batch. Another thing to address on re-land. -- No critical bugs or logic errors introduced by this revert. -- Suggested test cases -- No perf tests impacted. |
Light Code ReviewClean revert of 3839. The removed functions and their tests are consistently deleted across all call sites. The functional test test_seqpacking_cp_example is restored to its pre-3839 configuration. ObservationsVPP/dp_size ordering inconsistency for num_vision_patches (pre-existing, not introduced by this revert):
With integer division these are not equivalent (e.g. (3 // 2) * 4 = 4 vs (3 * 4) // 2 = 6). In practice VLM + VPP is rare and vision patch counts are typically divisible, so unlikely to bite today. Worth consolidating when 3839 is re-landed. .item() GPU-to-CPU sync in vlm_step.py (also pre-existing): the reverted grid.prod(dim=-1).sum().item() call in forward_step forces a device sync per micro-batch for VLM vision-patch counting. Another thing to address on re-land. No critical bugs or logic errors introduced by this revert. Suggested test casesNo perf tests impacted. |
…d of #3839) Re-applies #3839 (reverted in #4363 after an accidental merge) with one fix. #3839's last commit added `cfg.checkpoint.load = None` to the CP+packing functional test (test_sft_example_runs_with_cp_and_packing). With pretrained_checkpoint also None, finetune() then fails its precondition (finetune.py:50) with "Finetuning requires a loading from a pretrained checkpoint or resuming from a checkpoint". This drops that line, restoring the pre-#3839 behavior (inherit the recipe's default load) so the test runs. The `use_distributed_optimizer=False` setting added to that test is kept: it works around an NCCL watchdog hang seen only under the distributed optimizer + context parallelism in this test (root-cause tracked separately; the THD-FLOPS code itself is inert under CP>1, taking the BSHD fallback). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chen Cui <chcui@nvidia.com>
2 participants
Reverts #3839.
#3839 was merged accidentally while its CI was still red. The
test_sft_example_runs_with_cp_and_packing(CP=2, packed SQuAD) functional test inL0_Launch_traininghangs into an NCCL watchdog timeout (WorkNCCL(... OpType=ALLREDUCE, NumelIn=1 ...) Timeout(ms)=600000) after ~20 min. The hang reproduces only on #3839 across the recent CI window (every other branch passes the CP test) and is still under investigation — a faithful repro on cw-dfw (matching the CI mcore pinde6305c0) is pending.This revert restores
mainto a known-good state. #3839 will be re-opened with a fix once the CP-test hang is root-caused.🤖 Generated with Claude Code