Repository navigation
[pat] Support fixed head_dim in attention groupers - #4982
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/4982
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 33be250 with merge base cff77b4 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude review |
|
Claude finished @lisjin's task in 1m 14s —— View job Review: [pat] Support fixed head_dim in attention groupers
Overall this looks good. 1. Optimizer config errors only appear at the first prune step, not at construction
for group in self.regularized_param_groups():
...
self._get_grouper_kwargs(group) # fail fast on bad grouper configYou could go further and build the grouper once per param there, so divisibility errors also show up early. That costs some construction work, so it's optional. 2. The exactly-one check is written twice
grouper_kwargs.update(
{k: group[k] for k in ("num_heads", "head_dim") if group.get(k) is not None}
)Then the grouper is the only place the rule is enforced. This is minor; either way works. 3. One README line still mentions only
|
| name, value = ( | ||
| ("head_dim", head_dim) if head_dim is not None else ("num_heads", num_heads) | ||
| ) | ||
| if not isinstance(value, int) or isinstance(value, bool) or value <= 0: |
There was a problem hiding this comment.
| if not isinstance(value, int) or isinstance(value, bool) or value <= 0: | |
| if ( | |
| not isinstance(value, Integral) | |
| or isinstance(value, bool) | |
| or value <= 0 | |
| ): | |
| raise ... | |
| value = int(value) |
Otherwise this will fail on np.int32, np.int64
| ) | ||
| if not isinstance(value, int) or isinstance(value, bool) or value <= 0: | ||
| raise ValueError(f"{name} must be a positive integer, got {value!r}") | ||
| if packed_dim % value: |
There was a problem hiding this comment.
| if packed_dim % value: | |
| if packed_dim > 0 and packed_dim % value: |
Or add if packed_dim <= 0: raise validation somewhere above.
[ghstack-poisoned]
Allow heterogeneous attention widths to share a pruning configuration while preserving existing num_heads calls. Validate exclusive positive-integer arguments and packed-axis divisibility in the groupers, and reject missing or conflicting head arguments at optimizer construction before warmup. Adapted from qpat ebc76a1 (#57). Test Plan: ```bash source ~/.ao/bin/activate python -m pytest test/prototype/pat/test_pat_grouper.py test/prototype/pat/test_pat_pruneopt.py -q # 24 passed, 28 subtests passed ruff check --select F,I torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py ruff format --check torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py git diff --check ``` The new construction-time regression failed for all six missing/conflicting argument cases before the fix. Tags: stack_created_with_devmate ghstack-source-id: 7382331 Pull-Request: #4982
Allow heterogeneous attention widths to share a pruning configuration while preserving existing num_heads calls. Validate exclusive positive-integer arguments and packed-axis divisibility in the groupers, and reject missing or conflicting head arguments at optimizer construction before warmup. Adapted from qpat ebc76a1 (#57). Test Plan: ```bash source ~/.ao/bin/activate python -m pytest test/prototype/pat/test_pat_grouper.py test/prototype/pat/test_pat_pruneopt.py -q # 24 passed, 28 subtests passed ruff check --select F,I torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py ruff format --check torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py git diff --check ``` The new construction-time regression failed for all six missing/conflicting argument cases before the fix. Tags: stack_created_with_devmate ghstack-source-id: 7382331 Pull-Request: #4982
|
@claude review fixes |
|
Claude finished @lisjin's task in 41s —— View job Review of fixes ([UPDATE] commit
|
| # | Item | Status |
|---|---|---|
| 1 | Bad config should fail at construction | ✅ pruneopt.py:64 calls self._get_grouper_kwargs(group) in the __init__ loop. test_optimizer_rejects_invalid_config_before_warmup covers {}, explicit Nones, and both args set, with warmup_steps=10, on both axes. |
| 2 | Duplicate exactly-one check | ✅ The duplicate is kept on purpose, and it's justified now that (1) depends on it. |
| 3 | README recipe row | ✅ Now says "num_heads or head_dim". |
| 4 | Tuple-swap nit | ✅ Replaced with if/else. |
| 4 | Indivisible case through optimizer | ✅ test_optimizer_rejects_indivisible_head_dim added. It raises at step(), which is expected because the shape is only checked when the grouper is built. |
| 4 | Comment on the 12-zeros assertion | ✅ Added. |
All of these look right to me.
Still open: @z-a-f's inline comments
These weren't addressed in this update:
np.int64/np.int32rejected (attention.py:21).isinstance(np.int64(4), int)isFalse, so ahead_dimthat comes from a numpy computation or some config loaders raises "must be a positive integer". The suggested fix is right: checknumbers.Integral, still excludebool, then normalize withvalue = int(value)so laterview()calls get a Python int.
Fix this →packed_dim == 0(attention.py:23).0 % value == 0, so an empty packed axis passes validation and resolves tonum_heads=0(withhead_dim) orhead_dim=0(withnum_heads). In practice this only matters for zero-size tensors, so it's an edge case. Still, apacked_dim <= 0guard or a decision to skip such tensors would make the behavior explicit.
Minor observation
The construction-time check only enforces exactly one of the two arguments. Type and positivity checks, such as head_dim: 0 or a YAML string "64", still run only when the grouper is built at the first prune step, which is after warmup. If you want all argument errors to surface at construction, _get_grouper_kwargs could call a value-only check (the type/positivity part of _resolve_heads, factored out). This is optional; the divisibility check has to wait for the shape anyway.
Tests
I couldn't run the suite here because pytest needs approval in this environment. The new tests look correct when read.
Allow heterogeneous attention widths to share a pruning configuration while preserving existing num_heads calls. Validate exclusive positive-integer arguments and packed-axis divisibility in the groupers, and reject missing or conflicting head arguments at optimizer construction before warmup. Adapted from qpat ebc76a1 (#57). Test Plan: ```bash source ~/.ao/bin/activate python -m pytest test/prototype/pat/test_pat_grouper.py test/prototype/pat/test_pat_pruneopt.py -q # 24 passed, 28 subtests passed ruff check --select F,I torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py ruff format --check torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py git diff --check ``` The new construction-time regression failed for all six missing/conflicting argument cases before the fix. Tags: stack_created_with_devmate ghstack-source-id: 7382331 Pull-Request: #4982
Allow heterogeneous attention widths to share a pruning configuration while preserving existing num_heads calls. Validate exclusive positive-integer arguments and packed-axis divisibility in the groupers, and reject missing or conflicting head arguments at optimizer construction before warmup. Adapted from qpat ebc76a1 (#57). Test Plan: ```bash source ~/.ao/bin/activate python -m pytest test/prototype/pat/test_pat_grouper.py test/prototype/pat/test_pat_pruneopt.py -q # 24 passed, 28 subtests passed ruff check --select F,I torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py ruff format --check torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py git diff --check ``` The new construction-time regression failed for all six missing/conflicting argument cases before the fix. Tags: stack_created_with_devmate ghstack-source-id: 7382331 Pull-Request: #4982
Allow heterogeneous attention widths to share a pruning configuration while preserving existing num_heads calls. Validate exclusive positive-integer arguments and packed-axis divisibility in the groupers, and reject missing or conflicting head arguments at optimizer construction before warmup. Adapted from qpat ebc76a1 (#57). Test Plan: ```bash source ~/.ao/bin/activate python -m pytest test/prototype/pat/test_pat_grouper.py test/prototype/pat/test_pat_pruneopt.py -q # 24 passed, 28 subtests passed ruff check --select F,I torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py ruff format --check torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py git diff --check ``` The new construction-time regression failed for all six missing/conflicting argument cases before the fix. Tags: stack_created_with_devmate ghstack-source-id: 7382331 Pull-Request: #4982
Allow heterogeneous attention widths to share a pruning configuration while preserving existing num_heads calls. Validate exclusive positive-integer arguments and packed-axis divisibility in the groupers, and reject missing or conflicting head arguments at optimizer construction before warmup. Adapted from qpat ebc76a1 (#57). Test Plan: ```bash source ~/.ao/bin/activate python -m pytest test/prototype/pat/test_pat_grouper.py test/prototype/pat/test_pat_pruneopt.py -q # 24 passed, 28 subtests passed ruff check --select F,I torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py ruff format --check torchao/prototype/pat/group/attention.py torchao/prototype/pat/optim/pruneopt.py test/prototype/pat/test_pat_grouper.py git diff --check ``` The new construction-time regression failed for all six missing/conflicting argument cases before the fix. Tags: stack_created_with_devmate ghstack-source-id: 7382331 Pull-Request: #4982
Stack from ghstack (oldest at bottom):
Allow heterogeneous attention widths to share a pruning configuration while preserving existing num_heads calls. Validate exclusive positive-integer arguments and packed-axis divisibility in the groupers, and reject missing or conflicting head arguments at optimizer construction before warmup. Adapted from qpat ebc76a1 (#57).
Test Plan:
The new construction-time regression failed for all six missing/conflicting argument cases before the fix.
Tags: stack_created_with_devmate