Repository navigation
fix(datasets): use len(lengths) for default modes in multimodal add_document - #5726
chtruong814 merged 6 commits into
Conversation
…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]>
|
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:
See the contribution guide for more details. |
|
/ok to test 8fd3c7f |
|
Odd apparently obvious bug, running tests. |
asolergi-nv
left a comment
There was a problem hiding this comment.
Looks good to me, but IIRC we are no longer supporting MM training with the indexed datasets.
|
/ok to test 8d26ffe |
|
/ok to test b7a3fea |
|
/ok to test 2116952 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31657611170 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31848571795 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31850078613 |
What
IndexedDatasetBuilder.add_documentdocumentsmodesas defaulting toNone. On a multimodal builder (multimodal=True), the default path built the per-item mode list as[0] * lengths:lengthsis aList[int], so[0] * lengthsmultiplies a list by a list and raisesTypeError: 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 documentedmodes=Nonedefault 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 thatadd_itemandsequence_lengthsalready follow.Test
Adds
test_multimodal_builder_add_document_default_modesintests/unit_tests/data/test_builder.py: a multimodal builder callingadd_documentwith the defaultmodesno longer crashes, assigns one default mode per item (matchingsequence_lengths), and still honors an explicitmodeslist. It fails before the fix (TypeError) and passes after.black(24) +isort+ruffare clean on the changed files. The full unit suite runs on GPU CI; I verified the fix and theadd_documentbody standalone off-GPU (the old expression raisesTypeError, the new one yields one0per item).