Conversation
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
Light review — LGTM with one edge-case note. This adds Correctness (verified):
Observation (non-blocking): _build_packed_parquet_blend passes the single builder-managed pack_metadata_path (self.pack_metadata) to every source, and forwards pad_cu_seqlens=self._pad_cu_seqlens. If a user enables pad_cu_seqlens with a standalone blend, all sources read the same metadata file, which (a) will not exist for externally pre-packed sources, tripping the assert in GPTSFTPackedDataset, and (b) would apply one source's global max_seqlen to all sources even if it did exist. Consider validating that pad_cu_seqlens is incompatible with packed_train_data_blend, or deriving per-source metadata. Suggested test cases:
|
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test f651638 |
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
Addressed the packed-blend metadata edge case in |
|
/ok to test 542cd98 |
|
Light review - looks good overall. This adds weighted blending across multiple pre-packed Parquet SFT sources (packed_train_data_blend), with layered validation in PackedSequenceSpecs, GPTSFTDatasetConfig.validate(), and the new GPTSFTPackedParquetBlendDataset. The logic is sound and test coverage is solid. Verified:
Minor (non-blocking):
Suggested test cases:
No perf tests impacted. |
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
Addressed the latest review suggestions in |
@yaoyu-33, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 185947b |
|
CI attribution update: run |
|
/ok to test 227a2e7 |
|
/ok to test bb5e3ce |
Signed-off-by: Yu Yao <yaoyu.094@gmail.com>
|
/ok to test 1aa87db |
|
/ok to test 8f0cb6e |
|
Automatic Claude reviews have been retired. To request a pull-request review, post a comment containing: Add |
Summary
packed_train_data_blend=([sources], [weights])for offline-packed GPT SFT Parquet datamax_train_samplespad_cu_seqlensbecause each source requires its own packing metadataMotivation
A real NVIDIA Nemotron 3 Nano 30B-A3B SFT comparison on 8 H100s at sequence length 2048 and GBS 128 found bin/idx and Parquet statistically indistinguishable: indexed averaged 104.679 s per cold optimizer step and Parquet averaged 103.810 s. Since storage-format overhead is negligible at this model scale, weighted multi-source Parquet support is more useful than adding another packed storage format.
Tests
Validated on the current head in a Megatron Bridge Linux container:
python -m pytest -q tests/unit_tests/data/packing/test_parquet.py tests/unit_tests/data/builders/test_gpt_sft_config.py tests/unit_tests/data/builders/test_gpt_sft_builder.py— 101 passedpython -m pytest -q tests/unit_tests/training/test_config.py -k "packed or parquet or packing"— 36 passed, 243 deselectedpython -m pytest -q tests/unit_tests/doc_consistency/test_readme_consistency.py tests/unit_tests/tutorials/test_text_dataset_tutorials.py— 26 passedGPTSFTDatasetBuilder; 8 requested samples produced deterministic 6/2 source counts and collated successfullypre-commit run --all-files— passed