Skip to content

fix(training): correct diagnostic rank selection and repeated logs - #6211

Merged
cuichenx merged 2 commits into
mainfrom
chcui/codex/training-sync-logging-correctness
Sep 28, 2026
Merged

cuichenx merged 2 commits into
mainfrom
chcui/codex/training-sync-logging-correctness

Conversation

@cuichenx

Copy link
Copy Markdown
Contributor

An empty profile_ranks list currently disables allocator history and snapshots, and non-colocated MIMO can omit language-model setup diagnostics. This corrects logging rank selection and repeated external messages:

  • Treat an empty profile-rank list as all ranks for allocator history and snapshots, retaining the configured stop step and rank-specific filenames.
  • Include rank zero and the language model's first rank in MCore setup logging for both MIMO initialization paths; reset the selection for subsequent standard runs.
  • Emit repeated grouped-GEMM and absl informational messages only on rank zero, resolving rank when each message is emitted. Preserve warnings/errors and configured logger levels.

Syncs NVIDIA/Megatron-LM#6354, NVIDIA/Megatron-LM#7360, NVIDIA/Megatron-LM#5590, and NVIDIA/Megatron-LM#7414. Performance changes are outside this PR.

Validation: 246 focused logging, training utility, initialization, and MIMO provider tests passed in the NeMo 26.08.rc9 container against the pinned MCore. All-file pre-commit, Ruff checks, and format checks passed. Strict mypy remains blocked by missing optional dependency stubs and existing errors in these modules in the host tooling environment.

@cuichenx cuichenx added bug Something isn't working area:training Training loop, callbacks, and runtime integration needs-review PR is ready for code review and waiting on a reviewer mlm-sync Requires API/behavior sync with upstream Megatron-LM changes labels Sep 25, 2026
@claude

claude Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Light review — looks solid overall. The rank-selection fixes and the empty profile_ranks semantics are internally consistent (the memory-snapshot guards now match should_profile_rank in profiling.py, which already treats an empty list as all-ranks), and the new tests cover both branches well.

One documentation inaccuracy to flag:

  • [Medium] Stale profile_ranks docstring. src/megatron/bridge/training/config.py:758-761 (not touched by this PR) still reads: 'Memory-snapshot and recording-start guards use a strict membership check, so an empty list disables capture.' After this change an empty profile_ranks records memory history on ALL ranks (both start_memory_history_recording and training_log now short-circuit only when the list is non-empty). Please update the docstring so it matches the new behavior (empty => capture on every rank), otherwise the config docs state the opposite of what the code does.

Minor observations (no action needed):

  • _RankZeroInfoFilter is registered via addFilter, which is idempotent for the same instance, so repeated setup_logging() calls do not stack duplicates — the test confirms this.
  • build_infra reads language_grid.rank_offset only after a None check; the parametrized test covers offset 0/4 plus the parallelism_config is None fallback.

Suggested test cases:

  • test_empty_profile_ranks_records_on_every_rank (rank=0, rank=7) — added
  • test_profiling_memory_snapshot (profile_ranks=[7] and []) — added
  • test_build_infra_selects_language_representative_log_rank (offset=0, offset=4) — added
  • test_torch_dist_init_resets_default_logging_ranks (lazy=False, lazy=True) — added
  • test_external_info_logs_follow_current_rank_and_preserve_diagnostics (all six external loggers) — added
  • A test asserting training_log writes a memory snapshot on a non-profile_ranks rank when profile_ranks=[] (currently the empty-list path in training_log is only exercised at rank 0 via test_profiling_memory_snapshot)

No perf/recipe configs are touched — No perf tests impacted.

Signed-off-by: Chen Cui <chcui@nvidia.com>
@cuichenx
cuichenx merged commit d8fdaef into main Sep 28, 2026
90 checks passed
@cuichenx
cuichenx deleted the chcui/codex/training-sync-logging-correctness branch September 28, 2026 03:59

This branch was previously deployed

1 inactive deployment
test — 4f42669e Deployed Sep 27, 2026 by copy-pr-bot[bot] via cicd-wait-in-queue #22201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:training Training loop, callbacks, and runtime integration bug Something isn't working mlm-sync Requires API/behavior sync with upstream Megatron-LM changes needs-review PR is ready for code review and waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant