Skip to content

[build] fix: Restore Bridge compatibility with MCore dev - #4094

Closed
yaoyu-33 wants to merge 1 commit into
mainfrom
yuya/mcore-dev-compat-20260531
Closed

[build] fix: Restore Bridge compatibility with MCore dev#4094
yaoyu-33 wants to merge 1 commit into
mainfrom
yuya/mcore-dev-compat-20260531

Conversation

@yaoyu-33

Copy link
Copy Markdown
Contributor
Summary

What

  • Move Bridge off MCore helper surfaces removed on MCore dev.
  • Own local rank/world-size/SLURM helpers, vocab padding, logging helpers, model builder base types, and unimodal wrapping helpers.
  • Update unit-test patch paths to Bridge-owned helper modules.

Why

PR #4093 bumps MCore dev to 1fe78252e9690505a04759f56b6cbb35861fcb37 and fails during import/collection because MCore removed or flattened:

  • megatron.core._slurm_utils
  • safe_get_world_size from megatron.core._rank_utils
  • megatron.training.vocab_utils
  • megatron.training.utils.log_utils
  • megatron.training.models.base / dist_utils
  • megatron.training.utils.common_utils.get_local_rank_preinit

Validation

  • ruff format on changed files
  • ruff check on changed files
  • pre-commit run --files on changed files
  • git diff --check
  • Source-loader import check against current MCore submodule (378d81fbd32a3faa90726d0e59344f7893ce4b47)
  • Source-loader import check against the MCore dev target from chore(beep boop 🤖): Bump uv.lock (main, mcore-dev) (2026-05-31) #4093 (1fe78252e9690505a04759f56b6cbb35861fcb37)
  • Focused pytest on this host: blocked by local environment. uv run cannot install locked nvidia-resiliency-ext==0.6.0 for this host's platform, and the active Python environment has an older NVRX package than MCore requires.

Unblocks the Bridge-side compatibility failures observed in #4093.

Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented May 31, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@yaoyu-33 yaoyu-33 added area:build Dependencies, packaging, images, and environment setup area:misc Cross-cutting utilities, logging, helpers, and other changes bug Something isn't working full-test-suite mlm-sync Requires API/behavior sync with upstream Megatron-LM changes needs-more-tests Requires additional L0 and L1 test coverage before merge needs-review PR is ready for code review and waiting on a reviewer labels May 31, 2026
@yaoyu-33

Copy link
Copy Markdown
Contributor Author

/ok to test 47da44e

Comment on lines +122 to +130
print(
" > number of parameters on (tensor, pipeline) model parallel rank ({}, {}): {}".format(
pg_collection.tp.rank(),
pg_collection.pp.rank(),
sum([sum([p.nelement() for p in model_module.parameters()]) for model_module in model]),
),
flush=True,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: bare print() is discouraged by project rules (CLAUDE.md: "NEVER use bare print() — use logging.getLogger(__name__) or print_rank_0()"). The DP/CP rank guard makes this intentional (matching the MCore original), but since we're inlining anyway, consider switching to print_rank_0 or the module logger to stay consistent with Bridge conventions.

@claude

claude Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Light Code Review

Overall this is a clean inlining of MCore helpers that are being removed upstream. The imports are fully migrated, test patch paths are updated, and no stale references remain.

Minor observations

  1. Bare print() in _print_num_params (unimodal.py:122): Project rules prohibit bare print(). The DP/CP rank-0 guard replicates the MCore original, but since this is now Bridge-owned code, it could use print_rank_0 or the module logger. Left an inline comment.

  2. Redundant env-var check in resolve_slurm_world_size (slurm_utils.py): is_slurm_job() already checks SLURM_NTASKS in os.environ, so the second check is always True when reached. Same pattern in resolve_slurm_local_rank. Not a bug, just defensive duplication.

  3. No new tests for append_to_progress_log or barrier_and_log: These were previously tested via MCore; now they live in Bridge but test_log_utils.py does not cover them. The rest of the inlined surface has good existing coverage.

No critical bugs or logic errors found.

Suggested test cases

No perf tests impacted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:build Dependencies, packaging, images, and environment setup area:misc Cross-cutting utilities, logging, helpers, and other changes bug Something isn't working full-test-suite mlm-sync Requires API/behavior sync with upstream Megatron-LM changes needs-more-tests Requires additional L0 and L1 test coverage before merge 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