Skip to content

Fix static attention mask update #9101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

sxu
Copy link
Contributor

@sxu sxu commented Mar 10, 2025

Summary: The range based for loop was making a copy of the mask, and thus the updates did not take effect. Remove the copy and move constructors of StaticKVCache and StaticAttention as they are not needed. Also add the missing deallocate call in mask's destructor.

Differential Revision: D70914174

@sxu sxu requested review from lucylq and jackzhxng as code owners March 10, 2025 20:17
Copy link

pytorch-bot bot commented Mar 10, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9101

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

✅ No Failures

As of commit dd04ea9 with merge base d71f54a (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot facebook-github-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 Mar 10, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70914174

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70914174

@sxu sxu force-pushed the export-D70914174 branch from a23d57d to 534b005 Compare March 10, 2025 20:21
@sxu sxu requested a review from billmguo March 10, 2025 20:32
sxu added a commit to sxu/executorch that referenced this pull request Mar 10, 2025
Summary:

The range based for loop was making a copy of the mask, and thus the updates did not take effect. Remove the copy and move constructors of StaticKVCache and StaticAttention as they are not needed. Also add the missing deallocate call in mask's destructor.

Differential Revision: D70914174
@sxu sxu force-pushed the export-D70914174 branch from 534b005 to 2bdcae4 Compare March 10, 2025 20:33
sxu added a commit to sxu/executorch that referenced this pull request Mar 10, 2025
Summary:

The range based for loop was making a copy of the mask, and thus the updates did not take effect. Remove the copy and move constructors of StaticKVCache and StaticAttention as they are not needed. Also add the missing deallocate call in mask's destructor.

Differential Revision: D70914174
@sxu sxu force-pushed the export-D70914174 branch from 2bdcae4 to 7f564bb Compare March 10, 2025 20:34
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70914174

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70914174

sxu added a commit to sxu/executorch that referenced this pull request Mar 10, 2025
Summary:
Pull Request resolved: pytorch#9101

The range based for loop was making a copy of the mask, and thus the updates did not take effect. Remove the copy and move constructors of StaticKVCache and StaticAttention as they are not needed. Also add the missing deallocate call in mask's destructor.

Differential Revision: D70914174
@sxu sxu force-pushed the export-D70914174 branch from 7f564bb to c6ade12 Compare March 10, 2025 20:42
sxu added a commit to sxu/executorch that referenced this pull request Mar 11, 2025
Summary:

The range based for loop was making a copy of the mask, and thus the updates did not take effect. Remove the copy and move constructors of StaticKVCache and StaticAttention as they are not needed. Also add the missing deallocate call in mask's destructor.

Reviewed By: billmguo

Differential Revision: D70914174
@sxu sxu force-pushed the export-D70914174 branch from c6ade12 to 44ca9ac Compare March 11, 2025 16:46
sxu added a commit to sxu/executorch that referenced this pull request Mar 11, 2025
Summary:

The range based for loop was making a copy of the mask, and thus the updates did not take effect. Remove the copy and move constructors of StaticKVCache and StaticAttention as they are not needed. Also add the missing deallocate call in mask's destructor.

Reviewed By: billmguo

Differential Revision: D70914174
@sxu sxu force-pushed the export-D70914174 branch from 44ca9ac to be0b470 Compare March 11, 2025 16:47
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70914174

Summary:
Pull Request resolved: pytorch#9101

The range based for loop was making a copy of the mask, and thus the updates did not take effect. Remove the copy and move constructors of StaticKVCache and StaticAttention as they are not needed. Also add the missing deallocate call in mask's destructor.

Reviewed By: billmguo

Differential Revision: D70914174
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70914174

@sxu sxu force-pushed the export-D70914174 branch from be0b470 to dd04ea9 Compare March 11, 2025 16:50
@facebook-github-bot facebook-github-bot merged commit e763a83 into pytorch:main Mar 11, 2025
52 checks passed
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. fb-exported topic: not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants