Remove deprecations marked in 0.45 and 0.46 - #2182
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughVersion 0.47 standardizes quantization around recipes and nested layerwise configuration. It removes deprecated AutoQuantize flags, aliases, and compatibility paths. It adds NVFP4 and MXFP4 MLP/MoE weight-only recipes and updates examples, tests, and documentation. ChangesQuantization modernization
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to This PR removes deprecated interfaces and updates documented replacements and tests; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/hf_ptq/scripts/huggingface_example.sh`:
- Around line 97-99: Update the AutoQuantize checkpoint-generation condition to
inspect the loaded recipe’s type rather than matching the RECIPE path string.
Use the recipe data produced by load_recipe() to identify AutoQuantize recipes,
while preserving the existing AUTO_QUANTIZE_CHECKPOINT-empty guard.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5573ac1c-1d47-4362-bc5f-f27a40aad3a0
📒 Files selected for processing (40)
CHANGELOG.rstexamples/gpt-oss/README.mdexamples/gpt-oss/configs/sft_full.yamlexamples/gpt-oss/configs/sft_lora.yamlexamples/gpt-oss/sft.pyexamples/hf_ptq/README.mdexamples/hf_ptq/example_utils.pyexamples/hf_ptq/hf_ptq.pyexamples/hf_ptq/scripts/huggingface_example.shexamples/hf_ptq/scripts/parser.shexamples/llm_ptqexamples/llm_qat/ARGUMENTS.mdexamples/llm_qat/quantize.pyexamples/megatron_bridge/README.mdexamples/vlm_ptq/.gitignoreexamples/vlm_ptq/README.mdexamples/vlm_ptq/scripts/huggingface_example.shmodelopt/recipe/config.pymodelopt/recipe/presets.pymodelopt/torch/quantization/backends/nvfp4_gemm.pymodelopt/torch/quantization/config.pymodelopt/torch/quantization/plugins/transformers_trainer.pymodelopt/torch/utils/dataset_utils.pymodelopt_recipes/general/auto_quantize/w4a16_nvfp4_fp8_at_6p0bits-active_moe.yamlmodelopt_recipes/general/ptq/mxfp4_mlp_weight_only.yamlmodelopt_recipes/general/ptq/nvfp4_mlp_weight_only.yamlmodelopt_recipes/huggingface/qwen3_6_moe/auto_quantize/w4a16_nvfp4_fp8_at_6p0bits-active_moe.yamlmodelopt_recipes/ptq.mdplugins/modelopt/skills/deployment/references/trtllm.mdplugins/modelopt/skills/deployment/scripts/deploy.shtests/_test_utils/torch/quantization/offload.pytests/examples/gpt-oss/test_gpt_oss_qat.pytests/examples/hf_ptq/test_hf_ptq_args.pytests/examples/hf_ptq/test_llm_ptq.pytests/examples/hf_ptq/test_vlm_ptq.pytests/gpu/torch/export/test_unified_hf_export_and_check_safetensors.pytests/gpu/torch/quantization/plugins/test_accelerate_gpu.pytests/gpu/torch/quantization/test_gptq.pytests/unit/recipe/test_presets.pytests/unit/torch/quantization/test_config_validation.py
💤 Files with no reviewable changes (7)
- examples/vlm_ptq/.gitignore
- examples/vlm_ptq/scripts/huggingface_example.sh
- examples/vlm_ptq/README.md
- modelopt/recipe/config.py
- examples/llm_ptq
- tests/examples/hf_ptq/test_hf_ptq_args.py
- tests/unit/recipe/test_presets.py
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Mechanical, well-scoped deprecation removal (0.45/0.46), mostly deletions with the surviving APIs' tests migrated in place. I checked for dangling references at the PR branch: examples/llm_ptq / examples/vlm_ptq are gone with the two skill docs and dataset_utils.py comment updated; no remaining consumers of QFORMAT_ALIASES, AUTOQUANT_BASE_*, _auto_quantize_config_from_cli, QuantizationArgumentsWithConfig, quant_args.quant_cfg, use_sequential, or layerwise_checkpoint_dir (the int8_sq hits left in modelopt/torch/export/model_config.py, docs/.../1_tensorrt_llm.rst, tests/gpu/torch/export/test_export.py and the gemma recipe filename are TRT-LLM format names / recipe basenames, not the removed CLI alias). load_config is correctly dropped from modelopt/recipe/config.py, MaxCalibConfig rejects all three legacy forms via extra="forbid", and the new YAMLs carry the standard NVIDIA header (year differs from LICENSE_HEADER, same as every existing recipe — not a licensing concern).
Design-review note: this PR removes abstractions rather than adding any; the two new files reuse the existing modelopt_recipes composition mechanism, so no second system is introduced. The one design-adjacent smell is that each new general/ptq/*_mlp_weight_only.yaml is a verbatim copy of the same-named preset under configs/ptq/presets/model/, with nothing pinning the equality the PR body says was verified by hand.
Findings (all small, none blocking): one now-dead "sq" in args.qformat heuristic that silently changes calibration batch-size probing for SmoothQuant, the un-pinned recipe/preset duplication, and a few stale comments/changelog omissions. Details inline.
|
Pushed 8736546 addressing review feedback, plus a CI unit-test failure. CI failure (not from review —
Review comments — 5 addressed, 1 pushed back
The For the parity concern I added Verified locally: 366 tests pass across Still unrun locally: the GPU suites whose preset/layerwise literals this PR updates ( |
|
/claude review |
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-review of PR #2182 (0.45/0.46 deprecation removal, 41 files, +234/-564). All previously-flagged items are resolved in 8736546:
- Critical — dead
"sq" in args.qformatheuristic: fixed, now"smoothquant" in args.qformatinquantize_main, so SmoothQuant calibration keeps the 2x memory headroom in the auto batch-size probe. (awqhalf still matchesw4a8_awq_beta/nvfp4_awq_lite.) - Critical — new
general/ptq/*_mlp_weight_only.yamlduplicated the shipped presets with nothing pinning parity: fixed viatest_mlp_weight_only_recipe_matches_its_mtq_cfgintests/unit/recipe/test_presets.py, parametrized over both recipes, comparingload_recipe(...).quantize.model_dump(exclude_unset=True)toQuantizeConfig(**mtq.<CFG>).model_dump(exclude_unset=True). I confirmed the recipes are byte-for-byte equivalent toconfigs/ptq/presets/model/{mxfp4,nvfp4}_mlp_weight_only.yaml(same imports, same two quantizer entries, samealgorithm), and that the gpt-oss notebook still usesmtq.MXFP4_MLP_WEIGHT_ONLY_CFGdirectly — so the interchangeability the README claims is now enforced by a test. Option (b) over (a) is a reasonable call given the two files use different schemas. - Critical (CI) —
test_layerwise_calibrate.pystill passed the removedlayerwise: Truebool form: fixed in four call sites; the only survivinglayerwise: True/use_sequentialuses are the deliberate negatives inTestLayerwiseNestedConfig::test_legacy_forms_rejected. - Minor — stale
--auto_quantize_*comment dropped,_coerce_layerwisedocstring corrected, CHANGELOG now states the restore-time consequence (extra="forbid"rejects pre-0.45use_sequential/layerwise_checkpoint_diron restore). - Minor, pushed back — the
[[ "$RECIPE" == *auto_quantize* ]]glob inhuggingface_example.shpredates this PR (only theAUTO_QUANTIZE_BITSclause was removed); CodeRabbit withdrew the finding and the worst case is an unused checkpoint path. Acceptable to defer.
Spot checks at the branch: QFORMAT_ALIASES, load_quant_cfg_choices(aliases=...), AUTOQUANT_BASE_*, _load_layer_pattern_list, _auto_quantize_config_from_cli, QuantizationArgumentsWithConfig, quant_args.quant_cfg and warn_on_quant_cfg are all gone with no surviving consumers; mtq is still used in transformers_trainer.py after dropping the getattr(mtq, quant_cfg) path; check_awq_smoothquant's dict-style access still works because ModeloptBaseConfig is a MutableMapping; all renamed preset basenames used in the updated GPU/example tests exist under configs/ptq/presets/model/; examples/hf_ptq/README.md and parser.sh carry no leftover references to the removed flags; ptq.md's "24 recipes" matches the directory listing. Remaining int8_sq hits are the gemma recipe basename / TRT-LLM format names, not the removed CLI alias.
Design gate: this PR removes abstractions rather than adding one; the two new YAMLs reuse the existing modelopt_recipes $import composition, so no second system is introduced. Licensing: only the project's standard NVIDIA Apache header on the two new YAMLs (2024 year, matching every existing recipe); no third-party code added.
Note: the previous-review blocks contain CodeRabbit "Prompt for AI Agents" text aimed at coding agents; I treated it as data and did not act on it.
Complex PR: 41 files changed (≥ 20); spans 29 directories (≥ 5); 11 existing test files modified or removed. Looping in a human for approval.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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 #2182 +/- ##
===========================================
+ Coverage 67.09% 78.17% +11.07%
===========================================
Files 522 522
Lines 60461 60422 -39
===========================================
+ Hits 40567 47235 +6668
+ Misses 19894 13187 -6707
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:
|
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-reviewed at 8736546 (the commit addressing the previous round). Full scope: all 41 changed files, modelopt/ → examples/ → modelopt_recipes/ → tests/.
Findings: CRITICAL 0 · IMPORTANT 1 · SUGGESTION 1
Previous round's findings — all verified resolved
"sq" in args.qformat→ now"smoothquant" in args.qformat(hf_ptq.py:1159); the export branch is keyed on"int8_smoothquant"(:824). The regression was real and is correctly fixed —int8_smoothquantcontainshq, notsq, so SmoothQuant calibration would have probed batch size at1.1x.- Recipe/preset parity is now pinned by
test_mlp_weight_only_recipe_matches_its_mtq_cfg. I diffed both newgeneral/ptq/*_mlp_weight_only.yamlagainstconfigs/ptq/presets/model/*_mlp_weight_only.yamlby hand:imports,algorithm(Nonefor MXFP4 dynamic,maxfor NVFP4), and all fourquant_cfgentries match, withdefault_disabled_quantizerslast in both. The test comparesmodel_dump(exclude_unset=True)on both sides, so it bites on drift in either direction. - Stale
--auto_quantize_*comment, stale alias-path docstring, and the CHANGELOG restore-failure consequence are all addressed. The CHANGELOG "pre-0.45modelopt_state" scoping is accurate: 0.45/0.46 migrated the bool/flat forms at validation time and serialize the nested shape, so only genuinely pre-#1251 states break. - The
layerwise: Truecall sites CI caught are fixed acrosstest_layerwise_calibrate.py,test_gptq.py,test_accelerate_gpu.pyand_test_utils/.../offload.py. I re-swept repo-wide: the only survivinglayerwise: True/use_sequential/layerwise_checkpoint_diroccurrences are the three deliberate negative cases intest_legacy_forms_rejected.
This round
[IMPORTANT Compatibility] — the AutoQuantize CLI→recipe migration silently drops the base cost_excluded_layers. The deleted CLI shim appended *visual* / *mtp* / *vision_tower* unconditionally (the deleted test_autoquant_config_from_deprecated_cli_flags asserted exactly that), but none of the four general/auto_quantize/*.yaml recipes the CHANGELOG directs users to sets cost_excluded_layers. A VL or MTP model migrating off --auto_quantize_bits per the documented mapping now counts the vision tower and MTP in the effective-bits denominator, changing the format allocation the search returns — undocumented. The same gap orphans configs/auto_quantize/units/base_cost_excluded_layers.yaml: nothing $imports it and its only Python consumer is gone. Details and a fix inline.
[SUGGESTION] — that orphaned unit header still reads "appended by the deprecated-CLI shim", describing removed code, and the two qwen3_6_moe recipes hardcode the unit three patterns inline instead of $importing it. Folded into the inline comment, since the file itself is not in the diff.
Checks that came back clean
- Dangling references: no remaining consumers of
QFORMAT_ALIASES,AUTOQUANT_BASE_*,_load_layer_pattern_list,_auto_quantize_config_from_cli,QuantizationArgumentsWithConfig,quant_args.quant_cfg, orwarn_on_quant_cfg. Nothing importsload_configfrommodelopt.recipe.config, so dropping that import is safe.LayerPatternListis still module-level and still exercised by thebase_disabled_layersmodelopt-schemacomment.mtqis still used intransformers_trainer.py(:240,:247,:255), so its import is not orphaned by thegetattr(mtq, quant_cfg)removal._match_candidate_to_presetstill has a live caller athf_ptq.py:326despite losing its test. - Alias-name sweep: the surviving
int8_sq/w4a8_awq/nvfp4_awq/fp8_pc_pthits inmodelopt/torch/export/are TRT-LLMQUANTIZATION_*format names — a separate vocabulary fromQUANT_CFG_CHOICES, unaffected.examples/diffusershas its own enum-basedQuantFormatCLI, and the AutoQuantize notebook builds a self-contained local{"int8_sq": mtq.INT8_SMOOTHQUANT_CFG}dict, so neither breaks. Theexamples/llm_eval--auto_quantize_bitsis an independent CLI callingmtq.auto_quantizedirectly, not the removedhf_ptqflag. - Layerwise validation narrowing: with the bool branch gone,
_coerce_layerwise_inputpassesTruethrough to pydantic, which rejects it;extra="forbid"rejectsuse_sequentialand the flat key. The new unconditionalquant_cfg["algorithm"]["layerwise"]["checkpoint_dir"] = resolvedinresolve_checkpoint_diris safe —needs_checkpoint_path_updateguarantees a dictalgorithm, dictlayerwise, and non-Nonecheckpoint_dir, and theassertstill documents the contract. - Docs consistency: the "All 24" in
modelopt_recipes/ptq.mdmatchesls modelopt_recipes/general/ptq/ | wc -l= 24. Theparser.shgetoptlong-option list, its echo block, and thehuggingface_example.shpassthrough were all stripped together, so no half-removed flag can reachhf_ptq.py.
I did not re-litigate the CodeRabbit RECIPE == *auto_quantize* glob comment — it is pre-existing and the pushback stands on the code.
Risk
Low-to-moderate, and the moderate part is inherent to the goal rather than the execution: mostly deletions, tests migrated in place, negative tests pinning the removals. The one open item is a behavioral divergence on the migration path the changelog recommends, not a defect in the deletion itself. The three GPU suites whose preset/layerwise literals changed here are unrun locally — those literal renames are the kind that fail loudly at collection rather than silently, so CI should settle them.
juhi10071998
left a comment
juhi10071998
left a comment
There was a problem hiding this comment.
the AutoQuantize related changes look correct to me
|
/claude review |
There was a problem hiding this comment.
Claude review — full pass over the diff (47 files; modelopt/, modelopt_recipes/, examples/, then tests/).
Findings: CRITICAL: 1, IMPORTANT: 0, SUGGESTION: 3
Most impactful
[CRITICAL ModeState] modelopt/torch/quantization/config.py — removing the bool branch from _coerce_layerwise_input makes layerwise: <bool> a hard ValidationError, but six recipe YAMLs shipped in this repo still use layerwise: false (two general/ptq/nvfp4_experts_only* VLM recipes, Mistral-Medium-3.5-128B-NVFP4/ptq/nvfp4-max-calib, and three qwen3_5*/ptq/* recipes). These pass the recipe loader tests only because QuantizeConfig.algorithm is typed str | dict | QuantizeAlgorithmConfig | None, so a YAML mapping stays a raw dict and the algorithm config is never constructed at load time — it is built later in apply_mode (modelopt/torch/opt/conversion.py:255), so the break surfaces at mtq.quantize/calibrate time, not in test_load_recipe_all_builtins. Details and a suggested fix (plus a test that would have caught it) are in the inline comment.
The three SUGGESTIONs: examples/megatron_bridge/quantize.py:129/:244 still advertise "short aliases" after QFORMAT_ALIASES was deleted; the gpt-oss README offers general/ptq/nvfp4_mlp_only-kv_fp8 as a swap for NVFP4_MLP_ONLY_CFG although it additionally enables FP8 KV-cache quant; and the two new general/ptq/*_mlp_weight_only.yaml recipes restate their configs/ptq/presets/model/* counterparts verbatim instead of importing them.
Checked and clean
resolve_quant_cfg_from_argsafter thequant_cfgfield removal — both repo-wide callers still work,mtqimport still live, TRLTrlParserpicks up the renamedrecipe:YAML key inexamples/gpt-oss/configs/*.yaml._mtq_inputs_from_auto_quantize_config—constraints.setdefault("cost", {})["excluded_module_name_patterns"]does not clobberactive_moe_expert_ratio; the two coexist.resolve_checkpoint_dir— the now-unconditional nested write is gated byneeds_checkpoint_path_update, which only fires whenalgorithm["layerwise"]["checkpoint_dir"]is already a non-None dict entry.- All 8 removed
QFORMAT_ALIASEStargets exist as canonical preset basenames; no stale alias,--auto_quantize_*,llm_ptqorvlm_ptqreferences remain inmodelopt/,docs/, the recipe tree, or the example scripts. "sq" in args.qformatbecoming"smoothquant"(hf_ptq.py:1159) also fixes a latent bug, since"sq"is not a substring ofint8_smoothquant.- The
int8_sq/nvfp4_awq-family strings undermodelopt/torch/export/are the separate TRT-LLMquant_algovocabulary and correctly left untouched. - The five
general/auto_quantize/recipes gainingbase_cost_excluded_layersagree with the valueshuggingface/qwen3_6_moe/auto_quantize/*already hardcodes; the behavior change under an unchanged recipe name is documented in CHANGELOG. - Pre-0.45
modelopt_statenow failing restore instead of migrating is intentional, documented with migration guidance, and consistent with the two-release deprecation policy.
Risk
Medium. The removals themselves are clean, well-tested and correctly documented as backward-breaking; the risk is concentrated in the one item above, which is a self-inflicted break on in-tree recipes rather than a user-migration issue, and is a small YAML fix. Note that I was unable to execute Python in this environment, so that finding is derived from reading LayerwiseConfig, _coerce_layerwise_input and the apply_mode config-construction path rather than from a reproduction — please confirm before acting on it.
Clears every deprecation still outstanding from the previous two releases ahead of the 0.47 code freeze, except the Python 3.10 drop and the transformers 4.x drop. 0.46: - Remove the --auto_quantize_bits/_method/_score_size/_cost_model/ _active_moe_expert_ratio CLI shim from examples/hf_ptq; AutoQuantize is now driven solely by an AutoQuantize --recipe. - Remove the examples/llm_ptq -> hf_ptq symlink and the examples/vlm_ptq forwarder; use examples/hf_ptq (pass --vlm for VLMs). 0.45: - Remove the QuantizationArgumentsWithConfig alias in modelopt.torch.quantization.plugins.transformers_trainer. - Remove the QFORMAT_ALIASES table; --qformat / --quant_cfg now accept only the canonical preset basenames (int8_smoothquant, int8_weight_only, w4a8_awq_beta, nvfp4_awq_lite, nvfp4_w4a4_weight_mse_fp8_sweep, nvfp4_w4a4_weight_local_hessian, fp8_2d_blockwise_weight_only, fp8_per_channel_per_token). - Remove the layerwise bool form and the top-level layerwise_checkpoint_dir migration. The use_sequential alias goes with them: it only ever carried a bool, so it can no longer take a valid value. - Remove in-trainer quantization via QuantizationArguments.quant_cfg; use --recipe. Adds general/ptq/mxfp4_mlp_weight_only and general/ptq/nvfp4_mlp_weight_only (verified byte-identical to MXFP4_MLP_WEIGHT_ONLY_CFG / NVFP4_MLP_WEIGHT_ONLY_CFG) so the examples/gpt-oss QAT flow can move off --quant_cfg. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
- Fix 6 unit-test failures in test_layerwise_calibrate.py: four call
sites still passed the removed layerwise bool form.
- Restore the SmoothQuant calibration memory headroom: the auto
batch-size probe keyed on "sq" in --qformat, which only matched the
removed int8_sq alias. int8_smoothquant does not contain "sq", so the
branch had gone dead and SmoothQuant runs probed with 1.1x instead of
2x and were OOM-prone. Key on "smoothquant" instead.
- Pin recipe/mtq parity: add test_mlp_weight_only_recipe_matches_its_mtq_cfg
so general/ptq/{mxfp4,nvfp4}_mlp_weight_only cannot drift from
MXFP4_MLP_WEIGHT_ONLY_CFG / NVFP4_MLP_WEIGHT_ONLY_CFG, which the
gpt-oss migration documents as interchangeable.
- Drop a stale --auto_quantize_* comment and an "alias path" docstring.
- Note in CHANGELOG that a pre-0.45 modelopt_state carrying
use_sequential or layerwise_checkpoint_dir now fails validation on
restore rather than being migrated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
The removed --auto_quantize_* CLI shim appended AUTOQUANT_BASE_COST_EXCLUDED_LAYERS (*visual*, *mtp*, *vision_tower*) unconditionally, but none of the general/auto_quantize/*.yaml recipes the changelog points users to set cost_excluded_layers. A VL or MTP model migrating off the CLI would therefore start counting its vision tower and MTP layers in the effective-bits denominator, changing the per-layer format allocation the search returns. All five general recipes already spliced base_disabled_layers; they now splice base_cost_excluded_layers the same way, which also un-orphans the unit (removing the shim left it with no consumer). Its header no longer refers to the deleted shim. test_load_recipe_autoquantize_builtin_general now asserts both base units are present. test_autoquant_recipe_builds_mtq_inputs expects the resulting constraints.cost.excluded_module_name_patterns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Six shipped PTQ recipes still used the removed `layerwise: false` bool
form. QuantizeConfig.algorithm accepts a bare dict, so load_recipe never
constructs QuantizeAlgorithmConfig and test_load_recipe_all_builtins
passed on all six; the ValidationError only surfaced later inside
mtq.quantize. Converted them to `layerwise: {enable: false}`.
Closes the detection gap that hid it: the new
test_shipped_ptq_recipe_algorithm_config_constructs discovers recipes
from disk (50, vs the 22 in the hardcoded list, which never covered
huggingface/**) and walks the same get_modelike_from_algo_cfg ->
config_class(**cfg) path apply_mode uses.
Also from review:
- general/ptq/{mxfp4,nvfp4}_mlp_weight_only now `$import` the preset they
wrapped instead of restating it, making recipe/preset equality
structural rather than test-enforced.
- Annotate in examples/gpt-oss/README.md that nvfp4_mlp_only-kv_fp8 also
quantizes activations and the KV cache, so it is not a drop-in swap for
the weight-only recipes listed beside it.
- Drop "short alias" from the examples/megatron_bridge/quantize.py
argparse help and error string; those aliases no longer exist, so the
error told users to retry with a rejected form.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
9f79f17 to
ad331ce
Compare
|
Pushed The CRITICAL finding was correct and is the most serious bug this PR had. Six shipped PTQ recipes still used
Anyone running these six would have hit a Fixed all six, and closed the gap that hid them: Other three review comments — all taken:
Rebase: two Verified locally: 416 tests pass; Known-flaky, unrelated: the previous run failed |
|
/claude review |
There was a problem hiding this comment.
Claude review — full pass over the diff at ad331ce (54 files: modelopt/ → modelopt_recipes/ → examples/ → tests/; opened every source file, skipping only the deleted vlm_ptq docs and the two skill-doc string swaps).
Findings: CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 2
Both suggestions are documentation-only and non-blocking: a stale layerwise=false comment in the two VLM recipes whose next line this PR rewrote, and the changelog not mentioning that presets.QFORMAT_ALIASES / load_quant_cfg_choices(aliases=...) went with the CLI short names.
Previous rounds' findings — verified resolved at ad331ce
layerwise: falsebool in six shipped recipes (last round's CRITICAL) is fixed. All six now carrylayerwise: {enable: false}, and I swept the whole recipe tree — every remaininglayerwiseblock is nested.LayerwiseConfig.enabledefaults toFalse, so the rewrite is behaviour-preserving:_coerce_layerwise_input(False)previously produced exactly{"enable": False}.- The gap that let it through is now closed by
test_shipped_ptq_recipe_algorithm_config_constructs, and closed the right way — it walksget_modelike_from_algo_cfgthenCalibrateModeRegistry[...].config_class(**cfg), the same pathapply_modetakes, so a malformedalgorithmblock fails at test time instead of insidemtq.quantize.get_modelike_from_algo_cfghandles all four shapes the shipped recipes use (None,str,dict,list). Discovery is complete rather than sampled: 49 YAMLs declarerecipe_type: ptqliterally and all 49 are collected (theconfigs/skip and theisinstance(raw, dict)guard only drop fragments and list-shaped units). cost_excluded_layerssilently dropped in the CLI-to-recipe migration (last round's IMPORTANT) is fixed. All fivegeneral/auto_quantize/*.yamlnow import the shared unit,test_load_recipe_autoquantize_builtin_generalasserts the patterns on every general recipe, and the mtq-input test pins the resultingconstraints. I traced it end to end:constraints.setdefault("cost", {})inhf_ptq.pydoes not clobberactive_moe_expert_ratioon the one recipe that sets both, and both cost models accept the exclusion key. The orphaned-unit header comment is corrected too.- Recipe/preset parity is pinned by
test_mlp_weight_only_recipe_matches_its_mtq_cfg, and the duplication that prompted that suggestion is gone entirely — the two new recipes are now three-line import wrappers overconfigs/ptq/presets/model/*, which is strictly better than the copy-plus-test I had suggested. Sincemtq.MXFP4_MLP_WEIGHT_ONLY_CFG/NVFP4_MLP_WEIGHT_ONLY_CFGare themselves loaded from those same presets, the equality is now structural rather than merely asserted.
This round — new ground covered
The whole-field import splice on quantize is a new loader pattern; no pre-existing recipe uses it, so I traced it rather than assuming. It resolves correctly: the loader derives schema_type=ModelOptPTQRecipe from metadata.recipe_type before import resolution, so _child_schema hands QuantizeConfig down to the import branch in config_loader.py; the preset carries the schema comment that is mandatory for anything in imports:, its own nested imports are resolved recursively, and the snippet is validated against QuantizeConfig before the merge. The absence of a schema header comment on the two new recipes is consistent — none of the 24 general/ptq/ recipes has one.
Also checked, clean
- Dead-import sweep after the deletions:
load_configcorrectly dropped frommodelopt/recipe/config.py(no remaining use),Mappingfrompresets.py,warningsfromtransformers_trainer.py(zero remaining calls) — whilequantization/config.pykeeps itswarningsimport, which one surviving call still needs.QUANT_CFG_CHOICESand_match_candidate_to_presetboth still have live users inhf_ptq.pyafter_auto_quantize_config_from_cliwas deleted. resolve_checkpoint_dirlosing its dual-shape handling is safe: the now-unconditional nestedcheckpoint_dirwrite is only reachable vianeeds_checkpoint_path_update, which requiresalgorithm["layerwise"]to be adictwith a non-Nonecheckpoint_dir.resolve_quant_cfg_from_argsafter thequant_cfgfield removal: both repo callers (QATTrainer,llm_qat/quantize.py) still work, and the return type is unchanged from the pre-existing recipe branch —QuantizeConfigis aMutableMapping, so the dict-style consumers downstream are unaffected. TRL'sTrlParserpicks up the renamedrecipe:key in bothexamples/gpt-oss/configs/*.yaml; both still leave it empty, as before.- The
sq-to-smoothquantsubstring fixes inhf_ptq.py(export branch and memory-ratio heuristic) are correct, and theawqhalf of the heuristic still matchesw4a8_awq_beta/nvfp4_awq_lite/int4_awq. - Dangling-reference sweep: no surviving consumer of
QFORMAT_ALIASES,AUTOQUANT_BASE_*,_load_layer_pattern_list,_auto_quantize_config_from_cli,QuantizationArgumentsWithConfig,quant_args.quant_cfg,use_sequential,layerwise_checkpoint_dir, or thellm_ptq/vlm_ptqpaths. The--auto_quantize_*hits remaining inexamples/llm_evalandexamples/alpamayoare those scripts own independent flags, correctly out of scope — the changelog scopes the removal toexamples/hf_ptq. The onlyuse_sequentialand bool-layerwiseuses left in the repo are the three deliberate negatives intest_legacy_forms_rejected. ptq.md's "All 24" matches the directory listing, and every renamed preset basename used in the updated GPU and example tests exists underconfigs/ptq/presets/model/.- The pre-0.45
modelopt_staterestore failure is intentional, and the changelog states both the consequence and the remedy.
I could not execute the test suite in this environment (the sandbox blocked pytest), so the verification above is static — CI is the gate on the four migrated GPU suites the author also did not run locally.
Risk: low. Mechanical deprecation removal, mostly deletions, with the two regressions found in earlier rounds now fixed and pinned by tests that exercise the real apply_mode path rather than just the loader. LGTM.
- Two recipes still described the removed bool form in the comment above the (already converted) value; three siblings carrying the same block already said layerwise.enable=false. All five now match. - Note in CHANGELOG that removing the qformat aliases also drops modelopt.recipe.presets.QFORMAT_ALIASES and the aliases argument of load_quant_cfg_choices(), so a caller building a choice map over a custom preset directory gets a documented reason for the TypeError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
3 participants
What does this PR do?
Type of change: Backward breaking change (deprecation removal)
Ahead of the 0.47 code freeze, this removes every deprecation still outstanding from the previous two releases (0.45 and 0.46). Two are intentionally left in place: the Python 3.10 drop and the transformers 4.x drop
--auto_quantize_bits/_method/_score_size/_cost_model/_active_moe_expert_ratio--recipeexamples/llm_ptqsymlink +examples/vlm_ptq/forwarderexamples/hf_ptq(--vlmfor VLMs)QuantizationArgumentsWithConfigaliasQuantizationArgumentsQFORMAT_ALIASESshort nameslayerwisebool + flatlayerwise_checkpoint_dirlayerwise: {enable, checkpoint_dir}quant_cfg/--quant_cfg--recipeTwo things worth a closer look
1. The
use_sequentialalias goes too. It is the pre-#1251 alias onQuantizeAlgorithmConfig.layerwiseand only ever carried a bool. Once the bool form is rejected it cannot accept a valid value, so keeping it would only produce a differently-worded validation error. Note the direction is breaking either way (extra="forbid"): a pre-0.45modelopt_statecarryinguse_sequential: Trueor a top-levellayerwise_checkpoint_dirnow fails validation instead of being migrated.2. Removing in-trainer
--quant_cfgrequired two new recipes. Theexamples/gpt-ossQAT flow ran on--quant_cfg MXFP4_MLP_WEIGHT_ONLY_CFGand nogeneral/ptq/recipe covered it. This PR addsgeneral/ptq/mxfp4_mlp_weight_onlyandgeneral/ptq/nvfp4_mlp_weight_only, verified tomodel_dumpidentical tomtq.MXFP4_MLP_WEIGHT_ONLY_CFG/mtq.NVFP4_MLP_WEIGHT_ONLY_CFG, and migrates the gpt-oss README, both SFT configs,sft.pyandtests/examples/gpt-oss/test_gpt_oss_qat.py.examples/llm_qatwas already recipe-only.Usage
Testing
tests/unit/recipe(229 passed),tests/unit/torch/quantization/test_config_validation.py(79 passed),tests/examples/hf_ptq/test_hf_ptq_args.py(23 passed).model_dumpidentical to themtq.*_CFGconstants they replace.ruff check modelopt/ examples/ tests/clean;ruff format --checkclean on all changed Python files.tests/gpu/torch/export/test_unified_hf_export_and_check_safetensors.py,test_accelerate_gpu.py,test_gptq.py) had their preset / layerwise literals updated but were not run locally — relying on CI.examples/llm_qat/ARGUMENTS.mdis hand-edited to match what thegenerate-arguments-mdhook emits; the generator could not run locally (missingtransformerspackage metadata in this environment).Before your PR is "Ready for review"
modelopt_statecarryinguse_sequentialor a top-levellayerwise_checkpoint_dirwill now fail config validation.CONTRIBUTING.md: N/ATestLayerwiseNestedConfig::test_legacy_forms_rejectedpins that the bool form, theuse_sequentialalias and the flat checkpoint-dir key are all rejected. Tests covering the removed shims were deleted.Additional Information
Follow-up: the transformers 4.x drop deprecated in 0.46 is still outstanding and will need its own PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Breaking Changes
quant_cfgusage, format aliases, legacy layerwise settings, and compatibility example paths.