Skip to content

Move odd num_head in TransformerEncoder to slow_path #83483

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Move odd num_head in TransformerEncoder to slow_path #83483

wants to merge 1 commit into from

Conversation

zrphercule
Copy link
Contributor

Summary: odd nhead is not supported for masked softmax, therefore we just move it to use old slow_path

Test Plan: CI

Differential Revision: D38720086

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 15, 2022 •
edited
Loading

🔗 Helpful links

✅ No Failures (0 Pending)

As of commit 052ed6f (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@facebook-github-bot
Copy link
Contributor

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

@zrphercule zrphercule requested a review from erichan1 August 15, 2022 22:59
@erichan1 erichan1 mentioned this pull request Aug 15, 2022
Closed
erichan1
erichan1 approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@erichan1 erichan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix for #83355!

@erichan1
Copy link
Contributor

Failure is because we did not add odd nhead check to creating a NestedTensor. So when nhead is odd, we are sending a NestedTensor to slow path, which cannot accept NestedTensor. Best fix is probably to check if the first or any encoder layer in TransformerEncoder has odd nhead. Kind of ugly... but what can you do.

@albanD albanD removed their request for review August 16, 2022 15:29
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@zrphercule zrphercule force-pushed the export-D38720086 branch 2 times, most recently from 6eba5fe to fa96941 Compare August 17, 2022 22:21
@facebook-github-bot
Copy link
Contributor

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

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

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@zrphercule2 @facebook-github-bot
Move odd num_head in TransformerEncoder to slow_path (pytorch#83483)
052ed6f 
Summary:
Pull Request resolved: pytorch#83483

odd nhead is not supported for masked softmax, therefore we just move it to use old slow_path

Test Plan: CI

Reviewed By: erichan1

Differential Revision: D38720086

fbshipit-source-id: 75abd450d6b6525b4c84470e5d189f9c498db753
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered without a flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

@github-actions
Copy link
Contributor

Hey @zrphercule.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit that referenced this pull request Aug 20, 2022
@zrphercule2 @facebook-github-bot
Move odd num_head in TransformerEncoder to slow_path (#83483)
013b41e 
Summary:
Pull Request resolved: #83483

odd nhead is not supported for masked softmax, therefore we just move it to use old slow_path

Test Plan: CI

Reviewed By: erichan1

Differential Revision: D38720086

fbshipit-source-id: 76d8745e8c5748ab0d2038b3ba0bc20fbbd05f9b
@ngimel ngimel mentioned this pull request Apr 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erichan1 erichan1 erichan1 approved these changes

@jbschlosser jbschlosser Awaiting requested review from jbschlosser

@saketh-are saketh-are Awaiting requested review from saketh-are

Assignees
No one assigned
Labels
cla signed fb-exported Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zrphercule @facebook-github-bot @erichan1 @pytorchmergebot @zrphercule2