Skip to content

test: direct unit suite for opt config_loader (106 cases)#1914

Closed
arham766 wants to merge 2 commits into
NVIDIA:mainfrom
arham766:tests/opt-config-loader
Closed

test: direct unit suite for opt config_loader (106 cases)#1914
arham766 wants to merge 2 commits into
NVIDIA:mainfrom
arham766:tests/opt-config-loader

Conversation

@arham766

@arham766 arham766 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Type of change: new tests

Part of the unit-coverage initiative in #1902. Direct unit suite (106 cases) for modelopt/torch/opt/config_loader.py — the config loading/$import machinery. Design: a synthetic schema module is injected under a private modelopt.* name per-test (registered as the last setup statement, popped in teardown), keeping the suite hermetic and independent of the quantization stack; real-schema integration remains owned by tests/unit/recipe/test_loader.py — the overlap is deliberately layered (unit vs integration), and mutation testing showed the unit versions are sharper (a header/content merge-order flip is caught here but not by the recipe suite's non-conflicting-keys test).

Covers: ExMy num_bits/scale_bits parsing rules; path resolution (.yml-before-.yaml probe order, local-shadows-builtin precedence, error paths); modelopt-schema comment parsing; multi-document YAML merging (content-wins) and list snippets; $import resolution (replace/extend/inline-override precedence, later-wins, recursion, cycle/self-import detection) with exact error-message assertions; schema-resolution diagnostics incl. the initializing-module circular-import message; typing helpers (_unwrap_schema_type, _list_element_schema union ambiguity, _child_schema).

Bug documented in-test (NOTE comment; one-line fix available if preferred as a follow-up): a file whose first document is an explicit null followed by a list body (null / --- / - item) silently loads as {} — the docs[0] is None early-return fires before the two-document branch (config_loader.py:196), silently discarding the body.

Usage

N/A — tests only.

Testing

106 cases, ~1s, hermetic (tmp_path, no network, deterministic, Windows/Linux-safe paths). Adversarial review: 16 expectations independently re-derived from source, 7/7 seeded mutations killed, no vacuous tests, no sys.modules leak (teardown verified to run on failure; name collision-checked against the real package).

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

Summary by CodeRabbit

  • Tests
    • Added extensive unit coverage for configuration loading, including schema detection, YAML parsing, and multi-document merging behavior.
    • Improved validation of edge cases such as empty/comment-only files, duplicate schema markers, and robust schema/type handling.
    • Expanded verification for $import behaviors (precedence, nested merging, list/dict splicing) plus clear error reporting for unknown, missing, invalid, or circular imports.
    • Added round-trip checks to ensure loaded configurations serialize/deserialize consistently.

Hermetic unit tests for the config loading/import machinery via a
synthetic schema module injected under a private modelopt name
(registered/removed per test; the real-schema integration surface is
owned by tests/unit/recipe/test_loader.py, and the duplication is
layered unit-vs-integration by design). Covers ExMy parsing, path
probing and local-shadows-builtin precedence, multi-document merging,
$import replace/extend/inline-override/later-wins/cycle rules with
exact error messages, schema resolution diagnostics, and typing
helpers. Adversarially reviewed: 16 expectations re-derived, 7/7
seeded mutations killed - including a merge-order flip the recipe
suite's non-conflicting-keys test would not catch.

Also documents (NOTE test) that a null first YAML document followed by
a list body silently loads as {}, discarding the body.

Part of the coverage initiative in NVIDIA#1902.

Signed-off-by: arham766 <arhamislam766@yahoo.com>
@arham766 arham766 requested a review from a team as a code owner July 5, 2026 21:57
@copy-pr-bot

copy-pr-bot Bot commented Jul 5, 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 5, 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: ec8e08b0-fbfe-43b9-9e9f-8257a236cadd

📥 Commits

Reviewing files that changed from the base of the PR and between 30c346d and efed7a4.

📒 Files selected for processing (1)
  • tests/unit/torch/opt/test_config_loader.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/unit/torch/opt/test_config_loader.py

📝 Walkthrough

Walkthrough

This PR adds a comprehensive unit test suite for modelopt.torch.opt.config_loader, covering schema parsing, YAML loading, config resolution, validation helpers, $import behavior, error handling, and round-trip serialization checks.

Changes

Config loader test suite

Layer / File(s) Summary
Test scaffolding and helpers
tests/unit/torch/opt/test_config_loader.py
Defines synthetic schema types, a fixture registering a synthetic modelopt-schema module in sys.modules, and file-writing helpers used by the tests.
ExMy parsing and config path resolution
tests/unit/torch/opt/test_config_loader.py
Tests ExMy string conversion helpers and config path resolution across suffix handling, aliases, missing files, and built-in versus local precedence.
Schema comment parsing and raw YAML loading
tests/unit/torch/opt/test_config_loader.py
Tests # modelopt-schema: comment parsing, raw YAML loading for empty and multi-document files, list-snippet header extraction, and schema-attached raw loading.
Schema path resolution and typing helpers
tests/unit/torch/opt/test_config_loader.py
Tests schema path resolution and labeling, wrapper unwrapping, schema equality, typed-list element extraction, child-schema lookup, and _validate_modelopt_schema behavior.
load_config end-to-end tests
tests/unit/torch/opt/test_config_loader.py
Tests load_config for raw versus schema-validated results, explicit schema_type precedence, validation errors, defaults for empty payloads, and missing-file handling.
$import resolution core semantics
tests/unit/torch/opt/test_config_loader.py
Tests dict-level $import replacement and override behavior, multi-import merge precedence, recursion, and non-mutation of the input tree.
$import typed-list splicing and appending
tests/unit/torch/opt/test_config_loader.py
Tests typed-list splicing versus appending rules and schema-mismatch rejection cases for imported snippets.
$import error handling and cycle detection
tests/unit/torch/opt/test_config_loader.py
Tests unknown and missing import errors, invalid imports shape, empty import paths, schema mismatch on import, eager import loading, circular imports, and _find_import_marker behavior.
Round-trip serialization checks
tests/unit/torch/opt/test_config_loader.py
Tests that resolved configs and validated schema instances round-trip through YAML dump/reload and model_dump.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested labels: tests, torch, opt

Suggested reviewers: none identified

🚥 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 concisely describes the main change: adding a direct unit test suite for config_loader.
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 Only a test file changed; SECURITY.md exempts tests, and the diff contains no banned patterns or dependency additions.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
tests/unit/torch/opt/test_config_loader.py (1)

244-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Tests couple to real built-in config content.

test_resolve_config_path_builtin_library and test_load_raw_config_builtin_traversable assert on the actual shipped configs/numerics/fp8.yaml contents (e.g., {"num_bits": (4, 3), "axis": None}). An unrelated future edit to that built-in config's values would break these unit tests even though the path-resolution/loading logic under test is unaffected. Consider writing a temporary file under BUILTIN_CONFIG_ROOT (or monkeypatching that constant to a tmp directory) to fully decouple from production config content.

Also applies to: 423-428

🤖 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 `@tests/unit/torch/opt/test_config_loader.py` around lines 244 - 250, The
config loader tests are tied to the shipped built-in fp8 config content instead
of only validating path resolution/loading behavior. Update
test_resolve_config_path_builtin_library and
test_load_raw_config_builtin_traversable to use a temporary built-in config tree
by writing a test file under BUITIN_CONFIG_ROOT or monkeypatching that constant,
and assert against the test fixture content rather than the real
configs/numerics/fp8.yaml values. Keep the checks focused on
_resolve_config_path and _load_raw_config behavior so future production config
edits do not break these unit tests.
🤖 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 `@tests/unit/torch/opt/test_config_loader.py`:
- Around line 244-250: The config loader tests are tied to the shipped built-in
fp8 config content instead of only validating path resolution/loading behavior.
Update test_resolve_config_path_builtin_library and
test_load_raw_config_builtin_traversable to use a temporary built-in config tree
by writing a test file under BUITIN_CONFIG_ROOT or monkeypatching that constant,
and assert against the test fixture content rather than the real
configs/numerics/fp8.yaml values. Keep the checks focused on
_resolve_config_path and _load_raw_config behavior so future production config
edits do not break these unit tests.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 37b96803-9f8c-4b1e-b0cf-393ea9656bad

📥 Commits

Reviewing files that changed from the base of the PR and between b96a785 and 30c346d.

📒 Files selected for processing (1)
  • tests/unit/torch/opt/test_config_loader.py

…ippet

Signed-off-by: arham766 <arhamislam766@yahoo.com>
@arham766

arham766 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Consolidated into #1927 per maintainer feedback in #1902 — the suite was trimmed to only the lines codecov reports uncovered, with parametrization clusters deduplicated. Closing in favor of that PR.

@arham766 arham766 closed this Jul 6, 2026
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