feat(model): enable Nemotron Omni audio, video, and packing - #5007
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
cuichenx
force-pushed
the
agent/nemotron-omni-audio-video
branch
from
July 27, 2026 23:56
92ad3db to
7fb0413
Compare
cuichenx
marked this pull request as ready for review
July 28, 2026 00:14
Contributor
|
Light review — PR #5007 (collator-owned sequence packing for Nemotron Omni) LGTM. This is a well-scoped refactor moving THD packing ownership from the model to the collator, and enabling audio + multi-frame video modalities plus CP/SP sharding after length-preserving media insertion. Test coverage is thorough (expanded collate contract, sound encoder, dense + packed CP sharding, HF/Energon parity, 2-rank distributed CP, in-batch padding-mask emission). Verified, no blocking issues:
Minor (non-blocking):
Suggested test cases
|
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Contributor
Author
|
/ok to test 5aa4034 |
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Contributor
Author
|
/ok to test 558e6b0 |
This reverts commit 558e6b0. Signed-off-by: Chen Cui <chcui@nvidia.com>
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Rebased onto
mainafter #4885 merged asf6c9292c2fe1d144fa577562d681ebf1824f9ddb.What changed
[B, T, H]encoder output in the legacy-compatible layouthas_soundas the sole sound-capability and construction flag: HF import derives it fromsound_config, CORD image-text SFT/PEFT set it false to omit the encoder/projector and roughly 1.13 GiB of replicated BF16 encoder weights per encoder-stage rank, and HF export writessound_config=Noneso sound-free checkpoints stay sound-freecollapse_image_tokens=Truetotal_tokensfor text/image/video/audio rowsmodel_owns_packingcapabilities to falseWhy
The canonical model uses one projected media feature per placeholder, so media insertion is length preserving. Packing before forward is therefore exact and avoids a second packing implementation inside the model. CP sharding remains after media insertion because all token-aligned tensors must use the same rank-local ordering.
For packed batches,
padding_maskmarks only synthetic CP/SP alignment gaps (Truemeans padding). It is separate fromloss_mask, which controls token supervision, and packed-sequence metadata, which controls attention and Mamba sequence boundaries. The collator creates this mask and the model keeps it aligned through CP/SP, but this PR deliberately does not forward it into the MCore language model yet: the pinned MCore expert-bias router crashes when it combines the flattened token mask with per-expert routing probabilities (MCore #6111). Until that upstream path is fixed, alignment gaps remain excluded from token loss but are knowingly counted by MoE router auxiliary losses and expert-bias statistics.The #4885 canonical provider also retained sound/video modules but hard-failed their insertion. This stack enables both modalities while keeping the legacy collapse/expand contract explicit.
Why
num_image_tileswas removed from canonical inference scriptsPR #4776 added
num_image_tilesto the historical LLaVA inference path because that model received compact image placeholders and expanded each placeholder insideforward(). Every PP stage needed the replacement counts to reproduce the merged sequence length, and generation needed that length to avoid sampling from a padded tail.The canonical model in this PR has the opposite contract: inference computes the same per-tile projected-feature counts with
inference_num_image_tiles, aggregates them back to each source-media wrapper, and expandsinput_idsto one placeholder per projected feature before model forward. Media insertion is therefore one-for-one and length preserving, the model validates placeholder/feature equality, PP exchanges the actual variable sequence shape, and generation samples atinput_ids.shape[1] - 1. Forwardingnum_image_tilesto the canonical model would be unused legacy model-owned expansion metadata. The explicit deprecatedNemotronOmniLlavaModelcompatibility path still retains and consumes it.This preserves the #4776 correctness invariant while moving ownership from model-forward expansion to canonical preprocessing. Dedicated regressions compare legacy merged widths against canonical pre-expanded widths for both dynamic multi-tile images and temporal tubelets.
Validation
Commands ran in the NeMo 26.06/26.08 development containers with real CUDA where applicable.
97 passed22 passed-uGHAvfqs2I, QA 9539, real clip + extracted audio11.724884990.0140 passeduv run pre-commit run --all-files: passedgit diff --check: passedFresh validation after rebasing onto main
On source/test commit
a3c8d076f3fd6992715c2c9e30f21e33ef986bba(card-only head7888d7aba2d04220d6bfc35fad7db4209e61d6ac):144 passeduv run pre-commit run --all-filesandgit diff --check: passedgit range-diff: all 13 patches preservedThe two-rank NCCL, official Valor replay, and three-step comparison remain historical evidence from source/test commit
7dff2df3e773adf346cd6018596909760679efcd; they were not rerun for this rebase.The verification card is at
examples/model_verification_cards/nemotron-3-nano-omni-30b-a3b-reasoning/card.yaml. It records this focused evidence without promoting any full end-to-end inventory item; the target-scale 30B result above remains historical evidence from the pre-restack series.Follow-ups
seq_aux_loss. This stack preserves attention/Mamba boundaries; exact per-row seq-aux semantics will use the boundary-aware Megatron-Core unflattening extension requested in MCore #6108.padding_maskpropagation after MCore #6111 is fixed and the Bridge MCore pin includes that fix.model_owns_packingdelegation path must move to data/collator-owned packing before downstream packed-RL validation.Follow-up hardening
FutureWarningnoticesAdditional validation on head
32905fdba: 152 focused tests passed in the NeMo 26.06 CUDA container, including an actual tiny packed multimodal forward/backward/AdamW parameter update. The final target-scale experiment used an equivalent disposable omission on prior headf69c73acf; commitbf6fb39f4makes that omission explicit with a regression and an upstream TODO.