fix: apply_mode accepts ModeDescriptor instances and an empty mode list with a ModelLike#1918
Open
arham766 wants to merge 2 commits into
Open
fix: apply_mode accepts ModeDescriptor instances and an empty mode list with a ModelLike#1918arham766 wants to merge 2 commits into
arham766 wants to merge 2 commits into
Conversation
…st with a ModelLike get_mode_config only handled str/tuple entries, so apply_mode(model, [descriptor_instance]) raised TypeError - despite the docstring and the ModeType = ModeDescriptor | str annotation. Descriptors are now normalized to their name up front (registry descriptors are per-name singletons, so name resolution returns the same instance); str/tuple paths are byte-identical, and get_mode_config's only caller is apply_mode. apply_mode((cls, args, kwargs), []) - the documented 'vanilla case (just initialize+return)' - crashed in init_modellike, which unconditionally transferred never-initialized state. The transfer is now guarded by is_converted, keeping the two vanilla paths consistent: both return a plain initialized model without modelopt state. The state-carrying ModelLike path is unchanged (the manager is constructed before init_modellike runs in normal flows). Part of the findings in NVIDIA#1902 (wave 2). Signed-off-by: arham766 <arhamislam766@yahoo.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR makes ChangesModeDescriptor and ModelLike fixes
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modelopt/torch/opt/mode.py (1)
357-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNested ternary is functionally correct but a bit dense.
Logic correctly standardizes
ModeDescriptor/str/tuple inputs, matching the new descriptor-instance test. Consider an explicit if/elif/else for readability, though this is optional.♻️ Optional readability refactor
def get_mode_config(mode_like: ModeLike) -> ModeConfigList: """Standardize mode to ModeConfigDict and return.""" - mode_and_config = [ - ( - (m.name, {}) - if isinstance(m, ModeDescriptor) - else (m, {}) - if isinstance(m, str) - else (m[0], m[1] or {}) - ) - for m in val2list(mode_like) - ] - - return mode_and_config + def _to_pair(m): + if isinstance(m, ModeDescriptor): + return (m.name, {}) + if isinstance(m, str): + return (m, {}) + return (m[0], m[1] or {}) + + return [_to_pair(m) for m in val2list(mode_like)]🤖 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 `@modelopt/torch/opt/mode.py` around lines 357 - 370, The logic in get_mode_config is correct, but the nested ternary used to normalize ModeDescriptor, str, and tuple inputs is hard to read. Refactor the list construction in get_mode_config into an explicit if/elif/else structure inside the loop over val2list(mode_like), preserving the same output shape and default {} handling for each input type.
🤖 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 `@modelopt/torch/opt/mode.py`:
- Around line 357-370: The logic in get_mode_config is correct, but the nested
ternary used to normalize ModeDescriptor, str, and tuple inputs is hard to read.
Refactor the list construction in get_mode_config into an explicit if/elif/else
structure inside the loop over val2list(mode_like), preserving the same output
shape and default {} handling for each input type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 51da449d-b75e-4d01-ab1f-2de1af577fd1
📒 Files selected for processing (3)
modelopt/torch/opt/conversion.pymodelopt/torch/opt/mode.pytests/unit/torch/opt/test_chaining.py
Signed-off-by: arham766 <arhamislam766@yahoo.com>
arham766
force-pushed
the
fix/apply-mode-contract
branch
from
9451fad to
54b9a03
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Type of change: Bug fix
Two API-contract bugs in the opt mode framework (found writing #1915): apply_mode(model, [descriptor_instance]) raised TypeError despite the docstring and the ModeType = ModeDescriptor | str annotation — descriptors are now normalized to their name up front (registry descriptors are per-name singletons, so resolution round-trips; get_mode_config's only caller is apply_mode, audited); and apply_mode((cls, args, kwargs), []) — the documented vanilla initialize+return case — crashed in init_modellike's unconditional state transfer, now guarded by is_converted so both vanilla paths consistently return a plain initialized model. Existing ModelLike-with-state coverage stays green; regression tests appended to test_chaining.py, both verified to fail pre-fix with the exact reported signatures.
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"
Additional Information
Issue: #1902 (wave-2 findings)
Summary by CodeRabbit