Skip to content

feat(conversion): add a failure exit mode to HF/Megatron comparison - #6312

Open
yuchenwang3 wants to merge 3 commits into
NVIDIA-NeMo:mainfrom
yuchenwang3:fix/compare-failure-exit
Open

yuchenwang3 wants to merge 3 commits into
NVIDIA-NeMo:mainfrom
yuchenwang3:fix/compare-failure-exit

Conversation

@yuchenwang3

Copy link
Copy Markdown

What does this PR do?

Add --fail-on-mismatch to the HF/Megatron forward comparison. Today a different next token or cosine similarity below 0.99 is printed, but the command still exits successfully. The new flag makes those cases fail the command, so a validation job can use its exit status.

Changelog

  • Keep the existing diagnostic behavior when the flag is omitted.
  • Require matching next tokens within the HF vocabulary and cosine similarity >= 0.99. Absolute logit differences remain diagnostic metrics.
  • Reduce the verdict across all ranks before raising, including non-last pipeline stages, and destroy the process group on failure.
  • Document the flag and cover mismatched tokens, low cosine similarity, NaN logits, padded vocabulary entries, and failure propagation from either rank.

Validation

  • 38 targeted CPU tests passed on macOS, Python 3.12.13 and PyTorch 2.14.1. Model loads/forwards are mocked in the CPU tests; verdict propagation uses a real two-process Gloo group.
  • Unpatched main fails the three regression cases that expect an error for token mismatch, low correlation, or NaN logits.
  • All repository pre-commit hooks passed.
  • Strict mypy reports the same 39 existing diagnostics on main and this patch, with no new diagnostic.
  • CUDA/NCCL and full-model inference were not run for this patch.

Local test command (isolates the CPU test file from GPU-dependent conftest imports):

uv run --no-project --python 3.12 --with torch --with pytest --with rich --with pre-commit --with mypy \
  python -m pytest --noconftest tests/unit_tests/test_compare_mask_handling.py -q

GitHub Actions CI

External contributor; NVIDIA runner approval is needed for CI.

Before your PR is ready for review

  • Read the contributor guidelines.
  • Added necessary tests.
  • Updated the example documentation.
  • Changes optional components (not applicable).

Additional Information

Implementation and tests were written with OpenAI Codex.

@copy-pr-bot

copy-pr-bot Bot commented Oct 4, 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.

@cuichenx cuichenx added feature New capabilities, enhancements, or enablement work area:ckpt Checkpoint conversion, loading, export, and save paths needs-review PR is ready for code review and waiting on a reviewer labels Oct 4, 2026
@yuchenwang3

Copy link
Copy Markdown
Author

@cuichenx Could you review the opt-in --fail-on-mismatch mode and authorize CI for 2153756f2? All 38 CPU tests pass, including two-rank Gloo failure propagation. CUDA/NCCL still needs CI.

cuichenx
cuichenx previously approved these changes Oct 9, 2026
@cuichenx

cuichenx commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

/ok to test 2153756

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Automatic Claude reviews have been retired. To request a pull-request review, post a comment containing:

/review

Add model=claude to use a Claude reviewer (the default is model=codex). mode=light|strict selects the review depth; for example, /review model=claude mode=strict. Comment /review help for all options.

@cuichenx cuichenx removed the needs-review PR is ready for code review and waiting on a reviewer label Oct 9, 2026
Assisted-by: OpenAI Codex
Signed-off-by: Yuchen Wang <93700456+yuchenwang3@users.noreply.github.com>
Assisted-by: OpenAI Codex
Signed-off-by: Yuchen Wang <93700456+yuchenwang3@users.noreply.github.com>
@yuchenwang3
yuchenwang3 force-pushed the fix/compare-failure-exit branch from 7ebb0ad to bb4937a Compare October 9, 2026 21:59
@cuichenx cuichenx added the needs-review PR is ready for code review and waiting on a reviewer label Oct 9, 2026
cuichenx
cuichenx previously approved these changes Oct 10, 2026
@cuichenx

Copy link
Copy Markdown
Contributor

/ok to test bb4937a

@cuichenx cuichenx removed the needs-review PR is ready for code review and waiting on a reviewer label Oct 10, 2026
Mock scheduler selection instead of bypassing the Megatron forward helper.
Drive the real inference wrapper, iterator, and forward-step callback with
a single-microbatch test scheduler, then assert the scheduling and model
input contracts. Preserve the existing offline AutoConfig fixture.

Assisted-by: OpenAI Codex
Signed-off-by: Yuchen Wang <eang333cms@gmail.com>
@yuchenwang3
yuchenwang3 force-pushed the fix/compare-failure-exit branch from 1bd9164 to 12a6ab7 Compare October 12, 2026 18:25
@cuichenx cuichenx added the needs-review PR is ready for code review and waiting on a reviewer label Oct 12, 2026

This branch was successfully deployed

1 active (outdated) deployment
test — bb4937a6 Deployed Oct 10, 2026 by copy-pr-bot[bot] via cicd-wait-in-queue #22744
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ckpt Checkpoint conversion, loading, export, and save paths community-request feature New capabilities, enhancements, or enablement work 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.

2 participants