Skip to content

test: direct unit suites for opt Hparam and BaseSearcher (78 tests) - #1913

Closed
arham766 wants to merge 1 commit into
NVIDIA:mainfrom
arham766:tests/opt-hparam-searcher
Closed

test: direct unit suites for opt Hparam and BaseSearcher (78 tests)#1913
arham766 wants to merge 1 commit into
NVIDIA:mainfrom
arham766:tests/opt-hparam-searcher

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 suites for the two search-framework primitives (no direct coverage existed; the duplication audit against test_dynamic.py, tests/unit/torch/nas, and test_autoquant.py is in the review notes):

  • test_hparam.py (44 tests) — construction/dedup/original folding, configurability incl. the _force_configurable context manager, active-value validation with exact error messages, active_slice/enforce_order LongTensor semantics, choices-setter subset rules, importance registration (exact max-normalization values, multi-estimator summation, unsortable rejection), iand merging, and pickle round-trip (required for Megatron distributed broadcast per the source NOTE).
  • test_searcher.py (34 tests) — default-config contract, sanitize merge rules, the search() hook order and training-mode restoration, deepcopied default state, checkpoint path-resolution table (file/dir/extensionless/pathlib), strict vs non-strict state loading, end-to-end resume across two search() calls, scoring, and forward-loop construction with exact iteration counts.

Usage

N/A — tests only.

Testing

78 tests, <1s, hermetic (CPU-only, tmp_path, deterministic). Adversarially reviewed: 12 expectations independently re-derived from source, 6/6 seeded mutations killed, no vacuous tests, Windows/Linux path-agnostic. Full tests/unit/torch/opt dir passes alongside (excluding the pre-existing test_lr_config.py env failures).

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 comprehensive unit coverage for Hparam, including defaults, configurability, active/choices updates, ordering, importance handling, merging, and pickle roundtrips.
    • Added extensive BaseSearcher tests covering search flow, checkpoint save/load, resume behavior, scoring, and forward-loop/data-loader execution rules.

Hermetic CPU-only tests for the two search-framework primitives; no
direct suites existed (test_dynamic.py uses Hparam only as an opaque
payload; nas/autoquant tests cover subclasses). Adversarially reviewed
and verified to kill seeded mutations of both modules (importance
normalization, __iand__ intersection, active_slice truncation,
training-mode restore, checkpoint strict default, path resolution).

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:50
@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: f6bc7aee-5ec8-429e-b73f-78a8d29ceaba

📥 Commits

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

📒 Files selected for processing (2)
  • tests/unit/torch/opt/test_hparam.py
  • tests/unit/torch/opt/test_searcher.py

📝 Walkthrough

Walkthrough

Adds two new unit test modules: one exhaustively testing the Hparam primitive (construction, configurability, active/choices setters, order enforcement, importance/sortability, __iand__ merging, pickle roundtrip), and another testing BaseSearcher (config sanitization, search workflow, checkpointing, scoring, forward-loop construction). No public entities are modified.

Changes

Hparam Unit Tests

Layer / File(s) Summary
Construction, properties, and configurability
tests/unit/torch/opt/test_hparam.py
Tests cover module setup, construction semantics (original/active defaults, choices folding/sorting, min/max, iteration, repr), and configurability rules including manual override and the _force_configurable() context manager.
Active setter, active_slice, and choices setter
tests/unit/torch/opt/test_hparam.py
Tests validate the active setter, active_slice/enforce_order behavior (permutation conversion/validation, reset), and the choices setter (subset shrinking, required membership, rejection of new values).
Importance/sortability and __iand__ merging
tests/unit/torch/opt/test_hparam.py
Tests cover importance registration/normalization, order mode, sortability requirements, __iand__ merge semantics, and a pickle roundtrip preserving full state.

BaseSearcher Unit Tests

Layer / File(s) Summary
Test fixtures, config sanitization, and search() workflow
tests/unit/torch/opt/test_searcher.py
Adds concrete BaseSearcher subclasses/helpers and tests config defaulting/rejection, hook execution order, training-mode restoration, state reset/deep-copy, and state_dict() reflection.
Checkpoint path resolution and persistence
tests/unit/torch/opt/test_searcher.py
Tests checkpoint path resolution, save/load roundtrips, directory creation, strict vs non-strict key handling, and resuming a search from a saved checkpoint.
Scoring and forward-loop construction
tests/unit/torch/opt/test_searcher.py
Tests eval_score() requirements/behavior with the silent flag, and construct_forward_loop() rules including exclusivity constraints and max_iter_data_loader handling.

Estimated code review effort: 2 (Simple) | ~15 minutes

Related Issues: None found.

Related PRs: None found.

Suggested labels: tests

Suggested reviewers: None specified.

🚥 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 describes the main change: adding direct unit test suites for Hparam and BaseSearcher.
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 two test files changed; no new modelopt/examples code or listed security anti-patterns (# nosec, unsafe loads, trust_remote_code=True, eval/exec) were found.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@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
kevalmorabia97 pushed a commit that referenced this pull request Aug 11, 2026
### What does this PR do?

Type of change: Bug fix

`Hparam._force_configurable()` restored `_is_configurable` only after a
normal context-manager exit. If code inside the context raised, the
restoration statement was skipped and a previously non-configurable
`Hparam` remained permanently configurable.

Wrap the `yield` in `try`/`finally` so the original state is restored on
both normal and exceptional exits. Add a regression test that raises
inside the context and verifies the temporary configurable state does
not leak.

### Usage

No call-site changes are required. Existing users of the context manager
retain the same behavior, including when an operation inside the context
fails:

```python
with hp._force_configurable():
    hp.choices = new_choices
```

### Testing

- Verified the new regression test fails against the pre-fix
implementation because `hp.is_configurable` remains `True`.
- `.venv/bin/python -m pytest tests/unit/torch/opt/test_dynamic.py -q`
(`3 passed`)
- `.venv/bin/python -m pytest tests/unit/torch/opt -q` (`66 passed, 2
skipped`)
- Relevant pre-commit hooks passed: file/merge/line-ending checks, Ruff
check and format, mypy, license insertion check, Bandit.
- `git diff --check`

### Before your PR is "*Ready for review*"

Make sure you read and follow [Contributor
guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md)
and your commits are signed (`git commit -s -S`).

Make sure you read and follow the [Security Best
Practices](https://github.com/NVIDIA/Model-Optimizer/blob/main/SECURITY.md#security-coding-practices-for-contributors)
(e.g. avoiding hardcoded `trust_remote_code=True`, `torch.load(...,
weights_only=False)`, `pickle`, etc.).

- 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](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?:
N/A — this is a small, backward-compatible correctness fix rather than a
critical bug fix.
- Did you get Claude approval on this PR?: N/A — not available to an
external contributor before submission.

### Additional Information

Related prior work: #1913 was closed without merging and only tested the
normal-exit path. This PR adds exception-path coverage together with the
production fix.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Improved configuration handling so temporary settings are reliably
restored even when an error occurs.
* Exceptions raised during configuration operations continue to be
reported correctly.

* **Tests**
* Added coverage for configuration restoration after a failed operation.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: taking-lying-flat <1615405@qq.com>
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