Skip to content

fix: config loader no longer discards the body of a null-header multi-document file#1919

Open
arham766 wants to merge 1 commit into
NVIDIA:mainfrom
arham766:fix/config-null-header
Open

fix: config loader no longer discards the body of a null-header multi-document file#1919
arham766 wants to merge 1 commit into
NVIDIA:mainfrom
arham766:fix/config-null-header

Conversation

@arham766

@arham766 arham766 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Type of change: Bug fix

Silent data loss in the config loader (found writing #1914): a null/~ first YAML document (or empty explicit document) tripped the empty-file early return before the two-document branch, so 'null / --- / ' loaded as {} and the body vanished. A null first document is now an empty header: dict bodies load as themselves, list bodies take the existing snippet path, a lone null still loads {}, and the >2-document error now applies to null-first files too (previously silently {}). Full branch-by-branch before/after table in the review notes; three regression tests in tests/unit/recipe/test_loader.py verified to fail pre-fix with the exact {}-instead-of-body symptom; builtin presets still load (import-time validation green).

Usage

N/A

Testing

Regression tests included, each verified to FAIL with the fix reverted. Combined battery with all sibling wave fixes: 381 passed across tests/unit/torch/opt, tests/unit/torch/utils, tests/unit/recipe, and quantization test_mode. Note: the unmerged suite PRs #1913-#1916 contain behavior-documenting NOTE tests pinning the OLD behavior fixed here — whichever lands second gets the NOTE tests flipped (happy to rebase either way).

Before your PR is "Ready for review"

  • Is this change backward compatible?: ✅
  • If you copied code from any other sources or added a new PIP dependency, did you follow guidance in CONTRIBUTING.md: N/A
  • Did you write any new necessary tests?: ✅
  • Did you update Changelog?: N/A
  • Did you get Claude approval on this PR?: N/A (external contributor)

Additional Information

Issue: #1902 (wave-2 findings)

Summary by CodeRabbit

  • Bug Fixes
    • Improved loading of YAML files with multiple documents, including cases where the first document is empty or null.
    • Config files with a blank or null header now load correctly when the actual settings are in the second document.
    • A single null YAML document now loads as an empty configuration.
  • Tests
    • Added coverage for multi-document and empty-header config loading scenarios.

…-document file

An explicit null/~ first YAML document (or an empty explicit document
via a leading '---' pair) tripped the empty-file early return before
the two-document branch, so 'null / --- / <body>' silently loaded as
{}. A null first document is now treated as an empty header: dict
bodies load as themselves, list bodies take the existing snippet path
with no imports, a lone null still loads as {}, and the >2-document
error now applies to null-first files too (previously they silently
returned {}).

Part of the findings in NVIDIA#1902 (wave 2).

Signed-off-by: arham766 <arhamislam766@yahoo.com>
@arham766 arham766 requested review from a team as code owners July 6, 2026 01:49
@arham766 arham766 requested a review from ChenhanYu July 6, 2026 01:49
@copy-pr-bot

copy-pr-bot Bot commented Jul 6, 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.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6813655c-72a9-4513-9429-d10a533539f6

📥 Commits

Reviewing files that changed from the base of the PR and between b96a785 and 4c5a1d4.

📒 Files selected for processing (2)
  • modelopt/torch/opt/config_loader.py
  • tests/unit/recipe/test_loader.py

📝 Walkthrough

Walkthrough

The config loader's YAML parsing logic was updated to treat a null first document in a two-document YAML file as an empty header instead of rejecting it, allowing the second document to carry the actual config body. Corresponding unit tests were added.

Changes

YAML Multi-Document Header Handling

Layer / File(s) Summary
Null header normalization
modelopt/torch/opt/config_loader.py
_load_raw_config_with_schema now normalizes a None first document into {} for two-document YAML inputs, and refines when an empty _RawConfig is returned.
Test coverage for null/empty headers
tests/unit/recipe/test_loader.py
Added four tests validating load_config behavior for multi-document YAML with null/empty headers paired with list, dict, or lone-null bodies.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main fix: preserving the body of null-header multi-document YAML files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Anti-Patterns ✅ Passed Inspected the changed loader and tests; no torch.load(weights_only=False), numpy.load(allow_pickle=True), trust_remote_code=True, eval/exec, or # nosec additions.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant