Skip to content

Propagate CreationMeta when chaining views#51061

Closed
jbschlosser wants to merge 3 commits intopytorch:masterfrom
jbschlosser:view_chaining_creation_meta
Closed

Propagate CreationMeta when chaining views#51061
jbschlosser wants to merge 3 commits intopytorch:masterfrom
jbschlosser:view_chaining_creation_meta

Conversation

@jbschlosser
Copy link
Copy Markdown
Contributor

@jbschlosser jbschlosser commented Jan 25, 2021

Fixes #49824

Background

When creating a view of a view, there was a possibility that the new view would be less restrictive than the previous view, incorrectly sidestepping the error that should be thrown when using in-place operations on the new view.

The fix addresses this by propagating CreationMeta from the previous view to the new view. Currently, the old view's creation_meta is only propagated when the new view's creation_meta == CreationMeta::DEFAULT. This ensures that the new view is not less restrictive than the previous view wrt. allowing in-place operations.

Test Plan

python test/test_autograd.py TestAutogradDeviceTypeCPU.test_inplace_view_of_multiple_output_view_cpu
python test/test_autograd.py TestAutogradDeviceTypeCUDA.test_inplace_view_of_multiple_output_view_cuda
python test/test_autograd.py TestAutogradDeviceTypeCPU.test_inplace_multiple_output_view_of_view_cpu
python test/test_autograd.py TestAutogradDeviceTypeCUDA.test_inplace_multiple_output_view_of_view_cuda

BC-Breaking Note

After this fix, an error will properly be thrown to avoid wrong gradients when an in-place operation is performed on a view of a view, where the second view has creation_meta == CreationMeta::DEFAULT (e.g. a standard single output view) while the first view has creation_meta != CreationMeta::DEFAULT (e.g. a multiple output or other restricted view).

Since previous behavior allowed such in-place operations without an error, this change is BC-breaking.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jan 25, 2021

💊 CI failures summary and remediations

As of commit 895554f (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 to the (internal) Dr. CI Users group.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 26, 2021

Codecov Report

Merging #51061 (6b486e7) into master (e34992e) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #51061      +/-   ##
==========================================
- Coverage   80.99%   80.99%   -0.01%     
==========================================
  Files        1916     1916              
  Lines      209552   209558       +6     
==========================================
- Hits       169736   169729       -7     
- Misses      39816    39829      +13     

Comment thread torch/csrc/autograd/VariableTypeUtils.h Outdated
Comment thread torch/csrc/autograd/VariableTypeUtils.h Outdated
Copy link
Copy Markdown
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@jbschlosser merged this pull request in 0b5303e.

@gchanan gchanan added the module: bc-breaking Related to a BC-breaking change label Feb 2, 2021
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
Summary:
Fixes pytorch#49824

## Background

When creating a view of a view, there was a possibility that the new view would be less restrictive than the previous view, incorrectly sidestepping the error that should be thrown when using in-place operations on the new view.

The fix addresses this by propagating `CreationMeta` from the previous view to the new view. Currently, the old view's `creation_meta` is only propagated when the new view's `creation_meta == CreationMeta::DEFAULT`. This ensures that the new view is not less restrictive than the previous view wrt. allowing in-place operations.

Pull Request resolved: pytorch#51061

Test Plan:
```
python test/test_autograd.py TestAutogradDeviceTypeCPU.test_inplace_view_of_multiple_output_view_cpu
python test/test_autograd.py TestAutogradDeviceTypeCUDA.test_inplace_view_of_multiple_output_view_cuda
python test/test_autograd.py TestAutogradDeviceTypeCPU.test_inplace_multiple_output_view_of_view_cpu
python test/test_autograd.py TestAutogradDeviceTypeCUDA.test_inplace_multiple_output_view_of_view_cuda
```

Reviewed By: heitorschueroff

Differential Revision: D26076434

Pulled By: jbschlosser

fbshipit-source-id: c47f0ddcef9b8449427b671aff9ad08edca70fcd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged module: bc-breaking Related to a BC-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

View CreationMeta are not propagated when chaining views

4 participants