Fix gemma4 multi component quantization - #2637
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves robustness in Olive’s weight quantization for tiny FP16 values and extends ModelConfig.select_components to support selecting multiple Hugging Face model components (aggregating their runtime source paths) for component-scoped PyTorch optimization.
Changes:
- Clamp
WeightQuantizer.find_qparamsscales to the smallest positive representable value to avoid FP16 underflow to zero. - Allow
HfModelcomponent selection of multiple components and aggregate theircomponent_source_pathsintomodel_attributes. - Update/add tests covering multi-component selection aggregation and tiny-FP16 scale underflow prevention.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
olive/common/quant/utils.py |
Clamps computed quantization scales to a minimum positive value to prevent underflow to zero. |
olive/model/config/model_config.py |
Updates HF component selection logic to support multiple selected components and aggregate source paths into model_attributes. |
test/common/quant/test_utils.py |
Adds a unit test ensuring tiny nonzero FP16 weights do not yield zero scales. |
test/model/test_composite_model.py |
Updates HF-model component selection tests to validate multi-component aggregation instead of expecting an error. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: xiaoyu-work <85524621+xiaoyu-work@users.noreply.github.com>
5996154 to
74a029e
Compare
Review team verdict: Request changesThe review team found one critical model-correctness issue and several major checkpoint integrity and integration problems.
Non-blocking compatibility questions
|
|
Copilot resolve the merge conflicts in this pull request |
…onent-quantization # Conflicts: # olive/passes/onnx/mobius_model_builder.py Co-authored-by: xiaoyu-work <85524621+xiaoyu-work@users.noreply.github.com>
Merged There was one conflict, in
|
Updated review team verdict: Request changesThe latest commits resolve most of the original findings, including override precedence, loss of skip-pattern coverage, stale checkpoint detection, CLI/footprint path updates, explicit rejection of unsupported multi-component PyTorch optimization, guarded embedding retying, resolved sibling-output detection, and Diffusers The following blockers remain:
DocumentationThe checkpoint tree still shows fixed names such as |
Updated review team verdict: Request changesThe latest update resolves four of the six previous blockers:
However, the rewritten tying and ownership/publication logic introduces one reproducible critical regression and leaves several hardening gaps.
PriorityThe tying regression, nested VLM tying reconciliation, and unsafe partial-config restoration are merge blockers. Ancillary-file ownership and crash recovery should also be addressed before relying on assembly as a safe checkpoint publication mechanism. The explicit no-op behavior and comprehensive |
Updated review team verdict: Request changesThe latest commit fixes the original tied-model regression for flat configurations such as Llama and Qwen. However, the same random-
Tying fix guidance
|
Ti-Tai Wang (titaiwangms)
left a comment
There was a problem hiding this comment.
Approved
The latest update resolves the previously blocking tying issues:
- Assembly no longer rewrites source tying fields when embedding/head storage is unchanged.
- Root and
text_config.tie_word_embeddingsvalues are compared independently across builds. - Cleanup no longer removes
output_dir/modelunless it is the actual source model directory.
The remaining concerns are non-blocking and can be handled as follow-ups:
- Use the same canonical parent for automatic assembly when a workflow configures only
builds._default.output_dir; currently this can fail after successful builds because assembly requires a top-levelengine.output_dir. - Define rerun and crash-recovery behavior for an already populated assembly root.
- Harden component publication against symlink redirection and check/use races.
- Optionally compare unowned tensor contents, rather than only shape and dtype, when passes may mutate tensors outside their selected component.
- Generalize tying-field discovery if models with deeper nested configurations such as
thinker_config.text_configneed assembly support. - Validate storage-changing tying metadata against the final input/output embedding tensor keys.
These should be tracked separately so they do not expand the scope of the Gemma4 tying fix further.
Describe your changes
This pull request improves quantization robustness for small floating point values and enhances the
ModelConfigcomponent selection logic to support multiple components, along with corresponding test updates.Quantization robustness:
olive/common/quant/utils.py, thefind_qparamsmethod now clamps the calculatedscalesto a minimum positive value to prevent underflow to zero for tiny FP16 values, ensuring numerical stability during quantization.test_find_qparams_tiny_fp16_values_do_not_underflow) to verify that quantizing nonzero FP16 groups does not produce zero scales, preventing underflow issues.ModelConfig component selection enhancements:
ModelConfig.select_components, allowing aggregation of multiple components for component-scoped PyTorch optimization._select_hf_componentto handle multiple selected components, aggregating their names and source paths intomodel_attributes, and improved error handling for missing source paths.Checklist before requesting a review
lintrunner -a(Optional) Issue link