Conversation
Each calibration algorithm now states what it reads, writes and assumes as an `AlgoCapabilities` on its `BaseCalibrateModeDescriptor` subclass: `writes_whole_module`, `refines`, `requires`, `may_write`, `invalid_if_present` and `scopable`. They live on the descriptor rather than a side table keyed by algorithm name. `CalibrateModeRegistry` is already the one-object-per-algorithm registry, so a second dict would be a parallel registry that can fall out of sync -- and an algorithm registered by a user, the documented extension point, would get `None` from such a lookup. On the descriptor it inherits a conservative default instead. `capabilities_for(algo, cfg)` derives them from the algorithm's own kwargs where they genuinely vary: `lsq` and `nvfp4_act_headroom` delegate weight scales to a configurable sub-algorithm, and that algorithm's `requires` and `may_write` become theirs. This subsumes `QuantizeAlgorithmConfig._mutates_weights`, a hand-maintained ClassVar overridden on four config classes that said exactly what `WEIGHT in may_write` says. Two statements of one fact drift, and a new algorithm only has to forget one of them -- understating it makes layerwise calibration skip the weight write-back and silently discard the algorithm's results. `layerwise.calib_mutates_weights` becomes `bool | None` (None = derive): `True` is always safe and merely costs I/O, `False` is safe only if the algorithm does not write weights, so there is no user preference here, only a right answer per algorithm. It remains as an explicit opt-out for amax-only algorithms, and an explicit `False` on a weight-writing one is now rejected at config time. Also fixes `MseCalibrator.reset()`, which deleted `_initial_amax`. That field is set only in `__init__` and never repopulated, so the "reset" left the instance permanently unusable: `algorithm=['max','mse','max']` crashes on main today because the next stage to collect stats re-enters the spent calibrator. `NVFP4MSECalibrator.reset()` already kept it and documented why; the base class now matches. The amax clone it retains is scalar or `[out_features]`, so the memory argument for dropping it did not apply to the class that was doing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fridah-nv <201670829+Fridah-nv@users.noreply.github.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Contributor
|
Contributor
Author
|
Superseded by #2534 — same commit, branch renamed to the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2530 +/- ##
==========================================
+ Coverage 68.89% 68.92% +0.03%
==========================================
Files 605 606 +1
Lines 67063 67129 +66
==========================================
+ Hits 46204 46270 +66
Misses 20859 20859
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Type of change: refactor + bug fix
Stack 1 of 4, splitting #2292 (scoped calibration pipelines) into independently reviewable pieces. This one lands nothing user-facing and is useful on its own.
algo_cfgDeclare what each calibration algorithm does
Each algorithm now states what it reads, writes and assumes as an
AlgoCapabilitieson itsBaseCalibrateModeDescriptorsubclass:writes_whole_module,refines,requires,may_write,invalid_if_present,scopable.They live on the descriptor rather than in a side table keyed by algorithm name.
CalibrateModeRegistryis already the one-object-per-algorithm registry, so a second dict would be a parallel registry that can fall out of sync — and an algorithm registered by a user, the documented extension point, would getNonefrom such a lookup. On the descriptor it inherits a conservative default instead.capabilities_for(algo, cfg)derives them from the algorithm's own kwargs where they genuinely vary:lsqandnvfp4_act_headroomdelegate weight scales to a configurable sub-algorithm, and that algorithm'srequiresandmay_writebecome theirs.This replaces
_mutates_weightsQuantizeAlgorithmConfig._mutates_weightswas a hand-maintained ClassVar overridden on four config classes that said exactly whatWEIGHT in may_writesays. Two statements of one fact drift, and a new algorithm only has to forget one of them — understating it makes layerwise calibration skip the weight write-back and silently discard the algorithm's results.layerwise.calib_mutates_weightsbecomesbool | None(None = derive).persistent_materialization(writeback=...)only controls whether weights are copied back, soTrueis always safe and merely costs I/O, whileFalseis safe iff the algorithm does not write weights. There is no user preference there, only a right answer per algorithm — so the field remains as an explicit opt-out for amax-only algorithms, and an explicitFalseon a weight-writing one is now rejected at config time.Also fixes
MseCalibrator.reset()_initial_amaxis set only in__init__and never repopulated, so deleting it inreset()did not reset the instance — it destroyed it. This reproduces onmaintoday:algorithm=['max','mse','max']crashes, because the next stage to collect stats re-enters the spent calibrator.NVFP4MSECalibrator.reset()already kept it and documented why; the base class now matches. The memory argument for dropping it does not apply to the class that was doing it — the base calibrator's amax clone is scalar or[out_features], while the large[num_blocks]clone belongs to the subclass that retains it.Testing
New
tests/unit/torch/quantization/test_algo_capabilities.py, plus a regression test for the calibrator fix (mutation-verified: restoring thedelfails it).pytest tests/unit/torch/quantization/→ 1128 passed, 8 skipped. Two pre-existing failures inplugins/test_huggingface.py::test_quantized_transformers_save_restore(a transformers-version issue, reproduced on a cleanmainworktree) and one pre-existing collection error intest_diffusers_wan_conv3d.py.Before your PR is "Ready for review"
calib_mutates_weightskeeps its old meaning when set explicitly; unset now derives instead of defaulting to a hand-maintained flag.CONTRIBUTING.md: N/A🤖 Generated with Claude Code