Skip to content

Accept int32 as index for index_add op #2580

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
Merged
merged 2 commits into from
Oct 30, 2020
Merged

Accept int32 as index for index_add op #2580

merged 2 commits into from
Oct 30, 2020
+52 −38

Conversation

@JackCaoG
Copy link
Collaborator

@JackCaoG JackCaoG commented Oct 28, 2020

This is to fix #2575, in pytorch pr pytorch/pytorch#46758, index_add with int32 support is added, xla needs to follow the same logic.

@JackCaoG JackCaoG requested a review from ailzhang October 28, 2020 19:40
@ailzhang
Copy link
Contributor

ailzhang commented Oct 28, 2020

could you add torch_pin to check whether this PR fixes all issues from the upstream PR? Thanks!

@JackCaoG JackCaoG force-pushed the enable_index_add_int32 branch from 5cd6b60 to bcd6f51 Compare October 28, 2020 19:46
@JackCaoG
Copy link
Collaborator Author

JackCaoG commented Oct 28, 2020

could you add torch_pin to check whether this PR fixes all issues from the upstream PR? Thanks!

sure, I run it locally(with pytorch patch) and confirmed that failed test is passing with this change.

@qizzzh
Copy link

qizzzh commented Oct 28, 2020

Thanks for the fix! I'm updating index_select to support int32 indices as well. Do you think we need to fix that for XLA too?

@JackCaoG
Copy link
Collaborator Author

JackCaoG commented Oct 28, 2020

@qizzzh qi will that be in a separate pr? If that's the case we can do a follow up pr on pt/xla when that happens.

@JackCaoG
Copy link
Collaborator Author

JackCaoG commented Oct 28, 2020

test passed locally on my cpu machine, rerun it on circleCI

@JackCaoG JackCaoG requested a review from davidel October 29, 2020 01:53
@qizzzh
Copy link

qizzzh commented Oct 29, 2020

@qizzzh qi will that be in a separate pr? If that's the case we can do a follow up pr on pt/xla when that happens.

It's in the same PR. Just updated. Please feel free to take a look. Thanks!

davidel
davidel reviewed Oct 29, 2020
View reviewed changes
Copy link
Collaborator

@davidel davidel left a comment

Choose a reason for hiding this comment

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

This needs a test ...

@JackCaoG JackCaoG force-pushed the enable_index_add_int32 branch from bcd6f51 to 1dd9e44 Compare October 29, 2020 20:22
@JackCaoG
Copy link
Collaborator Author

JackCaoG commented Oct 29, 2020

@qizzzh it seems like pt/xla does not check index_select index type, so no change needs to be done.

@JackCaoG JackCaoG requested a review from davidel October 29, 2020 20:25
@JackCaoG JackCaoG merged commit a61eb49 into master Oct 30, 2020
JackCaoG added a commit that referenced this pull request Oct 30, 2020
@JackCaoG
Revert "Accept int32 as index for index_add op (#2580)"
dd21040 
This reverts commit a61eb49.
@JackCaoG JackCaoG mentioned this pull request Oct 30, 2020
Merged
JackCaoG added a commit that referenced this pull request Oct 30, 2020
@JackCaoG
Revert "Accept int32 as index for index_add op (#2580)" (#2594)
2cd4f07 
This reverts commit a61eb49.
JackCaoG added a commit that referenced this pull request Oct 30, 2020
@JackCaoG
Revert "Revert "Accept int32 as index for index_add op (#2580)" (#2594)"
8bce332 
This reverts commit 2cd4f07.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidel davidel davidel approved these changes

@ailzhang ailzhang Awaiting requested review from ailzhang

+1 more reviewer

@qizzzh qizzzh qizzzh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

XLA test failures when using int32 indices/offsets for nn.EmbeddingBag

5 participants

@JackCaoG @ailzhang @qizzzh @davidel