Conversation
Signed-off-by: kaix-nv <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; 0 remain after this review. 📝 WalkthroughWalkthroughThe DASC lifecycle test now exports the calibrated policy while the ambient device is ChangesDASC policy export validation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Other Merge Risk: ⚪ Minimal · up to This test-only change improves coverage of meta-device policy export without altering production behavior, and validation checks passed. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
/claude review |
|
@coderabbitai full review |
|
|
@coderabbitai full review |
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| state = mto.modelopt_state(calibrated) | ||
| policy = mtss.export_policy(calibrated) | ||
| assert state["modelopt_state_dict"][0][0] == "dasc" | ||
| assert policy["layers"]["linear_attn"]["static_horizons"] |
There was a problem hiding this comment.
[SUGGESTION] This assertion can never fail on its own, so it carries less signal than it looks like it does.
DASCLayerPolicy.static_horizons is declared list[float] = Field(min_length=1) and validate_partition additionally requires len(static_horizons) == num_heads with every value finite and > 0.0 (modelopt/torch/sparsity/state_sparsity/config.py:215-225). By the time export_policy() returns a dumped policy, a non-empty list is guaranteed by the schema — a truthiness check is tautological beyond confirming the layers/linear_attn/static_horizons key path exists and that export_policy() didn't raise.
That said, the placement is the valuable part and it is correct: every horizon path pins CPU explicitly (policy.py:121-122, 214-215, 331-332, 523), so if a future change dropped one of those pins, the horizons/bounds tensors would materialize on meta and either the CPU-vs-meta comparison in validate_dasc_decay_parameters or the .tolist() in the dump would raise inside the torch.device("meta") block. This does close the gap the PR describes.
To make the assertion itself meaningful, tie the exported horizons to values computed on the normal CPU path — that catches a silently-wrong-but-non-empty policy, not just an exception:
def test_policy_lifecycle_ignores_the_default_device():
"""Keep calibration and checkpoint metadata validation on their declared CPU path."""
model = TinyGatedDeltaNetForCausalLM()
expected_horizons = mtss.analyze_gdn_decay(model)["linear_attn"].tolist()
with torch.device("meta"):
calibrated = mtss.calibrate(model, _config(wmax_candidates=[7]), [_candidate(7)])
state = mto.modelopt_state(calibrated)
policy = mtss.export_policy(calibrated)
assert state["modelopt_state_dict"][0][0] == "dasc"
assert policy["layers"]["linear_attn"]["static_horizons"] == pytest.approx(expected_horizons)Non-blocking — the current form is a net improvement over not calling export_policy() under the ambient device at all.
There was a problem hiding this comment.
Thanks. No further code change here: the load-bearing guard is that export_policy() executes inside the meta default-device context and propagates any validation failure. The truthiness check is retained only as a structural/schema assertion; exact horizon numerics and storage-canonical equality are already covered by dedicated tests, so duplicating that comparison here would mix concerns.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Claude review summary Findings: CRITICAL: 0 · IMPORTANT: 0 · SUGGESTION: 1 Scope Full review. The PR changes exactly one file ( Assessment The change does what the description claims. Moving The No mode-registration, config-schema, public-API, or export-path surface is touched; The one suggestion (non-blocking)
Risk Low. Test-only change that strictly tightens an existing regression; no production code paths affected. 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
431bb04
into
feature/dasc-state-sparsity-review-policy-device
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/dasc-state-sparsity-review-policy-device #2400 +/- ##
=================================================================================
Coverage 68.75% 68.75%
=================================================================================
Files 548 548
Lines 64244 64244
=================================================================================
Hits 44173 44173
Misses 20071 20071
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:
|
1 participant
Summary
Addresses the sole non-blocking suggestion on approved PR #2399:
export_policy()under the non-CPU ambient default-device contextValidation
Summary by CodeRabbit
metadevice.