Repository navigation
Address post-merge review of the chat JSONL blend and the Super VL QAD tutorial - #2863
yueshen2016 wants to merge 4 commits into
Conversation
A row that decodes but cannot be converted (e.g. a string messages field) is skipped with a warning and counted as rejected, as the tokenizing reader skips malformed records; the give-up rule still bounds an unusable source. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yue <yueshen@nvidia.com>
…D tutorial - Drop chat rows whose text field is not a string. - Rename the NemotronH omni QAD tutorial to the released checkpoint name, NVIDIA-Nemotron-3.5-Super-VL-120B-A12B-BF16. - Give the exact vLLM flags behind the results: --mamba-cache-dtype, --enable-mamba-cache-stochastic-rounding and --mamba-cache-philox-rounds 5, and the 32k max-model-len used for MMLU-ProX. - Add a scope note (text-only QAD at 32k; AA-LCR at 64k; no vision benchmarks) and a CHANGELOG entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yue <yueshen@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Security Anti-Patterns PASS. The pull request changes one package Python file and no example Python files. The added Python code contains no torch.load(..., weights_only=False),numpy.load(..., allow_pickle=True), hardc…Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies both main changes: the chat JSONL blend review fixes and the Super VL QAD tutorial updates. Full details: Security Anti-Patterns
-
Explanation
PASS. The pull request changes one package Python file and no example Python files. The added Python code contains no
torch.load(..., weights_only=False),numpy.load(..., allow_pickle=True), hardcodedtrust_remote_code=True, external-inputeval()/exec(), or# nosec. Nopyproject.tomlor requirements file changed, so no new dependency license issue is introduced.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yue <yueshen@nvidia.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2863 +/- ##
==========================================
+ Coverage 76.87% 78.91% +2.04%
==========================================
Files 658 658
Lines 73950 73956 +6
==========================================
+ Hits 56850 58366 +1516
+ Misses 17100 15590 -1510
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ased checkpoint Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yue <yueshen@nvidia.com>
|
/claude review |
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
LGTM: the fixes preserve source-level read failures and add regression coverage; human sign-off is required for the justified existing-test edits.
Needs action:
- Confirm the edits to
test_prepare_megatron_data_blend.py: the error assertion matches the read-only failure message, and the files fixture retains malformed-JSON coverage while adding an unconvertible row.
No action needed:
- The design extends the existing chat converter and Megatron-Bridge filter rather than introducing another system. The PR explains alignment with the tokenizing reader; Pydantic validation or dataset filtering would add unnecessary machinery here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| break | ||
| except Exception as e: # e.g. a gated split, or a schema the streaming reader cannot cast | ||
| raise RuntimeError(f"{name}: failed to read or convert") from e | ||
| raise RuntimeError(f"{name}: failed to read") from e |
There was a problem hiding this comment.
[SUGGESTION] The outer except Exception still catches conversion failures, so the narrowed message can now be wrong.
The inner handler only catches (AttributeError, KeyError, TypeError, ValueError). Anything else raised by _to_chat or by Megatron-Bridge's is_text_only_chat_example — an IndexError on an unexpected message shape, a RecursionError on a deeply nested content, a KeyError subclass that isn't one — still propagates to line 298 and is reported as "{name}: failed to read". The old wording (failed to read or convert) covered exactly that case; the new one sends someone debugging a 36k-row, 11-source blend looking at dataset access and gating when the read actually succeeded.
Either keep the outer message honest, or make the inner catch the only conversion boundary. The cheapest fix is the former:
except Exception as e: # e.g. a gated split, or a schema the streaming reader cannot cast
raise RuntimeError(f"{name}: failed to read or convert") from eand revert the matching test change. If the intent really is "the outer handler is read-only now", then the inner except has to be except Exception so no conversion error can reach it — but that is the broader catch, and it makes the masking concern on line 288 worse.
| try: | ||
| chat = _to_chat(row, source["content_field"], reasoning_content) | ||
| # Judge the source row's top-level media keys too, which the conversion drops. | ||
| if chat is not None and not is_text_only_chat_example({**row, **chat}): | ||
| chat = None | ||
| except (AttributeError, KeyError, TypeError, ValueError) as e: | ||
| # As in the tokenizing reader: one malformed record must not lose the source. | ||
| print(f"[WARN] {name}: skipping a row that cannot be converted: {e!r}", flush=True) | ||
| chat = None |
There was a problem hiding this comment.
[IMPORTANT Compatibility] The per-row try also swallows the config-level KeyError that used to fail fast, turning a typo into a silent 100% rejection.
source["content_field"] is evaluated inside the new try. prepare_chat_data_blend validates the values of content_field (line 377: {source["content_field"] for ...} - {"messages", "text"}) — but that validation itself raises KeyError only on the messages/text path, and crucially _take_chat_rows is also reachable with a source dict whose content_field key is absent in the files: shape used by this PR's own test fixture path. Any such KeyError now gets caught, printed once per row as "skipping a row that cannot be converted", and counted as a rejection.
The consequence: instead of one clear KeyError: 'content_field', the user gets 10 * want + 100 warning lines (for the tutorial's largest source, ~7,000 lines) and then only 0/6890 usable rows (gave up after 6891 rejected rows) — which reads as "this dataset is bad", not "my YAML is wrong". Same for reasoning_content being an invalid mode: prepare_messages_for_chat_template raises ValueError, which is now per-row-swallowed rather than surfacing.
Hoist the lookup out of the loop so a config error can't be mistaken for data:
content_field = source["content_field"]
...
for row in _iter_rows(source, output_dir, shuffle, seed):
try:
chat = _to_chat(row, content_field, reasoning_content)That keeps the intended fix (a malformed row is skipped) while a malformed source still fails on the first access, before any rows are read.
| chat = None | ||
| except (AttributeError, KeyError, TypeError, ValueError) as e: | ||
| # As in the tokenizing reader: one malformed record must not lose the source. | ||
| print(f"[WARN] {name}: skipping a row that cannot be converted: {e!r}", flush=True) |
There was a problem hiding this comment.
[SUGGESTION] This warning is unbounded and prints once per rejected row.
The existing malformed-JSONL warning in _iter_rows has the same shape, but it fires only on genuinely corrupt lines — rare. This one fires on every row that raises, and the give-up budget is 10 * want + 100. A source whose schema systematically mismatches (every row raising, e.g. messages stored as a string throughout) emits up to that many identical lines before the run gives up — for the tutorial's 20-weight source, ~7,000 lines of the same repr, which buries the actual RuntimeError at the end.
Consider capping it, e.g. warn on the first few and then let the existing ({rejected} rejected) / give-up message carry the count:
except (AttributeError, KeyError, TypeError, ValueError) as e:
# As in the tokenizing reader: one malformed record must not lose the source.
if unconvertible < 5:
print(f"[WARN] {name}: skipping a row that cannot be converted: {e!r}", flush=True)
unconvertible += 1
chat = NoneReporting unconvertible alongside rejected in the final summary line would also distinguish "this source is the wrong schema" from "this source is mostly multimodal", which the single rejected counter currently conflates.
| if not text: | ||
| text = row.get("text") | ||
| # A non-string value (e.g. a list) would otherwise pass as list content. | ||
| if not isinstance(text, str) or not text: |
There was a problem hiding this comment.
[SUGGESTION] This type check is the right fix for text; the messages branch above it is left relying on the new downstream except for the same class of input.
For text the PR validates at the source — the root cause. For messages (line 191) the equivalent bad input ({"messages": "hello"}, this PR's own new test case) is only caught after it has been fed through prepare_messages_for_chat_template, which iterates the string character by character and appends each char (chars aren't dicts, so they take the not isinstance(msg, dict) pass-through at dataset_utils.py:86), producing {"messages": ["h", "e", "l", "l", "o"]}. The AttributeError only surfaces later, at line 215's turn.get("role").
That works today, but it's fragile in a way specific to this function: if _to_chat ever gains a step between the conversion and the assistant-turn check that tolerates a list of strings, a string messages value silently becomes a per-character conversation instead of being rejected. Validating in the same place as text closes it and makes the two branches read alike:
if content_field == "messages":
messages = row.get("messages")
if not messages or not isinstance(messages, list):
return NonePer CONTRIBUTING's "fix the bug cause, not the side effect", this is the cause; the broad except in _take_chat_rows then stays a backstop for the unforeseen rather than the mechanism a case you already know about depends on. Worth keeping the except either way — this just stops it from being load-bearing here.
| | --- | --- | | ||
| | `--kv-cache-dtype` | `fp8` | | ||
| | `--max-model-len` | 262144 | | ||
| | `--max-model-len` | 262144 (32768 for MMLU-ProX) | |
There was a problem hiding this comment.
[SUGGESTION] This --max-model-len exception contradicts the "same vLLM settings" claim the results table rests on.
Line 17 says "All checkpoints were served with the same vLLM settings (see Section 5)", and the whole point of the new note above the table is to be precise about what the numbers do and don't represent. But MMLU-ProX — the one benchmark with a large, surprising QAD gain (74.41 → 81.15, the row that gets its own paragraph of explanation) — was served at 32768 rather than 262144.
A reader comparing the MMLU-ProX column will want to know whether that 32768 applied to all three checkpoints or only some. If it applied to all three uniformly the comparison is sound and one clause fixes it; if not, the row isn't apples-to-apples and that matters a lot given it's the headline movement. Either way it's worth stating rather than leaving in a parenthetical two tables away from the claim it qualifies.
Suggest saying it explicitly at line 17, e.g. "All checkpoints were served with the same vLLM settings (see Section 5), MMLU-ProX at --max-model-len 32768 for all three", and then the parenthetical here is just a pointer rather than the only mention.
There was a problem hiding this comment.
Claude review
Scope: full review (the trigger comment carried no scoping instructions). All 7 changed files reviewed — modelopt/torch/utils/plugins/prepare_megatron_data_blend.py, the renamed tutorial README + data_blend.yaml, both index READMEs, CHANGELOG.rst, and the test file. Small diff (+38/-23), so no coverage cap applied.
Findings
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| IMPORTANT | 1 |
| SUGGESTION | 3 |
Most impactful
[IMPORTANT Compatibility] source["content_field"] is evaluated inside the new per-row try. A missing or mistyped content_field in the YAML used to fail fast with a clear KeyError; it now gets caught per row, printed as "skipping a row that cannot be converted", and counted as a rejection — so a config typo surfaces as thousands of warning lines followed by only 0/N usable rows (gave up after ...), which reads as a bad dataset rather than bad YAML. An invalid reasoning_content mode (ValueError from prepare_messages_for_chat_template) is swallowed the same way. Hoisting the lookup above the loop keeps the intended row-level fix while config errors still fail on first access.
The suggestions, in rough order of usefulness:
- The outer
except Exceptionstill catches conversion failures, so narrowing its message tofailed to readcan now misreport. The inner handler lists only four exception types; anything else raised by_to_chatoris_text_only_chat_examplestill reaches it. Reverting the message (and the matching test regex) is the cheap fix. - The
messagesbranch did not get the type checktextdid.{"messages": "hello"}is only rejected afterprepare_messages_for_chat_templatehas turned it into a per-character list, via the newexcept. Addingisinstance(messages, list)next to thetextcheck fixes the cause symmetrically and keeps theexcepta backstop rather than load-bearing. - The new per-row warning is unbounded — up to
10 * want + 100identical lines on a systematically mismatched source, burying the realRuntimeError. Worth capping, and counting unconvertible rows separately from rejected ones. --max-model-len 32768for MMLU-ProX sits in a parenthetical two tables below the "all checkpoints were served with the same vLLM settings" claim, on the one benchmark with the headline QAD movement. Worth stating explicitly whether it applied to all three checkpoints.
Verified clean
- The rename is complete: no
NemotronH-Omni-W4A4-QAD/NemotronH Omni/nemotron_h_omnireferences remain outside legitimate architecture identifiers (NemotronH_Omni_Reasoning_V3,modelopt/torch/export/plugins/mcore_nemotron_omni.py,modelopt/torch/models/hf.py). Both index links and theCHANGELOG.rstURL point at the new directory, anddocs/has no link to the old path. - The referenced recipe
modelopt_recipes/models/nvidia/NVIDIA-Nemotron-3.5-Super-VL-120B-A12B-BF16/ptq/nvfp4-mse.yamlexists. - The
CHANGELOG.rstentry is in the right unreleased section (0.49.0, Megatron Framework (M-LM / M-Bridge)) alongside the sibling Qwen3.6 tutorial entry, and is two sentences written for external users. - Rejection accounting is intact: an unconvertible row increments
rejectedand stays bounded bymax_rejected, matching the PR description and the give-up path. No mode registration, config schema,modelopt_state, export, or public API surface is touched, and no hot path changed. - Test additions are proportionate and extend existing tests rather than adding new ones, per CONTRIBUTING's "be stingy" guidance. The two new cases land in
tests/gpu_megatron/— reasonable given themegatron.bridgeimport, but it does mean thelinuxunit job will not exercise them, consistent with the 0% patch coverage Codecov reports.
Risk: low. Four of seven files are docs or a pure rename, and the code change is a 14-line hardening of a data-prep helper that no training or export path depends on structurally. The IMPORTANT finding is a diagnosability regression rather than a correctness one — no blend produced today changes — but it is worth the one-line hoist before merge, since misattributing a config error to a dataset is expensive on an 11-source, 36k-row blend.
🤖 Generated with Claude Code
kevalmorabia97
left a comment
kevalmorabia97
left a comment
There was a problem hiding this comment.
Approving to unblock. Please address claude review concerns before merging
This branch has not been deployed
3 participants
What does this PR do?
Type of change: Bug fix + documentation
Picks up the post-merge review of #2791 (chat JSONL blends) and #2720 (the W4A4 QAD tutorial), and renames the tutorial now that the model is released (#2819 already names it).
prepare_megatron_data_blend(chat JSONL mode, #2791)files:row{"messages": "hello"}) no longer fails the whole source. Conversion and the Megatron-Bridge text-only check run in a per-rowtry; a failure is logged, counted as a rejected row, and still bounded by the give-up limit, as the tokenizing mode skips such records. (r4279468917)textvalue that is not a string (e.g. a list) is dropped instead of kept as list content. (r4279469089)Tutorial (#2720)
tutorials/NemotronH-Omni-W4A4-QAD/→tutorials/NVIDIA-Nemotron-3.5-Super-VL-120B-A12B-BF16/, matching the Nano tutorial and [Nemotron 3.5 Super VL] Update the NemotronH NVFP4 PTQ recipe to the Nemotron 3.5 Super VL checkpoint #2819's recipe path; the title, intro, checkpoint placeholders and both index links use the released name.vllm servecommand:--enable-mamba-cache-stochastic-roundingand--mamba-cache-philox-rounds 5. It also corrects--mamba-conv-cache-dtype float32to the flag the runs used,--mamba-cache-dtype float32, and notes that MMLU-ProX was served at--max-model-len 32768.nvidia/NVIDIA-Nemotron-3.5-Super-VL-120B-A12B-NVFP4, the calibration and QAD data are all public Hugging Face datasets, and the evaluation results differ from the official model card scores.CHANGELOG.rstentry for the tutorial.Testing
On oci-jhb, NeMo 26.08 container with Megatron-Bridge
main:tests/gpu_megatron/torch/utils/plugins/test_prepare_megatron_data_blend.py: 28 passed.1 failedeach).train.jsonl0301f3c9…,validation.jsonl9171c8db…), with no conversion warnings.Not rerun: PTQ, QAD, export or serving; those paths change only in docs.
Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Follow-up to #2791 and #2720.
🤖 Generated with Claude Code
Summary by CodeRabbit