Skip to content

[pat] (2/5) Add QKGrouper and refactor AttentionHeadGrouper for vmap - #4378

Merged
lisjin merged 1 commit into
mainfrom
lvj/pat-grouper-vmap-refactor
May 14, 2026
Merged

[pat] (2/5) Add QKGrouper and refactor AttentionHeadGrouper for vmap#4378
lisjin merged 1 commit into
mainfrom
lvj/pat-grouper-vmap-refactor

Conversation

@lisjin

@lisjin lisjin commented May 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • QKGrouper: new grouper that prunes only the Q and K slabs of a packed QKV tensor, leaving V untouched.
  • AttentionHeadGrouperDim0/Dim1: stop in-place-mutating self.p.data and reassign self.p instead, so torch.vmap (used by PruneOptimizer's local_map path from PR1) can iterate over the grouped view without tripping the in-place check.

Changes

  • torchao/prototype/pat/group/attention.py
  • torchao/prototype/pat/group/__init__.py: re-export QKGrouper.

Depends on #4377

@lisjin
lisjin requested a review from andrewor14 May 11, 2026 16:38
@lisjin
lisjin requested review from jerryzh168 and vkuzo as code owners May 11, 2026 16:38
@lisjin lisjin added the module: not user facing Use this tag if you don't want this PR to show up in release notes label May 11, 2026
@pytorch-bot

pytorch-bot Bot commented May 11, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/4378

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 11, 2026
@lisjin

lisjin commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

@claude Please review changes

@claude

claude Bot commented May 11, 2026

Copy link
Copy Markdown

Claude finished @lisjin's task in 1m 25s —— View job


Review of PR #4378

  • Gather context and read changed files
  • Review code changes
  • Post review feedback

Overall: LGTM. The vmap refactor is clean and correct. One minor suggestion below.


AttentionHeadGrouperDim0 / Dim1 refactor

The change from self.p.data = self.p.data.view(...) to self.p = self.p.view(...) is the right approach for vmap compatibility. Since self._param retains the original tensor (set in Grouper.__init__), the base __exit__ correctly restores state via self.p = self._param without needing the explicit reshape-back that the old Dim0.__exit__ performed.

For Dim1, the custom __exit__ is still needed for the transpose+copy-back to self._orig_p, and the refactored version correctly uses a local data variable instead of mutating self.p.data — clean.

QKGrouper

The MRO and super() calls look correct:

  1. PackedGrouperMixin.__init__ sets embed_dim / pack_dim
  2. Dim1Grouper.__init__ (via super(PackedGrouperMixin, self)) wires up the slice through Grouper.__init__

One minor observation:

qk_reg_index validation (attention.py:32-35): The else branch silently treats any non-zero value as "key". Consider either an explicit elif qk_reg_index == 1 with a ValueError for invalid values, or just an assert:

assert qk_reg_index in (0, 1), f"qk_reg_index must be 0 (query) or 1 (key), got {qk_reg_index}"

This would prevent confusing behavior if someone mistakenly passes 2 expecting the value slice. Very minor — up to you whether it's worth adding for a prototype API.


No other issues found. The changes are minimal, well-motivated, and correctly preserve the grouper semantics while eliminating the in-place .data mutations that conflict with vmap.

@lisjin
lisjin force-pushed the lvj/pat-grouper-vmap-refactor branch from df62bda to cbada5f Compare May 12, 2026 13:45
@lisjin
lisjin force-pushed the lvj/fix-dtensor-redistr branch from 0f63547 to c383fbf Compare May 12, 2026 14:06
@lisjin
lisjin force-pushed the lvj/pat-grouper-vmap-refactor branch from cbada5f to 8efeba6 Compare May 12, 2026 14:06
@lisjin
lisjin changed the base branch from lvj/fix-dtensor-redistr to main May 14, 2026 15:23
QKGrouper applies pruning only to query and key weights of a packed
QKV tensor.

AttentionHeadGrouperDim0/Dim1 now reassign self.p instead of mutating
self.p.data, so torch.vmap (used by PruneOptimizer's local_map path)
can iterate over the grouped view.
@lisjin
lisjin force-pushed the lvj/pat-grouper-vmap-refactor branch from 8efeba6 to 23d1942 Compare May 14, 2026 15:30
@lisjin
lisjin merged commit 690aa63 into main May 14, 2026
15 of 18 checks passed
@lisjin
lisjin deleted the lvj/pat-grouper-vmap-refactor branch May 14, 2026 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: not user facing Use this tag if you don't want this PR to show up in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants