Skip to content

Fix Hparam configurability restoration after exceptions - #2128

Merged
kevalmorabia97 merged 1 commit into
NVIDIA:mainfrom
taking-lying-flat:agent/restore-hparam-configurable-state
Aug 11, 2026
Merged

Fix Hparam configurability restoration after exceptions#2128
kevalmorabia97 merged 1 commit into
NVIDIA:mainfrom
taking-lying-flat:agent/restore-hparam-configurable-state

Conversation

@taking-lying-flat

@taking-lying-flat taking-lying-flat commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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:

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 and your commits are signed (git commit -s -S).

Make sure you read and follow the Security Best Practices (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?: 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.

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.

Signed-off-by: taking-lying-flat <1615405@qq.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 10, 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 Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

_force_configurable now restores the original configurability state after exceptions. A unit test verifies temporary enablement, state restoration, and exception propagation.

Changes

Configurability restoration

Layer / File(s) Summary
Exception-safe context manager
modelopt/torch/opt/hparam.py, tests/unit/torch/opt/test_dynamic.py
_force_configurable() restores _is_configurable in a finally block. The test verifies restoration after a RuntimeError and confirms exception propagation.

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

Suggested reviewers: aanoosheh

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
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 The commit adds only try/finally cleanup and a regression test; no prohibited security patterns, # nosec comments, or dependency changes appear in the diff.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the fix to restore Hparam configurability after exceptions.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@taking-lying-flat
taking-lying-flat marked this pull request as ready for review August 11, 2026 00:28
@taking-lying-flat
taking-lying-flat requested review from a team as code owners August 11, 2026 00:28
@taking-lying-flat
taking-lying-flat marked this pull request as draft August 11, 2026 00:48
@taking-lying-flat
taking-lying-flat marked this pull request as ready for review August 11, 2026 01:52
@kevalmorabia97

Copy link
Copy Markdown
Collaborator

/ok to test 5496251

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.13%. Comparing base (c6889c6) to head (5496251).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2128      +/-   ##
==========================================
- Coverage   78.73%   78.13%   -0.60%     
==========================================
  Files         522      522              
  Lines       60342    60343       +1     
==========================================
- Hits        47508    47152     -356     
- Misses      12834    13191     +357     
Flag Coverage Δ
examples-diffusers 20.80% <0.00%> (-0.01%) ⬇️
examples-gpt-oss 13.28% <0.00%> (-0.01%) ⬇️
examples-hf_ptq 21.48% <0.00%> (-0.04%) ⬇️
examples-llm_distill 13.34% <0.00%> (-0.01%) ⬇️
examples-llm_eval 17.10% <0.00%> (-0.01%) ⬇️
examples-llm_qat 16.65% <0.00%> (-0.01%) ⬇️
examples-llm_sparsity 15.93% <0.00%> (-0.01%) ⬇️
examples-megatron_bridge 25.69% <100.00%> (-0.15%) ⬇️
examples-specdec_bench 13.02% <0.00%> (-0.01%) ⬇️
examples-speculative_decoding 17.53% <0.00%> (-0.08%) ⬇️
examples-torch_onnx 21.89% <0.00%> (-0.01%) ⬇️
examples-torch_trt 15.10% <0.00%> (-0.01%) ⬇️
gpu 58.60% <100.00%> (-0.71%) ⬇️
regression 14.91% <0.00%> (+0.06%) ⬆️
unit 55.30% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kevalmorabia97
kevalmorabia97 merged commit e4fe1e5 into NVIDIA:main Aug 11, 2026
55 checks passed
@taking-lying-flat
taking-lying-flat deleted the agent/restore-hparam-configurable-state branch August 11, 2026 09:54
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.

2 participants