Single grouped weight fixes#3225
Conversation
Greptile SummaryThis PR delivers three targeted fixes to the single grouped weight feature introduced in #3178. All three fixes address correctness gaps that surface when a grouped weight is used across training and eval steps, constructed on the meta device, or packed with MXFP8.
Confidence Score: 5/5Safe to merge — all three fixes are narrowly scoped, well-tested, and address real correctness gaps without changing any shared code paths. The changes are precise bug fixes with direct test coverage for each scenario. The set_usage(columnwise=None) change correctly leverages the existing None-means-no-op contract in Quantizer.set_usage. The register_parameter override is guarded by name == 'weight' and single_grouped_weight, so it cannot affect non-grouped paths. The high-precision init-val stacking is consistent with the shapes expected by the test assertions. No silent data-loss or backward-hook ordering issues were found. Files Needing Attention: No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant GroupedLinear_ops as GroupedLinear (ops)
participant GroupedLinear_mod as GroupedLinear (module)
participant GroupedTensor
participant Quantizer
Note over Caller,GroupedTensor: Fix 1 — high-precision init-val preservation
Caller->>GroupedLinear_mod: make_grouped_weights()
GroupedLinear_mod->>GroupedLinear_mod: collect high_precision_init_vals from weight0..N
GroupedLinear_mod->>GroupedLinear_mod: consistency check (all or none must have init val)
GroupedLinear_mod->>GroupedTensor: make_grouped_tensor_with_shapes()
GroupedLinear_mod->>GroupedLinear_mod: torch.stack(init_vals) → attach to grouped_parameter
GroupedLinear_mod->>GroupedLinear_mod: clear init_vals from individual weights
Note over Caller,GroupedLinear_ops: Fix 2 — late-attach delayed-wgrad marking
Caller->>GroupedLinear_ops: "GroupedLinear(device=meta, delay_wgrad_compute=True)"
GroupedLinear_ops-->>Caller: "shell (weight=None)"
Caller->>GroupedLinear_ops: register_parameter(weight, grouped_param)
GroupedLinear_ops->>GroupedLinear_ops: "override: set skip_backward_post_hook=True"
GroupedLinear_ops-->>Caller: parameter marked before DDP hooks install
Note over Caller,Quantizer: Fix 3 — columnwise usage preservation (MXFP8)
Caller->>GroupedLinear_ops: "forward() training, requires_grad=True"
GroupedLinear_ops->>Quantizer: "set_usage(rowwise=True, columnwise=True)"
Note right of Quantizer: columnwise_usage=True stored on weight's quantizer
Caller->>GroupedLinear_ops: "forward() eval, requires_grad=False"
GroupedLinear_ops->>Quantizer: "set_usage(rowwise=True, columnwise=None)"
Note right of Quantizer: columnwise_usage preserved (None = no-op)
Reviews (5): Last reviewed commit: "Fix inconsistent line endings" | Re-trigger Greptile |
|
/te-ci pytorch L1 |
|
/te-ci pytorch L1 |
| # Full-iteration CUDA graph replay does not rerun this Python metadata | ||
| # update. Remember that a grad-enabled forward requested columnwise | ||
| # storage so eager eval cannot drop buffers still used by captured dgrad. | ||
| fc1_weight_quantizer.set_usage( | ||
| rowwise=True, | ||
| columnwise=input_requires_grad or fc1_weight_quantizer.columnwise_usage, | ||
| ) | ||
| fc1_op.weight.quantizer = fc1_weight_quantizer | ||
| grouped_fc1_weight = fc1_op.weight |
There was a problem hiding this comment.
The op and the param purposely have different quantizers so that you can change them independently. However, we are incorrectly reusing the same quantizer in two places, and getting unexpected interactions. The op's quantizer is set in each forward pass:
The proper fix is keep the op and param quantizers distinct.
|
/te-ci pytorch |
Signed-off-by: CarlosGomes98 <carlosmiguel.gomes@live.com.pt>
Signed-off-by: CarlosGomes98 <carlosmiguel.gomes@live.com.pt>
Signed-off-by: CarlosGomes98 <carlosmiguel.gomes@live.com.pt>
2c7db67 to
b4f2be1
Compare
…d weight param Co-authored-by: Tim Moon <4406448+timmoon10@users.noreply.github.com> Signed-off-by: Tim Moon <4406448+timmoon10@users.noreply.github.com>
GitHub inserted CRLFs. Microsoft... Signed-off-by: Tim Moon <tmoon@nvidia.com>
Description
A few fixes to the single grouped weight implementation
12406c9 — Preserve grouped weight initialization metadata
99b3f37 — Mark late grouped weights for delayed wgrad
aa4e233 — Preserve grouped MXFP8 columnwise usage
See #3178 for the original PR.
PR #3224 is very relevant to this one, both are required fixes.
Fixes # (issue)
Type of change
Changes
Please list the changes introduced in this PR:
Checklist: