Preserve DASC numeric argument compatibility - #2396
Conversation
Signed-off-by: kaix-nv <kaix@nvidia.com>
|
/claude review |
|
@coderabbitai full review |
|
Warning Review limit reachedNext included review available in 30 seconds. View limit detailsLimit details: You’ve used all 12 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
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 (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change centralizes state sparsity argument validation, reuses shared defaults, creates horizon epsilon tensors on CPU, and expands tests for invalid and tensor scalar inputs. ChangesState sparsity validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The shared validation and policy integration changes have no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| try: | ||
| epsilon_is_valid = math.isfinite(epsilon) and 0.0 < epsilon < 1.0 | ||
| except (TypeError, ValueError, OverflowError): | ||
| 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, ValueError, OverflowError): | ||
| 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.
[IMPORTANT Compatibility] The normalized exception set misses RuntimeError, which is exactly what a non-scalar tensor raises — so the public ValueError contract this PR is consolidating now leaks for a whole input class that the previous version rejected cleanly.
What changed: the version removed from policy.py gated on isinstance(epsilon, float | int) first, so any non-float/int argument (including every torch.Tensor) took the ValueError path. This one drops the isinstance gate and relies on math.isfinite raising a normalizable exception instead. That holds for lists/strings/complex (TypeError), numpy arrays (TypeError), and oversized ints (OverflowError) — but math.isfinite reaches a tensor's __float__, and torch raises RuntimeError: a Tensor with N elements cannot be converted to Scalar for any tensor whose numel() != 1 (including empty tensors). RuntimeError is not in the except tuple, so it propagates unchanged out of mtss.analyze_gdn_decay and mtss.compute_gdn_decay_horizons.
Why it matters: the new test_analysis_accepts_tensor_scalar_arguments deliberately makes 0-d tensors a supported input, which makes epsilon=torch.tensor([1e-3, 2e-3]) (or a stray A_log-shaped tensor) a very plausible caller mistake rather than an exotic one. Callers that follow the documented contract and wrap these entry points in except ValueError will crash instead of surfacing the intended message — a regression relative to the base branch, on the exact axis this PR is about. 0.0 < epsilon < 1.0 and the if not epsilon_is_valid bool conversion have the same exposure for multi-element tensors.
Fix: add RuntimeError to both handlers (a targeted follow-up test with a 2-element tensor would lock it in):
| try: | |
| epsilon_is_valid = math.isfinite(epsilon) and 0.0 < epsilon < 1.0 | |
| except (TypeError, ValueError, OverflowError): | |
| 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, ValueError, OverflowError): | |
| static_gate_input_is_valid = False | |
| if not static_gate_input_is_valid: | |
| raise ValueError("static_gate_input must be finite") | |
| try: | |
| epsilon_is_valid = math.isfinite(epsilon) and 0.0 < epsilon < 1.0 | |
| except (TypeError, ValueError, OverflowError, RuntimeError): | |
| 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, ValueError, OverflowError, RuntimeError): | |
| 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.
Fixed in #2397 with a stricter scalar boundary rather than catching arbitrary framework RuntimeError. The annotated inputs now require numbers.Real; NumPy real scalars remain accepted, while scalar and multi-element tensors are rejected uniformly with ValueError before math.isfinite. Multi-element regressions cover both public entry points.
| def test_analysis_accepts_tensor_scalar_arguments(): | ||
| """Preserve support for real-like scalar values accepted by the numeric operations.""" | ||
| horizons = mtss.compute_gdn_decay_horizons( | ||
| torch.tensor([0.0]), | ||
| torch.tensor([0.0]), | ||
| epsilon=torch.tensor(1e-3), # type: ignore[arg-type] | ||
| static_gate_input=torch.tensor(-0.3), # type: ignore[arg-type] | ||
| ) | ||
| assert horizons.shape == (1,) | ||
| assert torch.isfinite(horizons).all() | ||
|
|
There was a problem hiding this comment.
[SUGGESTION] This test promotes tensor scalars from "happens to work" to a checked-in contract, but the signatures it exercises still annotate epsilon: float / static_gate_input: float — hence the two # type: ignore[arg-type]. A checked-in test asserting behavior the public annotation contradicts is a maintenance trap: the next contributor who tightens validation (e.g. reinstating an isinstance gate, or switching to numbers.Real as the PR description considered) will read the annotation, make the change, and only discover the intended guarantee from a CI failure with no stated rationale.
Worth resolving one way or the other: either widen the annotation to what is actually accepted (float | int | torch.Tensor) and say so in the Args: of the two public docstrings, or drop this test and treat 0-d-tensor support as incidental. The former is probably right given the PR is deliberately preserving duck-typed scalar compatibility — a one-line docstring note is what makes the type: ignore pair legible to a future reader.
There was a problem hiding this comment.
Agreed and fixed in #2397. Tensor support was inconsistent with the public float annotations, so the tensor-scalar acceptance test and as_tensor change are removed. numbers.Real now makes the intended scalar contract explicit at runtime.
There was a problem hiding this comment.
Claude review — 1 IMPORTANT, 1 SUGGESTION
Scope: full review (trigger comment carried no scoping instructions). All 3 changed files reviewed: modelopt/torch/sparsity/state_sparsity/config.py, .../policy.py, tests/unit/torch/sparsity/state_sparsity/test_dasc.py (+47/-18).
Findings: CRITICAL: 0 · IMPORTANT: 1 · SUGGESTION: 1
Most impactful finding
[IMPORTANT Compatibility] _validate_analysis_arguments normalizes TypeError/ValueError/OverflowError but not RuntimeError — inline.
The version this replaces gated on isinstance(epsilon, float | int) first, so every non-float/int argument took the ValueError path. Dropping that gate in favor of "let math.isfinite raise something normalizable" covers lists, strings, complex, numpy arrays (TypeError) and oversized ints (OverflowError), but not tensors: math.isfinite reaches Tensor.__float__, and torch raises RuntimeError: a Tensor with N elements cannot be converted to Scalar whenever numel() != 1. That escapes both except tuples and propagates out of mtss.analyze_gdn_decay / mtss.compute_gdn_decay_horizons unchanged.
This matters more than it would have before the PR, because test_analysis_accepts_tensor_scalar_arguments makes 0-d tensors a supported input — so passing an A_log-shaped tensor by mistake is now a near-miss of a supported call, not an exotic one, and it breaks the documented ValueError contract on the exact axis this PR consolidates. Adding RuntimeError to both handlers is a two-line fix.
Basis: torch's documented scalar-conversion behavior and the removed isinstance gate, reasoned from the diff — I did not execute it in this environment (the verification command needed approval).
What looks right
- Consolidating epsilon / static-gate validation into
config.pyis a clean single source of truth, and the module-level_DEFAULT_*constants keep the field defaults and the helper defaults from drifting. - The new signature stays positional-compatible with the two existing
_validate_analysis_arguments(epsilon, static_gate_input)call sites inpolicy.py(lines 119, 238) — no silent keyword breakage. torch.as_tensor(epsilon, device="cpu", dtype=torch.float64)is the right fix for the copy-construct warning, and correctly preserves the CPU-float64 horizon math.mathremains used in both files (config.pyhelper,policy.py:335), so no dead import from the move.- Duck-typing over
numbers.Realis well justified in the PR body:numbers.Realwould have excluded 0-d tensors and narrowed the existing API.
Risk
Low. Small, well-scoped change to argument validation on two public entry points, with no mode-registration, modelopt_state, config-schema, or export surface touched — DASCConfig field defaults are unchanged in value, so serialized-config compatibility is intact. The one IMPORTANT finding is a narrow contract leak with a two-line fix, not a behavior or numerics regression.
🤖 Generated with Claude Code
![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-public-api #2396 +/- ##
==================================================================================
+ Coverage 68.75% 78.78% +10.03%
==================================================================================
Files 548 548
Lines 64232 64243 +11
==================================================================================
+ Hits 44163 50615 +6452
+ Misses 20069 13628 -6441
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 both Claude findings on #2396 while retaining CodeRabbit’s oversized-integer fix: - match the annotated scalar-float API by accepting `numbers.Real` inputs, including NumPy real scalars - reject all tensor objects before scalar conversion, so multi-element tensors cannot leak RuntimeError - keep OverflowError normalization for oversized integer inputs - remove the tensor-scalar compatibility claim and restore the original scalar conversion - cover multi-element tensors through both exported public entry points ## Validation - focused DASC suite: 46 passed, 1 optional Megatron skip - pre-commit hooks on all three 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 decay-analysis inputs by requiring real-number values for epsilon and static gate parameters. - Invalid non-numeric and tensor-based inputs are now rejected consistently, including zero-dimensional tensor values. - Improved numerical consistency when calculating decay horizons by preserving CPU double-precision behavior. - Added clearer safeguards against invalid or unsupported input types before analysis calculations are performed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: kaix-nv <kaix@nvidia.com>
c318a99
into
feature/dasc-state-sparsity-review-public-api
1 participant
Summary
Addresses all remaining suggestions and the CodeRabbit oversized-integer finding on #2395:
config.pytorch.as_tensorfor scalar epsilon conversion without copy-construction warningsnumbers.Realwas considered, but it excludes PyTorch scalar tensors (and Decimal) on the validated host, so guarded numeric operations preserve the previous API more faithfully.Validation
Summary by CodeRabbit
Bug Fixes
Tests