Add DATASET_COMBOS for grouped calibration datasets - #1508
Conversation
Lets a single --dataset token fan out to several entries in SUPPORTED_DATASET_CONFIG, with that entry's num_samples split evenly across the members. Initial combos: - ``default``: cnn_dailymail + nemotron-post-training-dataset-v2. hf_ptq.py now uses this when --dataset is omitted, replacing a hardcoded two-element list. --calib_size default bumped 512 -> 1024 so the per-dataset sample count after the even split matches the prior behavior. - ``nemotron-sft-mix``: the seven nvidia/Nemotron-* SFT datasets registered in #1498. get_supported_datasets() now lists combo names so they appear in --dataset --help. Expansion happens inside get_dataset_dataloader after list normalization. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a DATASET_COMBOS registry that expands single ChangesDataset Combos Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modelopt/torch/utils/dataset_utils.py (1)
198-212: ⚡ Quick winAdd import-time validation for combo integrity.
Please validate combo definitions early (non-empty members, no name collisions with
SUPPORTED_DATASET_CONFIG, and all members registered). Without this, typos can silently fall through to auto-detected HF loading and produce hard-to-debug behavior.🔧 Proposed guardrails
DATASET_COMBOS: dict[str, list[str]] = { @@ } + +_dataset_keys = set(SUPPORTED_DATASET_CONFIG) +_combo_keys = set(DATASET_COMBOS) +overlap = _dataset_keys & _combo_keys +if overlap: + raise ValueError(f"DATASET_COMBOS keys must not overlap dataset keys: {sorted(overlap)}") + +for combo_name, members in DATASET_COMBOS.items(): + if not members: + raise ValueError(f"DATASET_COMBOS['{combo_name}'] must contain at least one dataset.") + unknown = [m for m in members if m not in _dataset_keys] + if unknown: + raise ValueError( + f"DATASET_COMBOS['{combo_name}'] contains unknown datasets: {unknown}" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/torch/utils/dataset_utils.py` around lines 198 - 212, DATASET_COMBOS needs import-time validation to avoid silent typos and collisions: add a guard that runs when the module loads to (1) ensure each combo in DATASET_COMBOS has a non-empty list of members, (2) ensure no combo name conflicts with SUPPORTED_DATASET_CONFIG keys, and (3) ensure every member string in each combo is a registered dataset (exists in SUPPORTED_DATASET_CONFIG); raise a clear ValueError listing offending combo names/members if any checks fail so callers of get_dataset_dataloader receive immediate, actionable errors.examples/llm_ptq/hf_ptq.py (1)
536-537: ⚡ Quick winAvoid hardcoding combo members in the warning text.
This message can drift from the registry over time. Prefer referencing only the combo name (or deriving members from the registry) so runtime messaging stays accurate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/llm_ptq/hf_ptq.py` around lines 536 - 537, Replace the hardcoded combo-members text in the warning string ("No dataset specified. Defaulting to the 'default' combo (cnn_dailymail + nemotron-post-training-dataset-v2).") so it no longer lists combo members directly; either simplify the message to only reference the combo name (e.g., "No dataset specified. Defaulting to the 'default' combo.") or dynamically derive members from the dataset registry (e.g., lookup the 'default' combo via the registry API and interpolate its members) and use that value in the log call where the original string appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@examples/llm_ptq/hf_ptq.py`:
- Around line 536-537: Replace the hardcoded combo-members text in the warning
string ("No dataset specified. Defaulting to the 'default' combo (cnn_dailymail
+ nemotron-post-training-dataset-v2).") so it no longer lists combo members
directly; either simplify the message to only reference the combo name (e.g.,
"No dataset specified. Defaulting to the 'default' combo.") or dynamically
derive members from the dataset registry (e.g., lookup the 'default' combo via
the registry API and interpolate its members) and use that value in the log call
where the original string appears.
In `@modelopt/torch/utils/dataset_utils.py`:
- Around line 198-212: DATASET_COMBOS needs import-time validation to avoid
silent typos and collisions: add a guard that runs when the module loads to (1)
ensure each combo in DATASET_COMBOS has a non-empty list of members, (2) ensure
no combo name conflicts with SUPPORTED_DATASET_CONFIG keys, and (3) ensure every
member string in each combo is a registered dataset (exists in
SUPPORTED_DATASET_CONFIG); raise a clear ValueError listing offending combo
names/members if any checks fail so callers of get_dataset_dataloader receive
immediate, actionable errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4939a8b8-763d-4295-bcd5-ab868e512468
📒 Files selected for processing (3)
CHANGELOG.rstexamples/llm_ptq/hf_ptq.pymodelopt/torch/utils/dataset_utils.py
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1508 +/- ##
==========================================
- Coverage 76.93% 75.47% -1.47%
==========================================
Files 474 474
Lines 51506 53469 +1963
==========================================
+ Hits 39625 40354 +729
- Misses 11881 13115 +1234
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
End-to-end probing of the nemotron-sft-mix combo showed both ``interactive_agent`` and ``tool_calling`` fail under pyarrow's streaming JSON reader: the former at JSONL row 4 (``Column(.../member_id/type) changed from string to array``), the latter deterministically at sample ~885 (``Missing a closing quotation mark in string``). Both failures reproduce after a full HF cache wipe with ``force_redownload``, so they are content defects in the published JSONL — not local cache corruption. ``search``, which the previous comment incorrectly described as the skipped split, is the only one that streams cleanly (verified to 2500 samples). Switch the config to use only ``search`` and update the comment to describe the actual failure modes of the dropped splits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Small, cohesive feature (~40 lines). Logic looks correct: divmod-based even split with remainder distribution is fine, and get_supported_datasets() correctly surfaces combo names in --help. A few things worth a human look before merge:
-
No unit tests for the new combo expansion. The diff adds a non-trivial branch to
get_dataset_dataloader(combo-membership lookup, even split, remainder fan-out) with zero coverage. The "test plan" in the PR body is manual end-to-end runs. A simple unit test that mocksget_dataset_samplesand asserts the expandeddataset_name/num_sampleslists for a combo input (including the remainder case, e.g.n=10over 7 members →[2,2,2,1,1,1,1]) would be cheap and would also pin combo membership against accidental edits. The existingtests/unit/torch/utils/test_dataset_utils.py::TestGetDatasetDataloaderBlendingis the natural home. -
--calib_sizedefault change from512→1024is broader than the PR body suggests. The justification ("preserve previous total sample count for thedefaultcombo") only holds when the user is also using the new default-combo path. Anyone runninghf_ptq.py --dataset cnn_dailymail(or any single dataset) without--calib_sizenow gets 2× the calibration samples they got before — a silent runtime/cost regression for existing single-dataset workflows. Two cleaner options: (a) keep--calib_sizedefault at 512 and have thedefaultcombo not split (or absorb the doubling inside the combo), or (b) call out the doubling explicitly in CHANGELOG so users with pinned scripts notice. The CHANGELOG entry today only mentions the doubling in the context of the combo, not as a standalone default change. -
nemotron-sft-agentic-v2split swap is a piggybacked behavior change. Changingsplitfrom["interactive_agent", "tool_calling"]to["search"]silently alters what users of that single dataset key get, independent of combos. The PR body justifies it (data-quality bugs in the upstream JSONL files at the pinned revision) and the rationale is reasonable, but it's worth a maintainer eyeballing — and ideally a CHANGELOG line of its own, since it's a behavior change on a previously-released dataset entry from #1498. -
Minor: combos are only expanded inside
get_dataset_dataloader. If someone passes a combo name straight toget_dataset_samples(a public function listed in__all__and exposed viaget_supported_datasets()), it falls through to the "treat as HF path" branch and will fail confusingly. Either expand combos inget_dataset_samplestoo, or document that combos are dataloader-only.
None of these are blockers; flagging for owner sign-off because of the silent default change and the absence of tests on new logic.
Passing both a combo and one of its member datasets (e.g. ``--dataset cnn_dailymail,default``) silently double-samples the member — the combo expansion would take a per-member quota from cnn_dailymail in addition to the explicit entry. Raise ValueError in ``get_dataset_dataloader`` listing the combo and the overlapping member(s) so the user can drop one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
- Validate DATASET_COMBOS at import time: empty member lists, name collisions with SUPPORTED_DATASET_CONFIG, and references to unregistered datasets now raise ValueError up front so typos surface immediately rather than as confusing HF lookup failures. - Reject combo names in ``get_dataset_samples`` with a message pointing callers to ``get_dataset_dataloader``. Previously a combo name would fall through to the auto-detected HF path and fail opaquely. - Add five unit tests in ``TestDatasetCombosExpansion`` covering even split, divmod remainder distribution (n=10 over 7 members → [2,2,2,1,1,1,1]), plain + combo composition, overlap rejection, and the new ``get_dataset_samples`` combo-name guard. Tests mock ``get_dataset_samples`` so they don't touch the HF Hub. - Split the CHANGELOG entry: document the combo + member overlap and combo-in-``get_dataset_samples`` rejections, and add a separate bullet for the ``nemotron-sft-agentic-v2`` split swap so it isn't buried inside the combo entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/torch/utils/dataset_utils.py`:
- Around line 624-634: The combo-expansion logic (using DATASET_COMBOS and
building expanded_names/expanded_num_samples) currently appends members even
when their allocated sample count is zero, causing later calls to
get_dataset_samples to load unused datasets; change the distribution loop that
builds expanded_names/expanded_num_samples so it only appends a member when its
computed allocation (base + (1 if i < remainder else 0)) is > 0, and also add a
guard before invoking get_dataset_samples to skip entries where num_samples == 0
so no HF datasets are loaded for zero-allocated members.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48a8c88b-028b-475d-b8cd-9595e1077786
📒 Files selected for processing (3)
CHANGELOG.rstmodelopt/torch/utils/dataset_utils.pytests/unit/torch/utils/test_dataset_utils.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.rst
| for ds_name, n in zip(dataset_name, num_samples): | ||
| if ds_name in DATASET_COMBOS: | ||
| members = DATASET_COMBOS[ds_name] | ||
| base, remainder = divmod(n, len(members)) | ||
| for i, member in enumerate(members): | ||
| expanded_names.append(member) | ||
| expanded_num_samples.append(base + (1 if i < remainder else 0)) | ||
| else: | ||
| expanded_names.append(ds_name) | ||
| expanded_num_samples.append(n) | ||
| dataset_name, num_samples = expanded_names, expanded_num_samples |
There was a problem hiding this comment.
Skip zero-allocated combo members before loading datasets.
When a combo gets fewer samples than its member count, Lines 625-633 still enqueue members with 0 allocation, and Lines 637-640 still call get_dataset_samples for them. That triggers unnecessary HF loads and can fail calibration even though those members contribute no samples.
Proposed fix
expanded_names: list[str] = []
expanded_num_samples: list[int] = []
for ds_name, n in zip(dataset_name, num_samples):
+ if n <= 0:
+ continue
if ds_name in DATASET_COMBOS:
members = DATASET_COMBOS[ds_name]
base, remainder = divmod(n, len(members))
for i, member in enumerate(members):
- expanded_names.append(member)
- expanded_num_samples.append(base + (1 if i < remainder else 0))
+ alloc = base + (1 if i < remainder else 0)
+ if alloc <= 0:
+ continue
+ expanded_names.append(member)
+ expanded_num_samples.append(alloc)
else:
expanded_names.append(ds_name)
expanded_num_samples.append(n)
dataset_name, num_samples = expanded_names, expanded_num_samples
+
+ if not dataset_name:
+ raise ValueError("No positive sample allocations were produced from --dataset/--calib_size.")Also applies to: 637-640
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelopt/torch/utils/dataset_utils.py` around lines 624 - 634, The
combo-expansion logic (using DATASET_COMBOS and building
expanded_names/expanded_num_samples) currently appends members even when their
allocated sample count is zero, causing later calls to get_dataset_samples to
load unused datasets; change the distribution loop that builds
expanded_names/expanded_num_samples so it only appends a member when its
computed allocation (base + (1 if i < remainder else 0)) is > 0, and also add a
guard before invoking get_dataset_samples to skip entries where num_samples == 0
so no HF datasets are loaded for zero-allocated members.
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Re-review of PR #1508. Most previously-flagged critical items are addressed in the new diff:
- ✅ Unit tests for combo expansion: new
TestDatasetCombosExpansion(5 tests) covers even split, remainder distribution (10/7 → [2,2,2,1,1,1,1]), plain+combo composition, overlap rejection, and combo-name inget_dataset_samples. - ✅ Import-time validation of
DATASET_COMBOS(_validate_dataset_combos()checks empty members, key collisions, and unknown member references). - ✅ Combo names rejected from
get_dataset_sampleswith a pointer toget_dataset_dataloader. - ✅ Mixing a combo with a member dataset (
cnn_dailymail,default) raises a clearValueError. - ✅
nemotron-sft-agentic-v2split swap now has its own CHANGELOG bullet.
What still warrants a human eyeball before merge:
-
--calib_sizedefault 512 → 1024 still silently doubles single-dataset workflows. This was the most concrete concern in the previous review (meenchen item 2). The author's PR body justifies it as "--calib_sizenow denotes the total calibration budget regardless of combo cardinality," and the CHANGELOG mentions the bump — but only in the context of thedefaultcombo. A user runninghf_ptq.py --dataset cnn_dailymail(single dataset, no--calib_size) now silently gets 1024 samples where they previously got 512. That's a 2× cost/runtime regression on existing pinned scripts that is not explicitly called out as a standalone behavior change. Worth either (a) keeping the default at 512 and absorbing the doubling inside thedefaultcombo, or (b) adding an explicit CHANGELOG line that single-dataset users without--calib_sizewill now get 2× the samples. -
Minor / not blockers: the warning message in
hf_ptq.pyhardcodes the combo members in prose (will drift ifDATASET_COMBOS["default"]changes); zero-allocated combo members (e.g.nemotron-sft-mixwith--calib_size 3→ 4 members get allocation 0) still get passed toget_dataset_samples(name, 0), which triggers a pointless HFload_datasetcall per zero-member. Only a concern for callers passing very small calib sizes against the 7-member combo; the default 1024/7 → 146 each path is fine.
Deferring to owner to confirm the --calib_size default change is intended for single-dataset users too.
| raise ValueError( | ||
| f"--dataset includes both combo '{ds_name}' and its " | ||
| f"member(s) {sorted(overlap)}; remove one to avoid " | ||
| "double-sampling." | ||
| ) |
There was a problem hiding this comment.
| raise ValueError( | |
| f"--dataset includes both combo '{ds_name}' and its " | |
| f"member(s) {sorted(overlap)}; remove one to avoid " | |
| "double-sampling." | |
| ) | |
| raise ValueError( | |
| f"--{dataset_name} includes both combo '{ds_name}' and its " | |
| f"member(s) {sorted(overlap)}; remove one to avoid " | |
| "double-sampling." | |
| ) |
There was a problem hiding this comment.
Also remove --, seems to be referring to --dataset CLI arg, but meaningless inside this utils file.
Fridah-nv
left a comment
Fridah-nv
left a comment
There was a problem hiding this comment.
I notice that the current PR does not support per-member knob of num_samples, i.e. specify different number of samples for each dataset under the combo. This would be a useful feature for dataset blend study.
Is the scope of the PR only to add the calib dataset example for GLM5? Overall I haven't fully explored the nemotron v3 dataset blend, not sure if we want to recommend it as default.
Yes. If user want this, then they should specify them 1by1 instead of using the combo. |
The combo's seven member datasets are exactly the ones in NVIDIA's ``nemotron-post-training-v3`` HuggingFace collection (https://huggingface.co/collections/nvidia/nemotron-post-training-v3), so reuse the upstream name to make the relationship explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
| - DeepSeek PTQ (``examples/deepseek/ptq.py``) now defaults to native top-k calibration with post-hoc per-layer peer-max sync of expert ``input_quantizer.amax``; the all-experts path is preserved behind ``--calib_all_experts``. | ||
| - Add NVFP4 W4A16 weight-only quantization (``w4a16_nvfp4``): FP4 weights with group_size=16, BF16 activations, no calibration forward pass required. Use ``mtq.W4A16_NVFP4_CFG`` or ``--qformat w4a16_nvfp4`` in ``hf_ptq.py``. vLLM deployment support is in progress. | ||
| - Add ``DATASET_COMBOS`` to ``modelopt.torch.utils.dataset_utils`` — single ``--dataset`` tokens that fan out to multiple registered datasets; per-entry ``num_samples`` is split evenly across the members. Initial combos: ``default`` (``cnn_dailymail`` + ``nemotron-post-training-dataset-v2``, used by ``hf_ptq.py`` when no ``--dataset`` is provided) and ``nemotron-post-training-v3`` (the seven ``nvidia/Nemotron-*`` SFT datasets added in #1498, mirroring the `nemotron-post-training-v3 collection <https://huggingface.co/collections/nvidia/nemotron-post-training-v3>`_). Combo names are listed by ``get_supported_datasets()`` and surfaced in ``--dataset`` help. ``get_dataset_dataloader`` rejects inputs that mix a combo with one of its member datasets (e.g. ``cnn_dailymail,default``) to avoid double-sampling, and ``get_dataset_samples`` rejects combo names so callers route through the dataloader. ``hf_ptq.py`` default ``--calib_size`` is bumped from ``512`` to ``1024`` so the total calibration sample count under the new ``default`` combo matches the previous two-dataset fallback. | ||
| - The ``nemotron-sft-agentic-v2`` registered dataset (added in #1498) now uses only the ``search`` split. The previously configured ``interactive_agent`` and ``tool_calling`` splits contain content-level defects (heterogeneous schema and a malformed JSON row, respectively) that cause pyarrow's streaming JSON reader to fail deterministically. |
There was a problem hiding this comment.
nemotron-agentic-v1 also has tool_calling split which is correctly structured if you are interested to use it. Its also much larger than nemotron-sft-agentic-v2's tool_calling split
There was a problem hiding this comment.
maybe next run. RIght now we pick these 7 datasets for GLM5.1
| # Useful when callers want a single ``--dataset`` token that fans out to several | ||
| # entries; per-dataset ``num_samples`` is split evenly across the members. | ||
| DATASET_COMBOS: dict[str, list[str]] = { | ||
| "default": ["cnn_dailymail", "nemotron-post-training-dataset-v2"], |
There was a problem hiding this comment.
having default dataloader means if we later change default to something else, people will see regression in their ptq results. Is that okay? Or should we explicitly name it something else?
There was a problem hiding this comment.
I see what you mean. Let me actually rename it.
``default`` is too generic for a registry key — it doesn't say what the
combo contains and clashes with the common English sense ("the default
thing"). Rename it to ``cnn_nemotron_v2_mix`` so the combo's contents
(cnn_dailymail + nemotron-post-training-dataset-v2) are obvious from the
name, matching the descriptive style of the other combo
(``nemotron-post-training-v3``).
``hf_ptq.py``'s no-``--dataset`` fallback now uses the new key.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
## Summary - Add ``DATASET_COMBOS`` to ``modelopt.torch.utils.dataset_utils`` — single ``--dataset`` tokens that fan out to several entries in ``SUPPORTED_DATASET_CONFIG``. The per-entry ``num_samples`` is split evenly across the members inside ``get_dataset_dataloader``. - Two initial combos: - ``cnn_nemotron_v2_mix`` → ``cnn_dailymail`` + ``nemotron-post-training-dataset-v2``. Replaces the hardcoded two-element fallback list in ``hf_ptq.py`` when ``--dataset`` is omitted. - ``nemotron-post-training-v3`` → the seven ``nvidia/Nemotron-*`` SFT datasets registered in #1498 (mirroring the upstream [`nemotron-post-training-v3` collection](https://huggingface.co/collections/nvidia/nemotron-post-training-v3)). - ``get_supported_datasets()`` now appends combo names so they show up in ``--dataset`` help. - ``hf_ptq.py``'s default ``--calib_size`` bumped from ``512`` to ``1024`` so the ``cnn_nemotron_v2_mix`` combo's even split preserves the previous total sample count (was 512 per-dataset × 2 datasets = 1024; now 1024 split → 512 per-dataset × 2). ``--calib_size`` now denotes the total calibration budget regardless of combo cardinality. - Reject mixing a combo with one of its member datasets in the same ``--dataset`` list (e.g. ``cnn_dailymail,cnn_nemotron_v2_mix``) — combo would otherwise double-sample the explicit member with a smaller per-member quota. - Reject combo names in ``get_dataset_samples``; combos are dataloader-only. The error message points callers to ``get_dataset_dataloader``. - Validate ``DATASET_COMBOS`` at import time: empty member lists, name collisions with ``SUPPORTED_DATASET_CONFIG``, and references to unknown datasets raise ``ValueError`` up front. ## Test plan End-to-end validated against ``/hf-local/Qwen/Qwen3.5-0.8B`` via ``get_dataset_dataloader`` on the actual streamed data, plus 5 new unit tests in ``TestDatasetCombosExpansion`` (all 44 tests in ``test_dataset_utils.py`` pass with no regressions). - [x] ``python -c "from modelopt.torch.utils.dataset_utils import DATASET_COMBOS, get_supported_datasets; assert 'cnn_nemotron_v2_mix' in get_supported_datasets() and 'nemotron-post-training-v3' in get_supported_datasets()"`` - [x] ``hf_ptq.py`` with no ``--dataset`` flag still calibrates on cnn_dailymail + nemotron-post-training-dataset-v2 with the same total sample count as before. - [x] ``--dataset nemotron-post-training-v3 --calib_size 1024`` allocates 146 per member across the seven Nemotron datasets; full 1022-sample dataloader builds without error. - [x] ``--dataset cnn_dailymail,nemotron-post-training-v3 --calib_size 256,1024`` composes correctly: 256 from cnn_dailymail (as a plain entry) plus the 7-way split from the combo. (The earlier ``cnn_dailymail,cnn_nemotron_v2_mix`` example is rejected by design since ``cnn_dailymail`` is a member of that combo.) - [x] ``--dataset cnn_dailymail,cnn_nemotron_v2_mix`` raises ``ValueError`` with a clear message. - [x] ``get_dataset_samples("cnn_nemotron_v2_mix", ...)`` raises ``ValueError`` pointing to ``get_dataset_dataloader``. - [x] Unit tests: ``pytest tests/unit/torch/utils/test_dataset_utils.py`` — 44 passed. ## Post-validation fix End-to-end testing surfaced that the original ``nemotron-sft-agentic-v2`` entry kept the two splits (``interactive_agent``, ``tool_calling``) that pyarrow's streaming JSON reader cannot parse, and excluded ``search`` which is the only clean split. Failures reproduce deterministically across cache wipes with ``force_redownload``, so they are content-level defects in the published JSONL files at the pinned revision, not local artifacts: - ``interactive_agent`` — heterogeneous schema (``Column(.../member_id/type) changed from string to array``) at JSONL row 4. - ``tool_calling`` — malformed JSON row in a later shard, fails at sample ~885 with ``Missing a closing quotation mark in string``. - ``search`` — streams cleanly (verified to 2500 samples). Commit ``10f3cfd`` corrects ``nemotron-sft-agentic-v2`` to use only ``search``, with an updated comment. The CHANGELOG calls this out as a separate bullet so the behavior change on a previously-released dataset entry from #1498 is discoverable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Dataset combo support: a single dataset token can expand into multiple registered datasets with even sample splitting; predefined combos added (e.g., cnn_nemotron_v2_mix, nemotron-post-training-v3) and listed as supported. * **Updates** * Default dataset when none specified now uses cnn_nemotron_v2_mix. * Calibration size default increased from 512 to 1024. * **Bug Fixes** * nemotron-sft-agentic-v2 now uses only the deterministic "search" split to avoid streaming JSON errors. * **Tests** * Added coverage for combo expansion, splitting, overlap validation, and rejection behavior. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/Model-Optimizer/pull/1508?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chenjie Luo <chenjiel@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
](https://app.coderabbit.ai/change-stack/NVIDIA/Model-Optimizer/pull/1508?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
5 participants
Summary
DATASET_COMBOStomodelopt.torch.utils.dataset_utils— single--datasettokens that fan out to several entries inSUPPORTED_DATASET_CONFIG. The per-entrynum_samplesis split evenly across the members insideget_dataset_dataloader.cnn_nemotron_v2_mix→cnn_dailymail+nemotron-post-training-dataset-v2. Replaces the hardcoded two-element fallback list inhf_ptq.pywhen--datasetis omitted.nemotron-post-training-v3→ the sevennvidia/Nemotron-*SFT datasets registered in Add 7 nvidia/Nemotron-* calibration datasets to SUPPORTED_DATASET_CONFIG #1498 (mirroring the upstreamnemotron-post-training-v3collection).get_supported_datasets()now appends combo names so they show up in--datasethelp.hf_ptq.py's default--calib_sizebumped from512to1024so thecnn_nemotron_v2_mixcombo's even split preserves the previous total sample count (was 512 per-dataset × 2 datasets = 1024; now 1024 split → 512 per-dataset × 2).--calib_sizenow denotes the total calibration budget regardless of combo cardinality.--datasetlist (e.g.cnn_dailymail,cnn_nemotron_v2_mix) — combo would otherwise double-sample the explicit member with a smaller per-member quota.get_dataset_samples; combos are dataloader-only. The error message points callers toget_dataset_dataloader.DATASET_COMBOSat import time: empty member lists, name collisions withSUPPORTED_DATASET_CONFIG, and references to unknown datasets raiseValueErrorup front.Test plan
End-to-end validated against
/hf-local/Qwen/Qwen3.5-0.8Bviaget_dataset_dataloaderon the actual streamed data, plus 5 new unit tests inTestDatasetCombosExpansion(all 44 tests intest_dataset_utils.pypass with no regressions).python -c "from modelopt.torch.utils.dataset_utils import DATASET_COMBOS, get_supported_datasets; assert 'cnn_nemotron_v2_mix' in get_supported_datasets() and 'nemotron-post-training-v3' in get_supported_datasets()"hf_ptq.pywith no--datasetflag still calibrates on cnn_dailymail + nemotron-post-training-dataset-v2 with the same total sample count as before.--dataset nemotron-post-training-v3 --calib_size 1024allocates 146 per member across the seven Nemotron datasets; full 1022-sample dataloader builds without error.--dataset cnn_dailymail,nemotron-post-training-v3 --calib_size 256,1024composes correctly: 256 from cnn_dailymail (as a plain entry) plus the 7-way split from the combo. (The earliercnn_dailymail,cnn_nemotron_v2_mixexample is rejected by design sincecnn_dailymailis a member of that combo.)--dataset cnn_dailymail,cnn_nemotron_v2_mixraisesValueErrorwith a clear message.get_dataset_samples("cnn_nemotron_v2_mix", ...)raisesValueErrorpointing toget_dataset_dataloader.pytest tests/unit/torch/utils/test_dataset_utils.py— 44 passed.Post-validation fix
End-to-end testing surfaced that the original
nemotron-sft-agentic-v2entry kept the two splits (interactive_agent,tool_calling) that pyarrow's streaming JSON reader cannot parse, and excludedsearchwhich is the only clean split. Failures reproduce deterministically across cache wipes withforce_redownload, so they are content-level defects in the published JSONL files at the pinned revision, not local artifacts:interactive_agent— heterogeneous schema (Column(.../member_id/type) changed from string to array) at JSONL row 4.tool_calling— malformed JSON row in a later shard, fails at sample ~885 withMissing a closing quotation mark in string.search— streams cleanly (verified to 2500 samples).Commit
10f3cfdcorrectsnemotron-sft-agentic-v2to use onlysearch, with an updated comment. The CHANGELOG calls this out as a separate bullet so the behavior change on a previously-released dataset entry from #1498 is discoverable.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Updates
Bug Fixes
Tests