Skip to content

Add mock SFT dataset and varlen dataset family - #6734

Merged
ilml merged 11 commits into
NVIDIA:mainfrom
ilml:split/3386-dataset-r2
Aug 27, 2026
Merged

Add mock SFT dataset and varlen dataset family#6734
ilml merged 11 commits into
NVIDIA:mainfrom
ilml:split/3386-dataset-r2

Conversation

@ilml

@ilml ilml commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds the mock SFT dataset (with a generalized SFT padding divisor) and the varlen dataset family (VarlenLowLevelDataset, VarlenDataset, MockVarlenDataset) with unit tests. The datasets are inert until the follow-up training-integration PR wires them into pretraining.

Part of the #3386 split series tracked in the Dynamic Context Parallelism project. Original changes by @xiaoyao0115 in #3386.

Note: Replaces #6685, which was auto-closed when its base mirror ref was deleted on #6684's merge. All of @jaredcasper's review comments on #6685 are addressed here (and canonically in #5902): sequence-packing field checks moved into ModelParallelConfig.__post_init__, argument formatting matched to the file's style with no reformat of untouched lines.

Series status

Remaining PRs of the #3386 split, tracked in the Dynamic Context Parallelism project. All PRs target main — no stacked bases. Reviews run in parallel; merges go in order:

Order PR Content
1 #5902 config field, CLI arg, validation
2 this PR (#6734) mock SFT + varlen datasets
3 #6742 training-loop + varlen wiring (pretrain_gpt + pretrain_hybrid)

#6734 carries a temporary copy of #5902's commit, and #6742 carries #6734's commits (review its last three commits) — the duplicates drop out automatically as parents merge and the branches are rebased.

Merged so far: #5901, #5903, #6625, #5908, #6684, #5902.

Contribution process

  • Draft PR per contributing guidelines
  • Commits signed off (DCO)

ilml and others added 3 commits August 21, 2026 03:03
Split 2/10 from NVIDIA#3386 (sequence packing / THD E2E support). Adds the
sequence_packing_scheduler knob to ModelParallelConfig, its validation
(field checks in ModelParallelConfig post_init; TE>=2.9 pin and
alltoall dispatcher in TransformerConfig), the explicit CLI args, and validate_args checks.
Feature-flagged no-op until the scheduler lands.

Original changes by @xiaoyao0115 in NVIDIA#3386.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ilml <tolong@nvidia.com>
Split 6/10 from NVIDIA#3386 (sequence packing / THD E2E support). Adds
MockSFTDataset/MockSFTLowLevelDataset with file- and distribution-mode
sequence-length configs, the load_json_arg helper, the
--sft-mock-dataset-config-json arg with lognormal default, and
generalizes the SFT padding divisor beyond cp>1.

Original changes by @xiaoyao0115 in NVIDIA#3386.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ilml <tolong@nvidia.com>
Split 9/10 from NVIDIA#3386 (sequence packing / THD E2E support). Adds the
variable-length packed (THD) dataset family: HF-hub/parquet/jsonl
loading, THD __getitem__ with cu_seqlens, SBHD validation mode, and the
mock variant, with unit tests.

Also adds hybrid_context_parallel=False to the _make_config test helper
(deviation from NVIDIA#3386: fixes a latent AttributeError in
_calculate_padding_divisor with SimpleNamespace configs).

Original changes by @xiaoyao0115 in NVIDIA#3386.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ilml <tolong@nvidia.com>
@ilml

ilml commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test d9fdfa6

@copy-pr-bot

copy-pr-bot Bot commented Aug 21, 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.

@ilml

ilml commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 946476b

The module-level import broke the minimal-install import check: pandas
is deliberately only in the ci dependency group, not a core dependency.
Guard the import inside the mode == "file" branch with a targeted
ImportError, matching the existing pattern in varlen_dataset.py.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ilml <tolong@nvidia.com>

@janEbert janEbert 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.

It seems like Pandas is just used to load CSV/JSON. In the case of JSON, I think you can load from JSON directly with datasets.load_dataset.
For CSV, I'd also say just use the csv package from stdlib, unless it's too slow compared to using Pandas.

Then we avoid introducing the additional dependency, since Pandas isn't used anywhere else yet (except CI).

@ilml

ilml commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 4ccffde

Per review: pandas was only used to read the mock sequence-length CSV
and local jsonl files, and is not a package dependency (it is only in
the ci group).

- MockSFTLowLevelDataset(mode="file") reads the CSV with the stdlib csv
  module, skipping non-numeric cells such as a header row.
- VarlenLowLevelDataset parses local jsonl with the stdlib json module
  and builds the table with Dataset.from_list. This keeps the property
  the pandas path existed for: one pyarrow pass over the whole file, so
  rows with differing field sets unify into one schema instead of
  failing datasets.load_dataset("json")'s per-chunk inference with a
  CastError (e.g. LongAlpaca-12k).
- Remove the now-stale pytest.importorskip("pandas") gates so the tests
  also run in environments without pandas.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ilml <tolong@nvidia.com>
@ilml

ilml commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 41de47f

The pre-existing allgather-vs-variable_seq_lengths ValueError ran for
dense models too, where the (unused) dispatcher default is allgather.
Sequence packing derives variable_seq_lengths=True, so every dense
packing config failed validation. Gate the check on num_moe_experts,
matching the packing-specific dispatcher assert below, and update the
MoE test to expect the general check's ValueError, which fires before
the packing assert for allgather.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ilml <tolong@nvidia.com>
@ilml

ilml commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 0b38f86

@ilml
ilml enabled auto-merge August 27, 2026 05:17
@ilml

ilml commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 300c9c45b4ebf69d4e1a9dc8fe69449a82e94e7c

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the Approved All necessary approvals have been made label Aug 27, 2026
# Conflicts:
#	megatron/training/arguments.py
@ilml
ilml added this pull request to the merge queue Aug 27, 2026
@nemo-automation-bot

Copy link
Copy Markdown

🔄 Merge queue validation started!

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

Merged via the queue into NVIDIA:main with commit d2931db Aug 27, 2026
92 checks passed
@ilml
ilml deleted the split/3386-dataset-r2 branch August 27, 2026 12:32
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 complexity: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants