Conversation
Signed-off-by: Kai Xu <kaix@nvidia.com>
|
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)
Included review availability: Your plan provides up to 12 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe GDN module discovery path validates required tensors and supported module identities before policy processing. Tests verify module paths and conversion guidance in resulting errors. ChangesGDN validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Merge Risk: ⚪ Minimal · up to The GDN validation changes are covered by focused regression tests and required checks, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ) | ||
| raise ApplyModeError(f"DASC found no supported GDN modules; expected one of: {supported}") | ||
| return dict(sorted(modules.items())) | ||
| return dict(sorted(identity_modules)) |
There was a problem hiding this comment.
[SUGGESTION] Sorting the raw (name, module) tuples makes the sort key implicitly include the module.
Previously this was dict(sorted(modules.items())), where modules = dict(identity_modules) had already collapsed names, so only strings were ever compared. Now sorted() operates on tuples, and if two entries ever shared a name Python would fall through to comparing the second elements — nn.Module defines no ordering, so that raises TypeError: '<' not supported between instances of 'GatedDeltaNet' and 'GatedDeltaNet' instead of the intended ApplyModeError.
This is unreachable today: named_modules() defaults to remove_duplicate=True and yields unique qualified paths, so no tie can occur. Purely defensive — sorting on the name explicitly keeps the key intent obvious and immune to the comparison fallback, without reintroducing the dict alias this PR removed.
| return dict(sorted(identity_modules)) | |
| return dict(sorted(identity_modules, key=lambda item: item[0])) |
There was a problem hiding this comment.
Addressed in #2386. The sort now uses an explicit qualified-name key, so it cannot fall through to comparing nn.Module objects even under a hypothetical duplicate-name input.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude review passed — no blocking issues found. LGTM
Scope
Full review — the trigger comment was a bare /claude review. Both changed files opened (modelopt/torch/sparsity/state_sparsity/policy.py, tests/unit/torch/sparsity/state_sparsity/test_dasc.py), plus surrounding context in policy.py (_has_supported_gdn_identity, compute_gdn_decay_horizons, _analyze_gdn_modules) to confirm the extraction is semantics-preserving.
Findings
- CRITICAL: 0
- IMPORTANT: 0
- SUGGESTION: 1
Assessment
Risk: low. This is a pure internal refactor plus one regression test. No mode registration, modelopt_state schema, config field, public __init__.py export, or export path is touched, so there are no composability, restore-fidelity, or backward-compatibility concerns to weigh.
The extraction into _reject_incomplete_gdn_modules and _reject_unconverted_gdn_subclasses preserves behavior exactly:
- Check order is unchanged — incomplete-decay-tensor rejection still fires before the unconverted-subclass rejection, which still fires before the empty-
identity_modulesApplyModeError. A model malformed in both ways reports the same message it did before. - Predicates are identical to the inlined versions, including the
name or "<root>"fallback and the__mro__[1:]slice that lets an exact supported class pass the subclass check. - The dropped
modulesalias is a no-op —if not modulesandif not identity_modulesagree in truthiness, and the returned dict is still keyed and sorted by module path.
The two new/tightened test regexes match the ApplyModeError message strings in policy.py character-for-character, including the implicit-concatenation seam in the second one ("...use a " + "supported class directly"). The end-of-string anchors are the substantive part of the test change and pull real weight: they assert the diagnostic names only the offending path, so a regression that swept the valid sibling (good) into the error list would now fail rather than pass on a loose substring match. The new mixed valid-plus-unconverted-subclass case closes the gap its counterpart already covered for missing decay tensors.
The one SUGGESTION concerns dict(sorted(identity_modules)) sorting raw tuples, which makes the sort key implicitly include the nn.Module. It is unreachable given named_modules() yields unique paths — non-blocking and purely defensive.
I reviewed the test changes statically rather than executing them; running the suite locally needed a permission I did not have, so I did not independently reproduce the 23-passed result reported in the PR description.
🤖 Generated with Claude Code
|
## Summary Follow-up to #2385 addressing its sole defensive Claude suggestion: - sort discovered GDN modules with an explicit qualified-name key - prevent any hypothetical duplicate-name fallback from comparing nn.Module objects This PR is intentionally stacked on #2385 because repository rules protect a PR head branch after creation. ## Validation - focused DASC suite: 23 passed, 1 absent optional Megatron skip - DASC plus weight sparsity plus attention sparsity compatibility suite: 134 passed, 1 optional skip - full pre-commit on the touched file: passed - prior full-package coverage remains 408/408 statements; this change adds no statements and the line is exercised by the focused suite <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * GDN modules are now returned in a consistent, predictable order. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Kai Xu <kaix@nvidia.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/dasc-state-sparsity-complete-layers #2385 +/- ##
============================================================================
Coverage 78.76% 78.76%
============================================================================
Files 548 548
Lines 64189 64192 +3
============================================================================
+ Hits 50561 50564 +3
Misses 13628 13628
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
## Summary Consolidates the complete reviewed fix stack for #2375 into one DCO-safe commit: - harden package exports, measurement semantics, wrapper handling, and actionable calibration errors - validate exact installed GDN identities plus ModelOpt dynamic subclasses; reject lookalikes, ordinary subclasses, incomplete layers, and partial layer sets - make stale checkpoints saveable and restorable while keeping deployment export strict - make DASC recalibration replace and deduplicate existing mode state without stale-metadata refresh - record the declared decay-parameter checkpoint storage dtype and use derived FP16/BF16/FP32 rounding bounds - preserve BF16/FP16 storage and wider/cross-dtype reload compatibility without globally widening FP32 tolerance - add installed Transformers path coverage, optional Megatron gating, lifecycle, tamper, lossy-cast, and mixed-layer regressions - document the explicit storage-dtype contract This consolidated PR supersedes the mechanically stacked review-fix PRs #2377, #2378, #2379, #2380, #2382, #2383, #2384, and #2385. Its tree is byte-identical to the independently reviewed leaf commit from #2386. ## Validation - focused DASC suite: 23 passed, 1 absent optional Megatron skip - DASC plus weight sparsity plus attention sparsity compatibility suite: 134 passed, 1 optional skip - DASC package coverage: 408/408 statements, 100% - full pre-commit on all touched files: passed - real Transformers Qwen3NextGatedDeltaNet BF16 storage to FP32 reload smoke: passed - commit author and Signed-off-by identity both use kaix-nv <kaix@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for configuring decay-parameter storage precision with FP16, BF16, or FP32. * Added safer recalibration that replaces existing DASC state. * Expanded compatibility with supported GDN adapter classes and model wrappers. * Added improved validation for sparsity policies, measurements, model structure, and decay parameters. * Added support for perplexity-retention values above 1. * **Documentation** * Clarified evaluation responsibilities, recalibration behavior, stale-policy handling, supported adapters, and dtype requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: kaix-nv <kaix@nvidia.com>
1 participant
Summary
Follow-up to #2384 addressing its complete Claude review:
This PR is intentionally stacked on #2384 because repository rules protect a PR head branch after creation.
Validation
Summary by CodeRabbit
Bug Fixes
Tests