Skip to content

fix(mtp): mask logical sequence ends in padded CP rolls - #7278

Open
aroshanghias-nvd wants to merge 1 commit into
NVIDIA:mainfrom
aroshanghias-nvd:fix/packed-mtp-logical-ends
Open

aroshanghias-nvd wants to merge 1 commit into
NVIDIA:mainfrom
aroshanghias-nvd:fix/packed-mtp-logical-ends

Conversation

@aroshanghias-nvd

@aroshanghias-nvd aroshanghias-nvd commented Sep 12, 2026 •

Copy link
Copy Markdown
  • I, the PR author, have personally reviewed every line of this PR.

What does this PR do?

Complete the packed-MTP boundary correction from #6912 for CP>1. Padded physical boundaries locate each rank's chunks, but logical sequence lengths determine whether a shifted destination has a real next token. The CP path currently keeps padding-origin values and default all-ones mask entries alive past the logical end.

For seven logical tokens in eight physical slots, one left shift of an all-ones mask currently produces [1,1,1,1,1,1,1,0]; the valid result is [1,1,1,1,1,1,0,0]. Nonzero padding similarly leaks into the last logical target. This adds 13 lines to zero invalid destinations in each rank's two physical chunks, preserving the existing physical-indexing and CP1 corrections.

Related: #6912. This is a follow-up for logical-end validity, not a duplicate physical-boundary fix.

Validation

  • Added a regression covering CP1/2/4, padded/unpadded multiple sequences, tiny logical lengths, two tensor rows, float/bool masks, integer IDs, zero/nonzero padding, six successive shifts, sum enabled/disabled, and input/metadata immutability. Assertion outcomes are coordinated across ranks so a failing base cannot strand peers in communication.
  • Native CUDA/NCCL execution of the full imported module on four GB200 GPUs with Torch 2.11.0+cu130 and pytest 8.4.2: unchanged base 12 expected failures / 42 passes; exact patch 54 passes, on every rank.
  • Broader MTP selection: 94 passes (including those 54), 3 failures, 4 skips. The three embedding-scatter failures reproduce on the unchanged base: Torch LayerNorm rejects sequence parallelism during construction, before rolling. The four skipped HSM cases require eight ranks.
  • Additional native process_mtp_loss checks passed all four configurations: default masks, derived labels, five MTP depths, both calculate_per_token_loss settings, and TP2/CP2 or TP1/CP4, including CP groups [0,2] and [1,3]. Labels and auxiliary gradients matched an independent logical-sequence reference. These used synthetic output/loss callbacks, not a full-model training run.
  • Black 26.3.0, AST scope checks, and git diff --check pass.

Native validation used base f6c33bde4cdc10805ee7157db746d47b758f5f02. The PR was then based on main 4fe0daffe4fc45efc231efc7b5bfc3018d81d516; both changed files were identical between those bases, and their final bytes remain identical to the natively tested files. The full newer tree has not been rerun. The test harness bypassed only the unrelated autouse dataset-download fixture.

Patched module SHA256: a0749e517ae82c443271b329783fcdd4c0fac15df7a29235ea4fc750d4ab115c.
Test file SHA256: 5b43d53e0ee47ccb1f28d1313a98eef9e094962b38870c87b35fe402a119a1e1.

The additional per-sequence allocation/masking cost has not been benchmarked. This PR makes no training-quality or TMPE-remediation claim.

Review readiness

The current head is ee7b0088816963f2d712420413b1598cc318b892. A delegated code review covered every added line and the surrounding roll/caller contracts; no further source correction was identified. The exact source hashes, raw per-rank native XML, and caller receipts were rechecked. Targeted Black 26.3.0, AST parsing, git diff --check, and an independent integer coordinate audit passed again.

Repository CI has not run on this head: only DCO/DCO gate and reviewer-notification checks are successful. The runner-vetting bot requests additional validation. GitHub reports the SSH signature as unverified (unknown_key), and no upstream pull-request/7278 mirror was present at the readiness check. These observations do not establish the sole cause of missing CI. A trusted vetter can approve the exact head with /ok to test ee7b0088816963f2d712420413b1598cc318b892. Marking the PR ready begins review/CI routing; it does not certify passing CI or merge readiness.

Pre-checks

  • Added relevant unit tests.
  • Added full-model functional tests (not included; native caller checks described above).
  • Ran the full repository autoformatter (targeted Black and diff checks passed).

Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 12, 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.

@aroshanghias-nvd

Copy link
Copy Markdown
Author

@Phlip79 @Mellonta This is ready for technical review at ee7b0088816963f2d712420413b1598cc318b892. Every added line and the surrounding packed-roll/caller contracts have been reviewed; no further source changes were identified. Both changed files still match the exact natively tested bytes: 54/54 targeted cases passed on every rank, versus 12 failures on the unchanged base. Caller checks passed TP2/CP2 and TP1/CP4 with five MTP depths and both per-token loss settings. The existing three LayerNorm failures reproduce on the base; four larger-world tests were skipped.

Repository CI has not run on this head, and copy-pr-bot requests runner vetting. Could a trusted vetter approve the exact head with /ok to test ee7b0088816963f2d712420413b1598cc318b892? GitHub reports the existing SSH signature as unverified (unknown_key); the source commit does not need a rewrite to request explicit-SHA vetting. There is no full-model validation or performance benchmark, and this PR makes no training-quality or TMPE-remediation claim.

@Mellonta Mellonta left a comment

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.

@aroshanghias-nvd btw, where did you see this issue? is there a concrete use case?

khazic pushed a commit to khazic/Gym-lao that referenced this pull request Oct 15, 2026
## Summary
- Add the SA-V tracking verifier and its server configuration.
- Declare the SA-V environment servers used by video RL.
- Add portable five-row example data, schema, metrics, and checked
rollout artifacts.

## Why
This is the Gym-side verifier foundation for mixed SA-V tracking and
CapRL video training in NeMo-RL. No separate issue is needed; this is
part of the Super 3.5 video RL upstreaming stack.

## Validation
- Runtime evidence: the [healthy 16-node async `r18` W&B
run](https://wandb.ai/adlr/Nemotron-omni-RL-debug/runs/oci-hsg-cs-001_n3post_main_1d76d243b_vllm029_super35_video_step120_async16n_trainmb45056_expandable_uncapped_r18_20261003)
shows repeated GRPO cycles, stable TMPE, and increasing reward on the
equivalent video implementation.
- `gym dataset collate ... +mode=example_validation`: 5 examples;
generated `data/example_metrics.json`.
- Focused validation: 25 verifier tests and all 5 example rows against
`task_data.py` passed.
- Ruff format and lint passed for the changed Python files.
- Both Gym stack ranges pass `git diff --check`.

## Rollouts
`data/example_rollouts.jsonl` contains one checked rollout for each of
the five example rows. Representative task checks:

- `pt`:
`<answer>{"tracks":[{"point":1,"frame":1,"xy":[500,500],"visible":true}]}</answer>`
— `format_ok=true`, reward `1.0`, visible-frame mask membership `true`.
- `box`:
`<answer>{"tracks":[{"box":1,"frame":1,"bbox":[250,250,750,750],"visible":true}]}</answer>`
— `format_ok=true`, reward `1.0`, frame IoU `1.0`.
- `ref`:
`<answer>{"tracks":[{"id":7,"frame":1,"xy":[500,500]}]}</answer>` —
`format_ok=true`, reward `1.0`, predicted id `7` assigned to the GT
object and mask membership `true`.

The artifact also contains visible/invisible `pt` coverage and an
all-absent `ref` negative case, preserving the verifier's current
unmatched-ID behavior.

## Compatibility
Adds a new verifier/configuration and does not change unrelated
environments. Example media is embedded as a small inline JPEG, so no
employee or cluster-local paths are required.

## Stack
Gym PR 1 of 2. Consolidates and supersedes the implementation scope of
NVIDIA-NeMo#3504 after review history is preserved.

## Consolidated stack links

### Gym
1. [Gym NVIDIA-NeMo#3999 — SA-V tracking
verifier](NVIDIA-NeMo#3999)
2. [Gym NVIDIA-NeMo#4000 — R3 routing replay and rollout
identity](NVIDIA-NeMo#4000)

### NeMo-RL
1. [NeMo-RL NVIDIA-NeMo#4472 — Super video data and model
loading](NVIDIA-NeMo/RL#4472)
2. [NeMo-RL NVIDIA-NeMo#4473 — RL/MOPD video frame
contract](NVIDIA-NeMo/RL#4473)
3. [NeMo-RL NVIDIA-NeMo#4474 — sync/async Gym routing
runtime](NVIDIA-NeMo/RL#4474)
4. [NeMo-RL NVIDIA-NeMo#4475 — SA-V pin and CI
fixtures](NVIDIA-NeMo/RL#4475)
5. [NeMo-RL NVIDIA-NeMo#4476 — final dependency
pins](NVIDIA-NeMo/RL#4476)

### Child dependency refs
- [MCore integration
ref](https://github.com/ehosseiniasl/Megatron-LM/tree/ehsan/video-r3-mtp-gradient-integration-20261004),
adapting [MCore #7278](NVIDIA/Megatron-LM#7278)
and [MCore #7279](NVIDIA/Megatron-LM#7279) to
the pinned base.
- [MBridge integration
ref](https://github.com/NVIDIA-NeMo/Megatron-Bridge/tree/ehsan/video-r3-mcore-pin-integration-20261004).

---------

Signed-off-by: ehosseiniasl <ehsan.hosseiniasl@gmail.com>
Co-authored-by: Pulkit Kumar <pulkitk@nvidia.com>

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants