Check for internal memory overlap in some indexing-type functions #43423

peterbell10 · 2020-08-21T19:55:45Z

Stack from ghstack:

Check for internal memory overlap in some indexing-type functions #43423 Check for internal memory overlap in some indexing-type functions
TensorIteratorConfig: Check memory overlap by default #43422 TensorIteratorConfig: Check memory overlap by default

Differential Revision: D23298652

[ghstack-poisoned]

ghstack-source-id: 66e4daa Pull Request resolved: #43423

dr-ci · 2020-08-21T20:47:57Z

💊 CI failures summary and remediations

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

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

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 38 times.

…nctions" [ghstack-poisoned]

ghstack-source-id: 2e93fdc Pull Request resolved: #43423

ngimel · 2020-08-25T03:58:28Z

cc @neerajprad @fritzo for changes to distribution tests. My impression was that broadcasted tensors are routinely used in distributions, so requiring non-self-overlapping outputs could lead to significant memory penaly.

peterbell10 · 2020-08-25T13:24:40Z

torch/distributions/geometric.py

-        value, probs = broadcast_all(value, self.probs.clone(memory_format=torch.contiguous_format))
+        value, probs = broadcast_all(value, self.probs)
+        probs = probs.clone(memory_format=torch.contiguous_format)
        probs[(probs == 1) & (value == 0)] = 0


My impression was that broadcasted tensors are routinely used in distributions, so requiring non-self-overlapping outputs could lead to significant memory penaly.

This is a bug fix. If probs is broadcasted here, then this line can write to locations where value != 0 if it happens to overlap with a location where value is zero. The full tensor size will be required in the following calculation anyway, so I think this is fine.

The same reasoning applies to the change in multinomial.

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

ghstack-source-id: fbf48d6 Pull Request resolved: #43423

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

ghstack-source-id: 0d611f0 Pull Request resolved: #43423

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

ghstack-source-id: 930aca1 Pull Request resolved: #43423

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

ghstack-source-id: cef91a6 Pull Request resolved: #43423

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

ghstack-source-id: 5b34dba Pull Request resolved: #43423

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

ghstack-source-id: e39cf70 Pull Request resolved: #43423

codecov · 2020-08-29T19:54:32Z

Codecov Report

❗ No coverage uploaded for pull request base (gh/peterbell10/9/base@bc2fcac). Click here to learn what that means.
The diff coverage is n/a.

@@                   Coverage Diff                    @@
##             gh/peterbell10/9/base   #43423   +/-   ##
========================================================
  Coverage                         ?   69.31%           
========================================================
  Files                            ?      378           
  Lines                            ?    46747           
  Branches                         ?        0           
========================================================
  Hits                             ?    32405           
  Misses                           ?    14342           
  Partials                         ?        0

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 bc2fcac...84b1711. Read the comment docs.

fritzo

LGTM thanks for fixing this!

@ngimel I believe the only place where broadcasting would help reduce memory footprint is in MyDist(...),log_prob(), but those already perform arithmetic operations like (-probs).log1p() so I believe max extra overhead of this PR would be +25%.

fritzo · 2020-08-31T00:33:57Z

Can someone else please review the added at::assert_no_overlap()s?

zou3519 · 2020-08-31T14:14:38Z

Can someone else please review the added at::assert_no_overlap()s?

I can review it after I finish reviewing #43422

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

ghstack-source-id: b92298f Pull Request resolved: #43423

zou3519

Some not-uncommon use cases of masked_fill_ and index_put_ are idempotent. For example, I've seen user code out in the wild that tries to zero-out nans that does the following, but I don't know how many users actually try to do this on broadcasted tensors.

x[torch.isnan(x)] = 0
y.masked_fill_(torch.isnan(y), 0)

The other operators in this PR look fine, though.

My opinion is to not change masked_fill_ and index_put_, at least not in this PR, because it is a common paradigm to do x[x == blah] = value. If we do decide we want to fix masked_fill_ and index_put_, I think we should consider that change to be BC-breaking and have a deprecation cycle.

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 · 2020-09-01T17:47:56Z

The original issue (#39639) was originally about masked_fill_ specifically, so I don't want to ignore it completely here. I've lowered it to a deprecation warning. But I could move that to another PR as well if you prefer.

zou3519 · 2020-09-01T19:25:09Z

aten/src/ATen/native/TensorAdvancedIndexing.cpp

+    TORCH_WARN("Use of index_put_ on expanded tensors is deprecated. "
+               "Please clone() the tensor before performing this operation.");


Another way to get here is via advanced indexing. We should talk about that as well in the error message so that the user can connect these two ops. "Use of index_put_ on expanded tensors is deprecated. Please clone() the tensor before performing this operation. You may also encounter this error message when assigning items to a tensor using advanced indexing, that is also deprecated"

Good idea. Added a note in the warning for index_put_ and also masked_fill_ as well.

zou3519 · 2020-09-01T19:25:45Z

The original issue (#39639) was originally about masked_fill_ specifically, so I don't want to ignore it completely here. I've lowered it to a deprecation warning. But I could move that to another PR as well if you prefer.

Sure, let's do the deprecation warning in this PR then

zou3519

Let's add a mention of advanced indexing into the index_put_ deprecation. After that this should be good to go

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

ghstack-source-id: ba986a4 Pull Request resolved: #43423

facebook-github-bot · 2020-09-02T16:14:02Z

@zou3519 merged this pull request in c88ac25.

Check for internal memory overlap in some indexing-type functions

b32ce2e

[ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 21, 2020

Check for internal memory overlap in some indexing-type functions

0dfc142

ghstack-source-id: 66e4daa Pull Request resolved: #43423

pytorchbot added the open source label Aug 21, 2020

Update on "Check for internal memory overlap in some indexing-type fu…

bcca709

…nctions" [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 21, 2020

Check for internal memory overlap in some indexing-type functions

13abcd1

ghstack-source-id: 2e93fdc Pull Request resolved: #43423

peterbell10 requested a review from zou3519 August 22, 2020 16:01

ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 25, 2020

peterbell10 commented Aug 25, 2020

View reviewed changes

Update on "Check for internal memory overlap in some indexing-type fu…

d0a01aa

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 25, 2020

Check for internal memory overlap in some indexing-type functions

d06e540

ghstack-source-id: fbf48d6 Pull Request resolved: #43423

Update on "Check for internal memory overlap in some indexing-type fu…

0d2f30e

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 25, 2020

Check for internal memory overlap in some indexing-type functions

82feb6a

ghstack-source-id: 0d611f0 Pull Request resolved: #43423

peterbell10 mentioned this pull request Aug 26, 2020

Skip memory overlap checking tests pytorch/xla#2464

Merged

Update on "Check for internal memory overlap in some indexing-type fu…

7177026

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 26, 2020

Check for internal memory overlap in some indexing-type functions

9895651

ghstack-source-id: 930aca1 Pull Request resolved: #43423

Update on "Check for internal memory overlap in some indexing-type fu…

d14e584

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 27, 2020

Check for internal memory overlap in some indexing-type functions

b6de756

ghstack-source-id: cef91a6 Pull Request resolved: #43423

Update on "Check for internal memory overlap in some indexing-type fu…

13f6284

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 29, 2020

Check for internal memory overlap in some indexing-type functions

1c3cec0

ghstack-source-id: 5b34dba Pull Request resolved: #43423

Update on "Check for internal memory overlap in some indexing-type fu…

4d40ceb

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 29, 2020

Check for internal memory overlap in some indexing-type functions

fa58700

ghstack-source-id: e39cf70 Pull Request resolved: #43423

fritzo approved these changes Aug 31, 2020

View reviewed changes

Update on "Check for internal memory overlap in some indexing-type fu…

5e5f1dc

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

Update on "Check for internal memory overlap in some indexing-type fu…

9735a65

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Aug 31, 2020

Check for internal memory overlap in some indexing-type functions

70ed56f

ghstack-source-id: b92298f Pull Request resolved: #43423

zou3519 reviewed Aug 31, 2020

View reviewed changes

Update on "Check for internal memory overlap in some indexing-type fu…

757301b

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

zou3519 reviewed Sep 1, 2020

View reviewed changes

zou3519 added the module: deprecation label Sep 1, 2020

Update on "Check for internal memory overlap in some indexing-type fu…

84b1711

…nctions" Differential Revision: [D23298652](https://our.internmc.facebook.com/intern/diff/D23298652) [ghstack-poisoned]

peterbell10 added a commit that referenced this pull request Sep 1, 2020

Check for internal memory overlap in some indexing-type functions

b066d81

ghstack-source-id: ba986a4 Pull Request resolved: #43423

zou3519 approved these changes Sep 1, 2020

View reviewed changes

facebook-github-bot closed this in c88ac25 Sep 2, 2020

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

This was referenced Sep 2, 2020

Check for memory overlap in more ATen ops #39878

Closed

masked_fill_ (and possibly others) produces a different output than masked_fill on cpu #39639

Closed

facebook-github-bot deleted the gh/peterbell10/9/head branch September 6, 2020 14:16

mruberry added the Merged label Oct 28, 2020

		TORCH_WARN("Use of index_put_ on expanded tensors is deprecated. "
		"Please clone() the tensor before performing this operation.");

Check for internal memory overlap in some indexing-type functions #43423

Check for internal memory overlap in some indexing-type functions #43423

Uh oh!

Conversation

peterbell10 commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Uh oh!

ngimel commented Aug 25, 2020

Uh oh!

peterbell10 Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

fritzo left a comment • 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!

fritzo commented Aug 31, 2020

Uh oh!

zou3519 commented Aug 31, 2020

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

zou3519 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

peterbell10 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

zou3519 commented Sep 1, 2020

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

peterbell10 commented Aug 21, 2020 •

edited

Loading

dr-ci bot commented Aug 21, 2020 •

edited

Loading

codecov bot commented Aug 29, 2020 •

edited

Loading

fritzo left a comment •

edited

Loading

peterbell10 commented Sep 1, 2020 •

edited

Loading