Skip to content

test: direct unit suite for DistillationModel (37 tests)#1923

Closed
arham766 wants to merge 1 commit into
NVIDIA:mainfrom
arham766:tests/distillation-model
Closed

test: direct unit suite for DistillationModel (37 tests)#1923
arham766 wants to merge 1 commit into
NVIDIA:mainfrom
arham766:tests/distillation-model

Conversation

@arham766

@arham766 arham766 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Type of change: new tests

Part of the unit-coverage initiative in #1902. Direct unit suite for modelopt/torch/distill/distillation_model.py (+registry.py) using tiny hand-weighted Linear stacks so every activation and loss is exact in fp32 — the existing distill tests (timm-based, currently uncollectable without timm) assert scalar-ness and key presence, not values. Covers: convert-path construction (teacher instance vs ModelLike class), per-layer-pair capture hooks (exact tensor values, student-grad/teacher-no-grad asymmetry, overwrite semantics, eval-mode warning asymmetry), compute_kd_loss (exact two-pair balanced total 0.25·1+0.25·4+0.5·2=2.25; skip_balancer dict contents; captures cleared), teacher eval re-forcing and frozen params, only_teacher/only_student contexts incl. return values, state-dict key equality with a plain student + value restoration, export() class restore and hook cleanup, and error paths. Adversarial review: 14 derivations verified, 4/4 mutations killed including a pair-ordering probe. Follow-up finding: export() removes hooks but leaves stale _intermediate_output attributes pinning the last captured activations (incl. an autograd graph) on both student and teacher layers.

Usage

N/A — tests only.

Testing

Hermetic, CPU-only, deterministic, <2s. Combined run with the sibling calib/distill suites green; full tests/unit/torch/quantization dir unaffected. Adversarially reviewed with mutation testing as described above.

Before your PR is "Ready for review"

  • Is this change backward compatible?: ✅
  • If you copied code from any other sources or added a new PIP dependency, did you follow guidance in CONTRIBUTING.md: N/A
  • Did you write any new necessary tests?: ✅
  • Did you update Changelog?: N/A
  • Did you get Claude approval on this PR?: N/A (external contributor)

Additional Information

Issue: #1902

CPU-only tests with tiny hand-weighted Linear stacks so every
activation and loss is exact in fp32 - the existing distill tests
build on timm models and assert scalar-ness, not values. Covers
convert-path construction, per-layer-pair capture hooks (values,
grad_fn asymmetry, clearing), compute_kd_loss with exact two-pair
balanced totals and skip_balancer contents, teacher eval re-forcing,
fwd-only context managers, state-dict key equality and value
restoration, export hook cleanup, and error paths. Adversarially
reviewed: 4/4 seeded mutations killed (incl. a pair-ordering probe).

Also surfaces that export() leaves stale _intermediate_output
attributes pinning the last captured activations - reported for
follow-up.

Part of the coverage initiative in NVIDIA#1902.

Signed-off-by: arham766 <arhamislam766@yahoo.com>
@arham766 arham766 requested a review from a team as a code owner July 6, 2026 02:38
@copy-pr-bot

copy-pr-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@arham766, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c4dbebcb-b920-4802-a967-a04cefcdd2c8

📥 Commits

Reviewing files that changed from the base of the PR and between b96a785 and c20adb0.

📒 Files selected for processing (1)
  • tests/unit/torch/distill/test_distillation_model.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@arham766

arham766 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Consolidated into #1927 per maintainer feedback in #1902 — the suite was trimmed to only the lines codecov reports uncovered, with parametrization clusters deduplicated. Closing in favor of that PR.

@arham766 arham766 closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant