Conversation
Signed-off-by: kaix-nv <kaix@nvidia.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughDASC policy handling now classifies recoverable staleness, validates decay and analysis inputs before processing, updates restoration paths, documents serialization rules, and adds boundary and checkpoint recovery tests. ChangesDASC staleness handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The updated DASC validation and stale-policy recovery paths preserve fail-closed export behavior for unavailable decay tensors, with no unresolved merge risk identified. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
/claude review |
|
@coderabbitai full review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # Check the original tensors before a storage cast can hide an invalid integer dtype. | ||
| _validate_gdn_decay_tensors(a_log, dt_bias) |
There was a problem hiding this comment.
[SUGGESTION] This hoist is a behavioral no-op, and the comment describes a guarantee that already existed.
- With
storage_dtype is not None, the pre-cast validation was already in place (it was the line removed from inside theif), so the "invalid integer dtype hidden by the cast" case was already rejected before this PR. - With
storage_dtype is None,a_log/dt_biasare passed through unchanged andcompute_gdn_decay_horizonsruns the identical_validate_gdn_decay_tensors(a_log, dt_bias)at policy.py:121 — same function, same tensors, same message. So the only effect here is a second, redundant validation pass on the non-cast path, which cuts against the "Validate external input once" guideline in CONTRIBUTING.md.
Not harmful, but the PR summary lists this as a fix ("validate original decay tensors before checkpoint-storage casting can mask invalid dtypes"), and nothing about the observable contract changes. If the goal is a single explicit validation point in _analyze_gdn_modules, that's a reasonable readability choice — but then the comment should say that rather than implying a cast-masking hole was closed. If some other masking case was intended (e.g. a dtype that survives _validate_gdn_decay_tensors on the original but not after the cast), a test asserting the new rejection would pin it down; the added tests only cover the analyze_gdn_decay argument boundary.
There was a problem hiding this comment.
Agreed and corrected in #2395. The hoist was redundant on the no-cast path and did not close a new masking hole. It is reverted; the original pre-cast validation remains only in the storage-cast branch, while direct computation validates its actual inputs once.
| try: | ||
| epsilon_is_valid = math.isfinite(epsilon) and 0.0 < epsilon < 1.0 | ||
| except TypeError: | ||
| epsilon_is_valid = False | ||
| if not epsilon_is_valid: | ||
| raise ValueError("epsilon must be finite and in (0, 1)") | ||
| try: | ||
| static_gate_input_is_valid = math.isfinite(static_gate_input) | ||
| except TypeError: | ||
| static_gate_input_is_valid = False | ||
| if not static_gate_input_is_valid: | ||
| raise ValueError("static_gate_input must be finite") |
There was a problem hiding this comment.
[SUGGESTION] The new argument contract is only enforced on one of the two public analysis entry points, so "validate public analysis arguments uniformly" is not quite achieved.
compute_gdn_decay_horizons is exported alongside analyze_gdn_decay (__all__ at policy.py:43, re-exported from state_sparsity/__init__.py) and takes the same epsilon/static_gate_input kwargs, but keeps the weaker checks:
compute_gdn_decay_horizons(a, b, epsilon=[])→TypeErrorfrom0.0 < [], not theValueErrorthis PR standardizes on.epsilon=float("nan")→ValueError("epsilon must be in (0, 1)"), a different message for the same defect (two strings to keep in sync).static_gate_inputis not validated at all, sostatic_gate_input=float("nan")falls through to the horizon check and reports"GDN decay parameters produced non-finite or non-positive horizons"— i.e. it blames the model's decay tensors for a bad caller argument, which is exactly the failure modetest_analysis_rejects_invalid_static_gate_input_at_the_public_boundarywas added to prevent on the sibling API.
Extracting one helper keeps a single source of truth and closes the gap:
def _validate_analysis_arguments(epsilon: float, static_gate_input: float) -> None:
"""Normalize invalid public analysis arguments to the ValueError contract."""
if not isinstance(epsilon, float | int) or not (math.isfinite(epsilon) and 0.0 < epsilon < 1.0):
raise ValueError("epsilon must be finite and in (0, 1)")
if not isinstance(static_gate_input, float | int) or not math.isfinite(static_gate_input):
raise ValueError("static_gate_input must be finite")Then call it at the top of both compute_gdn_decay_horizons and analyze_gdn_decay (replacing lines 118-119 and 240-251).
Secondary note on the current form: isinstance is the idiom used a few lines below for decay_parameter_storage_dtype, and it is also tighter than try/except TypeError — math.isfinite does not raise TypeError for every non-scalar input (a multi-element tensor raises a different exception type from __float__), so the except TypeError normalization has holes that an explicit type check does not.
There was a problem hiding this comment.
Fixed in #2395. Both exported entry points now call one _validate_analysis_arguments helper before tensor/model work, and direct compute regressions cover invalid epsilon and static_gate_input values. The explicit scalar type check also avoids exception-type holes from tensor-like inputs.
| try: | ||
| validate_dasc_model_structure(model, policy) | ||
| except _DASCModelStructureMismatchError as error: | ||
| except _DASCRecoverableStalenessError as error: |
There was a problem hiding this comment.
[SUGGESTION] Widening this except to the new base class is the right call for save/restore symmetry, and fail-closed is preserved for the hard cases (_get_gdn_modules still raises a plain ApplyModeError for "no supported GDN modules" and for unconverted subclasses, so neither is swallowed here). One follow-up on the user-facing guidance:
For the newly recoverable case, the advice is wrong. _DASCDecayParametersUnavailableError fires when a supported GDN identity is present but A_log/dt_bias are temporarily unavailable — parameters flattened by a sharding wrapper, or a model not yet materialized off the meta device. The remedy there is to materialize the parameters and re-save/re-validate, not to recalibrate: the policy itself is still valid for this checkpoint, and re-running calibrate() on a model whose decay tensors are missing fails with the same error. Concatenating "re-run calibrate() before deployment" onto that message (line 103, and again at line 117) sends users down a dead end.
Since both handlers now cover two distinct conditions, consider making the remedy condition-specific, e.g.:
_STALENESS_REMEDIES = {
_DASCDecayParametersUnavailableError: (
"materialize the GDN decay parameters before saving or deploying"
),
_DASCModelStructureMismatchError: "re-run calibrate() before deployment",
}and formatting the warning with _STALENESS_REMEDIES[type(error)]. The same distinction applies to the guide text added in docs/source/guides/6_sparsity.rst — "export_policy rejects it until recalibration" is accurate for geometry/decay drift, but recalibration is not the fix for temporarily unavailable tensors.
Claude review — DASC analysis and restore boundariesScope: full review (the trigger comment carried no scoping instructions). All 4 changed files reviewed (83 additions / 16 deletions): Findings: CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 3 What I verified
Most impactful suggestions
Risk: low. Tightly scoped hardening of an unreleased feature branch, additive exception hierarchy, no public signature/default/state-schema changes, and each new behavior has a test. All three findings are non-blocking quality points. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
✅ Action performedFull review finished. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/dasc-state-sparsity-review-contract #2394 +/- ##
============================================================================
Coverage 78.78% 78.78%
============================================================================
Files 548 548
Lines 64227 64243 +16
============================================================================
+ Hits 50599 50615 +16
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 Addresses the two follow-up findings on #2394: - use one argument validator for both exported analysis entry points - normalize invalid epsilon and static gate inputs to ValueError before tensor/model work - remove the redundant validation hoist and retain the existing pre-cast check only where a storage cast occurs - add direct public-API regressions for `compute_gdn_decay_horizons` ## Validation - `python -m pytest -q tests/unit/torch/sparsity/state_sparsity/test_dasc.py` (36 passed, 1 optional Megatron skip) - pre-commit hooks on both changed files - signed commit with DCO sign-off <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved validation for sparsity analysis inputs, including finite numeric values and supported epsilon ranges. - Ensured invalid decay-tensor values are detected consistently across analysis workflows. - Standardized error handling for direct decay-horizon calculations. - **Tests** - Added coverage for invalid epsilon and static gate input values, including verification of consistent error messages. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: kaix-nv <kaix@nvidia.com>
b453370
into
feature/dasc-state-sparsity-review-contract
1 participant
Summary
Addresses the remaining review findings on #2389:
Validation
python -m pytest -q tests/unit/torch/sparsity/state_sparsity/test_dasc.py(34 passed, 1 optional Megatron skip)python -m pytest -q tests/unit/torch/sparsity/state_sparsity tests/unit/torch/sparsity/weight_sparsity tests/unit/torch/sparsity/attention_sparsity(312 passed, 1 optional Megatron skip)pre-commit run --files ...on all changed filesSummary by CodeRabbit
Bug Fixes
Documentation