[fix] output of `embedding_bag` with non-contiguous weight #44032

kshitij12345 · 2020-09-02T16:17:40Z

Fixes #43723

use weight.contiguous on fast-path as it expects contiguous tensor.

TODO:

Add tests

use weight.contiguous on fast-path as it expects contiguous tensor.

dr-ci · 2020-09-02T17:41:27Z

💊 CI failures summary and remediations

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

2/2 failures possibly* introduced in this PR
- 2/2 non-CircleCI failure(s)

ci.pytorch.org: 2 failed

Failed: pr/caffe2-pytorch-linux-bionic-rocm3.7-py3.6-test
Failed: pr/pytorch-linux-bionic-rocm3.7-py3.6

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

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 8 times.

codecov · 2020-09-02T21:04:10Z

Codecov Report

Merging #44032 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #44032   +/-   ##
=======================================
  Coverage   69.29%   69.29%           
=======================================
  Files         381      381           
  Lines       47214    47214           
=======================================
+ Hits        32717    32718    +1     
+ Misses      14497    14496    -1

Impacted Files	Coverage Δ
torch/testing/_internal/expecttest.py	`78.57% <0.00%> (+1.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df8da5c...fe6961b. Read the comment docs.

glaringlee

Thanks for adding tests.
I have a nit comment, please take a look at.

aten/src/ATen/native/EmbeddingBag.cpp

* use method chaining.

glaringlee · 2020-09-03T16:52:58Z

@kshitij12345 Thanks a lot for fixing this. i will approve this once the CI test is done without issues.

facebook-github-bot

@glaringlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

glaringlee

LGTM now.

kshitij12345 · 2020-09-08T14:24:50Z

@glaringlee Gentle Ping :)

glaringlee · 2020-09-08T14:31:20Z

@kshitij12345
Hi, your code will be landed today.
The code is in landing process, here is what happened, after the approval on github, I imported the code to our fb internal system, and there will be another round approval there before landing the code. We were on time off since last Friday due to labor day, so the other approval will happen today and your code will be landed then. Thanks for reaching out.

kshitij12345 · 2020-09-08T14:42:07Z

@glaringlee Oops I forgot it was Federal Holiday. Thanks!

facebook-github-bot · 2020-09-09T00:13:15Z

@glaringlee merged this pull request in 6dd53fb.

ngimel · 2020-09-09T03:58:13Z

test/test_nn.py

+    def test_embedding_bag_non_contiguous_weight(self, device, dtype):
+        weight_tensor = torch.randn(4, 3, dtype=dtype, device=device)
+
+        weight_tensor_non_contig = weight_tensor[:, :3]  # This is non-contiguous strided.


this is contiguous tensor

Right! Great catch!!
Was supposed to be

weight_tensor = torch.randn(3, 4, dtype=dtype, device=device)

@kshitij12345 I'll put a fix for u.

Sure. Thanks! Btw I am free this evening so I can put it up as well. Let me know if I should.

Ah, no worries. I have a clean pytorch repo on hand, will patch this shortly.

glaringlee · 2020-09-09T14:47:22Z

#44382 is entered for patching this.

Summary: Pull Request resolved: #44382 This is to fix a typo that introduced in #44032. Test Plan: Imported from OSS Reviewed By: mrshenli Differential Revision: D23601316 Pulled By: glaringlee fbshipit-source-id: 17d6de5900443ea46c7a6ee9c7614fe6f2d92890

dzhulgakov · 2020-09-11T20:30:34Z

aten/src/ATen/native/EmbeddingBag.cpp

  auto* output_data = output.data_ptr<float>();

  if (isFastPathIndexSelect(src, output)) {
+    auto* src_data = src.contiguous().data_ptr<float>();


wait, this is still a bug, right? If the tensor is non-contiguous then contiguous() will return a new tensor and it will be immediately destroyed (because we don't keep a reference to it around). So src_data will point to the deallocated memory :(

I wonder why ASAN doesn't catch it.

It should be

auto src_contig = src.contiguous(); auto* src_data = src_contig.data_ptr<float>();

cc @glaringlee @ngimel

@dzhulgakov oh, shoot.........my bad, will fix soon.

fix dangling ptr in #44032 Differential Revision: [D23661007](https://our.internmc.facebook.com/intern/diff/D23661007) [ghstack-poisoned]

fix

fc420fd

use weight.contiguous on fast-path as it expects contiguous tensor.

pytorchbot added the open source label Sep 2, 2020

smessmer requested a review from glaringlee September 2, 2020 21:15

smessmer added module: cuda Related to torch.cuda, and CUDA support in general triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module module: operators and removed module: cuda Related to torch.cuda, and CUDA support in general labels Sep 2, 2020

add relevant test

71ea404

glaringlee reviewed Sep 3, 2020

View reviewed changes

aten/src/ATen/native/EmbeddingBag.cpp Outdated Show resolved Hide resolved

aten/src/ATen/native/EmbeddingBag.cpp Outdated Show resolved Hide resolved

address comment

fe6961b

* use method chaining.

facebook-github-bot reviewed Sep 3, 2020

View reviewed changes

glaringlee approved these changes Sep 4, 2020

View reviewed changes

facebook-github-bot closed this in 6dd53fb Sep 8, 2020

facebook-github-bot added the merged label Sep 9, 2020

kshitij12345 deleted the fix/embedding-bag/fast-path branch September 9, 2020 03:40

ngimel reviewed Sep 9, 2020

View reviewed changes

glaringlee mentioned this pull request Sep 9, 2020

fix typo in embedding_bag_non_contiguous_weight test #44382

Closed

dzhulgakov reviewed Sep 11, 2020

View reviewed changes

glaringlee mentioned this pull request Sep 11, 2020

fix dangling ptr in embedding_bag #44571

Closed

glaringlee pushed a commit that referenced this pull request Sep 11, 2020

Update on "fix dangling ptr in embedding_bag"

9791096

fix dangling ptr in #44032 Differential Revision: [D23661007](https://our.internmc.facebook.com/intern/diff/D23661007) [ghstack-poisoned]

mruberry added the Merged label Oct 28, 2020

[fix] output of embedding_bag with non-contiguous weight #44032

[fix] output of embedding_bag with non-contiguous weight #44032

Uh oh!

Conversation

kshitij12345 commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 2 failed

Uh oh!

codecov bot commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

glaringlee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glaringlee commented Sep 3, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

glaringlee left a comment

Choose a reason for hiding this comment

Uh oh!

kshitij12345 commented Sep 8, 2020

Uh oh!

glaringlee commented Sep 8, 2020

Uh oh!

kshitij12345 commented Sep 8, 2020

Uh oh!

facebook-github-bot commented Sep 9, 2020

Uh oh!

ngimel Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

kshitij12345 Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

glaringlee Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

kshitij12345 Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glaringlee Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

glaringlee commented Sep 9, 2020

Uh oh!

dzhulgakov Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

glaringlee Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

[fix] output of `embedding_bag` with non-contiguous weight #44032

[fix] output of `embedding_bag` with non-contiguous weight #44032

kshitij12345 commented Sep 2, 2020 •

edited

Loading

dr-ci bot commented Sep 2, 2020 •

edited

Loading

codecov bot commented Sep 2, 2020 •

edited

Loading

kshitij12345 Sep 9, 2020 •

edited

Loading