Repository navigation
Conversation
local_hessian_calibrate ran max_calibrate's forward before its own Hessian forward even when no activation quantizer needs a data-driven amax (weight-only, constant-amax or dynamic activations), so that forward did nothing. It now opts into skip_forward_without_activation_calib, as GPTQ and imatrix do: the forward still runs whenever an activation quantizer needs it, and weight calibration runs on the weights either way. Weight-only local Hessian on phi-2 (128 x 512 tokens) runs one forward instead of two: max calibration 3.7 s -> 0.35 s, the whole call about 58 s -> 53 s. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Conditional max-calibration forward modelopt/torch/quantization/model_calib.py, tests/unit/torch/quantization/test_local_hessian.py |
local_hessian_calibrate passes skip_forward_without_activation_calib=True to max calibration. The test checks one forward-loop call for weight-only quantization and two when a static input-amax quantizer is configured. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Feature
Possibly related PRs
- NVIDIA/Model-Optimizer#2864: Adds the
skip_forward_without_activation_caliboption that this change enables in local Hessian calibration.
Merge Risk: 
c3aba
-
No merge-blocking correctness or calibration risk is established by the change.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: skipping the local Hessian max-calibration forward when activation calibration is not required. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Security Anti-Patterns PASS. The PR changes only modelopt/torch/quantization/model_calib.pyand a unit test. Added Python lines contain notorch.load(..., weights_only=False), unsafenumpy.load, hardcoded `trust_remot…Full details: Security Anti-Patterns
-
Explanation
PASS. The PR changes only
modelopt/torch/quantization/model_calib.pyand a unit test. Added Python lines contain notorch.load(..., weights_only=False), unsafenumpy.load, hardcodedtrust_remote_code=True,eval/exec, or# nosec. Nopyproject.tomlor requirements file changed, so no new dependency license issue is introduced.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3042 +/- ##
==========================================
- Coverage 79.87% 79.33% -0.54%
==========================================
Files 661 661
Lines 75417 75417
==========================================
- Hits 60237 59830 -407
- Misses 15180 15587 +407
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:
|
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
Nudge: exact-SHA verification is unavailable, and the ZeRO-3 compatibility claim needs evidence before the unconditional skip is merge-ready.
Needs action:
- Confirm the review against base
278953c31c06b6b2170316e801b910c079cb90d0and headc3abac5e6f511ec383780f15df5e68a61203c613; the available file tools expose an unidentified PR-branch snapshot, not SHA-pinned content. - Validate ZeRO-3 weight-amax initialization and refinement at
model_calib.py:1225, or retain the max forward for that path; the later Hessian forward runs after max-stat collection ends.
No action needed:
- No confirmed defect found in the supplied diff and accessible source; the change reuses the existing guard and adds meaningful forward-count coverage.
- Review tag:
fresh-20261028T202642Z/PR-3042.
|
Closing this. The skip only applies to weight-only |
What does this PR do?
Type of change: new feature (performance)
local_hessian_calibraterunsmax_calibratefirst, then its own forward to collect the per-block Hessians. Its weight search needs no forward at all. So when no activation quantizer needs a data-driven amax, the max-calibration forward does nothing: weight-only recipes, constant-amax activations, and dynamic or MX activations all fall in that case. It now passesskip_forward_without_activation_calib=True, as GPTQ (#2864) and imatrix already do._needs_activation_forward_for_max_calibdecides. Weight quantizers are calibrated on the weights directly either way.forward_loopmust do, such as materializing ZeRO-3 shards.local_hessianrecipes are W4A4 NVFP4. Their activation quantizers need a data-driven global amax, so they still run both forwards and are unchanged. That includesnvfp4_act_headroomwithweight_scale_algorithm: local_hessian, whose activation statistics come from that very forward. The skip helps weight-only, experts-only constant-amax and dynamic-activationlocal_hessiansetups.Usage
No API change.
Testing
phi-2, weight-only static NVFP4 with
local_hessian, 128 x 512 calibration tokens, single RTX PRO 6000 (shared workstation, so five runs each):forward_loopcallsmax_calibratemtq.quantizemainThe weight search takes about 48-50 s of either run, so the forward saved is about 3.4 s here. A layerwise run on a large MoE saves one full forward of each layer over all calibration data.
New CPU test
test_max_calibration_runs_a_forward_only_for_activation_amax, which countsforward_loopcalls. A weight-only config runs one forward, and fails with two onmain. A static input amax still runs two.tests/unit/torch/quantization/test_local_hessian.py: 13 passed.tests/gpu/torch/quantization/test_nvfp4_fp8_sweep_kernel.py(the local-Hessian kernel and end-to-end tests): 45 passed. Local-Hessian, max and fused-expert tests intest_calib.py,plugins/test_fused_experts.pyandtests/unit/recipe: 156 passed.Before your PR is "Ready for review"
CONTRIBUTING.md: N/A🤖 Generated with Claude Code
Summary by CodeRabbit