Skip to content

fix(datasets): use len(lengths) for default modes in multimodal add_document - #5726

Merged
chtruong814 merged 6 commits into
NVIDIA:mainfrom
winklemad:fix/multimodal-add-document-default-modes
Aug 15, 2026
Merged

chtruong814 merged 6 commits into
NVIDIA:mainfrom
winklemad:fix/multimodal-add-document-default-modes

Conversation

@winklemad

Copy link
Copy Markdown
Contributor

What

IndexedDatasetBuilder.add_document documents modes as defaulting to None. On a multimodal builder (multimodal=True), the default path built the per-item mode list as [0] * lengths:

if self.multimodal:
    self.sequence_modes.extend(modes if modes is not None else [0] * lengths)

lengths is a List[int], so [0] * lengths multiplies a list by a list and raises TypeError: can't multiply sequence by non-int of type 'list' — it never produces the intended default mode list. Any multimodal dataset build that relies on the documented modes=None default hits this.

No linked issue — found by reading the code.

Fix

Use [0] * len(lengths), so the default assigns one mode (0) per item — matching the one-mode-per-item contract that add_item and sequence_lengths already follow.

Test

Adds test_multimodal_builder_add_document_default_modes in tests/unit_tests/data/test_builder.py: a multimodal builder calling add_document with the default modes no longer crashes, assigns one default mode per item (matching sequence_lengths), and still honors an explicit modes list. It fails before the fix (TypeError) and passes after.

black (24) + isort + ruff are clean on the changed files. The full unit suite runs on GPU CI; I verified the fix and the add_document body standalone off-GPU (the old expression raises TypeError, the new one yields one 0 per item).

…ocument

IndexedDatasetBuilder.add_document defaults modes to None, and on a multimodal
builder it then built the per-item mode list as [0] * lengths. lengths is a
List[int], so [0] * lengths multiplies a list by a list and raises TypeError
instead of producing one default mode per item. Use [0] * len(lengths), which
matches the one-mode-per-item contract that add_item and sequence_lengths
already follow.

Signed-off-by: winklemad <[email protected]>
@winklemad
winklemad requested review from a team as code owners July 9, 2026 16:07
@copy-pr-bot

copy-pr-bot Bot commented Jul 9, 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.

@svcnvidia-nemo-ci
svcnvidia-nemo-ci marked this pull request as draft July 9, 2026 16:07
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

This PR has been automatically converted to draft because all PRs must start as drafts.

When you are ready for review, click Ready for Review to begin the review process. This will:

  1. Add the oncall reviewer (optional reviewer)
  2. Add required review teams based on your changes

See the contribution guide for more details.

@winklemad
winklemad marked this pull request as ready for review July 9, 2026 16:33
@svcnvidia-nemo-ci
svcnvidia-nemo-ci requested a review from a team July 9, 2026 16:33
@cspades

cspades commented Jul 10, 2026

Copy link
Copy Markdown
Member

/ok to test 8fd3c7f

@cspades

cspades commented Jul 10, 2026

Copy link
Copy Markdown
Member

Odd apparently obvious bug, running tests.

@asolergi-nv asolergi-nv 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.

Looks good to me, but IIRC we are no longer supporting MM training with the indexed datasets.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-maintainers Waiting on maintainers to respond waiting-on-customer Waiting on the original author to respond labels Jul 21, 2026
@guihong-nv

Copy link
Copy Markdown
Contributor

/ok to test 8d26ffe

@ilml

ilml commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

/ok to test b7a3fea

@Phlip79

Phlip79 commented Aug 5, 2026

Copy link
Copy Markdown
Member

/ok to test 2116952

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31657611170

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 13, 2026
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31848571795

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31850078613

This branch was previously deployed

2 inactive deployments
public — 2116952f Deployed Aug 14, 2026 by copy-pr-bot[bot] via finalize / publish-docs #6094
test — 2116952f Deployed Aug 14, 2026 by copy-pr-bot[bot] via cicd-wait-in-queue #18996
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved All necessary approvals have been made community-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants