Implement public API InferenceMode and its error handling by ailzhang · Pull Request #53343 · pytorch/pytorch

ailzhang · 2021-03-05T06:09:17Z

Stack from ghstack:

Relax some limitations of InferenceMode. #54403 Relax some limitations of InferenceMode.
Implement public API InferenceMode and its error handling #53343 Implement public API InferenceMode and its error handling

RFC: pytorch/rfcs#17

Differential Revision: D26973911

[ghstack-poisoned]

facebook-github-bot · 2021-03-05T06:10:00Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR
- 1/1 non-scanned failure(s)

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.

…handling" [ghstack-poisoned]

ghstack-source-id: 4d70e5a Pull Request resolved: #53343

…handling" [ghstack-poisoned]

ghstack-source-id: 1323176 Pull Request resolved: #53343

…handling" [ghstack-poisoned]

ghstack-source-id: ebd0d1a Pull Request resolved: #53343

…handling" [ghstack-poisoned]

ghstack-source-id: 86aeee4 Pull Request resolved: #53343

…handling" [ghstack-poisoned]

ghstack-source-id: 97f46a8 Pull Request resolved: #53343

…handling" [ghstack-poisoned]

ghstack-source-id: b547d87 Pull Request resolved: #53343

…handling" [ghstack-poisoned]

ghstack-source-id: ed3f4b6 Pull Request resolved: #53343

…handling" [ghstack-poisoned]

ezyang · 2021-03-24T22:37:23Z

Was reviewing the spec some more and I think we can probably do some more safe relaxations:

Taking a view on an inference mode tensor outside of inference mode is safe, you just need to produce an inference tensor in this case as well (currently this is an error)
Inplace modifying an inference mode tensor outside of inference mode is safe and you can skip the VC bump

(Don't block this PR on it)

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

bhosmer

This LGTM per Ed's analysis here and offline conversation 😁 one note about setting requires_grad, but if we want to do anything about that it can definitely wait for a followup.

bhosmer · 2021-03-25T02:02:31Z

test/cpp/api/inference_mode.cpp

+    ASSERT_TRUE(is_inference_tensor(c));
+
+    torch::Tensor tmp = torch::ones({1, 2, 3}).set_requires_grad(true);
+    ASSERT_TRUE(tmp.requires_grad()); // requires_grad is silently ignored when it's an inference tensor.


Did we decide there was a reason we wanted to silently ignore, like code reuse? Seems clear it would be saner devex to actually error if you set requires_grad true on an inference tensor.

The code got edited above, but just to be super explicit: it's not silently ignored; we DO set requires_grad=True (just like in no grad mode); the idea to not error is so that code that initializes parameters can run in inference mode without triggering errors

Yea code reuse is the main reason. And another reason is inference tensor can actually have requires_grad=true (and gradients) as shown in the second PR. :D

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

albanD

One case that I don't see mentioned here is:

a = torch.rand(10, requires_grad=True)
with inference_mode():
    b = a.view_as(a)
with torch.no_grad():
    c = b.view_as(b)
c += 1

I guess this is fine, but just want to make sure

pytorch/torch/csrc/autograd/variable.h

Lines 508 to 515 in dfc7fa0

    
           /// Handles correctly propagating CreationMeta when a new view is created from a previous view. 
        
           /// In general, we don't want the new view to be _less_ restrictive than the previous view 
        
           /// (it's okay to be _more_ restrictive). A CreationMeta value of DEFAULT is currently the least 
        
           /// restrictive, as the behavior for all other CreationMeta values is to error out for in-place ops. 
        
           /// If this changes, the logic here will need to be updated to properly handle the new semantics. 
        
           inline CreationMeta propagate_creation_meta(CreationMeta prev_view_creation_meta, CreationMeta new_view_creation_meta) { 
        
             return (new_view_creation_meta == CreationMeta::DEFAULT) ? prev_view_creation_meta : new_view_creation_meta; 
        
           }

handles that case properly.

Also after looking at all the examples, I am now convinced that we should present inference mode as no grad but more restrictive. As most of the behavior is actually the same.

c10/core/InferenceMode.h

c10/core/InferenceMode.cpp

c10/core/TensorImpl.h

test/cpp/api/inference_mode.cpp

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

albanD

LGTM!
Thanks for all the updates!

facebook-github-bot · 2021-03-27T20:46:11Z

@ailzhang merged this pull request in 7caa464.

facebook-github-bot · 2021-03-29T18:19:33Z

This pull request has been reverted by 263180d.

) Summary: Pull Request resolved: pytorch#55008 reland of pytorch#53343 For easier review, here's a diff between the version before revert. https://www.internalfb.com/phabricator/paste/view/P361764610 Test Plan: Imported from OSS Differential Revision: D27443229 Pulled By: ailzhang fbshipit-source-id: faeaff3b6165b933c9f354d5f0344e38269fbb12

Summary: https://www.internalfb.com/phabricator/paste/view/P360377337Pull Request resolved: #53343 For easier review, here's a diff between the version before revert. https://www.internalfb.com/phabricator/paste/view/P360750919 Pull Request resolved: #55008 Test Plan: Imported from OSS Pulled By: ailzhang Reviewed By: bhosmer Differential Revision: D27443229 fbshipit-source-id: 01b03446a1f6373f43dd5c7170d26226b50f363c

Implement public API AutoInferenceMode and its error handling

3665666

[ghstack-poisoned]

ailzhang requested review from albanD, glaringlee and soulitzer as code owners March 5, 2021 06:09

This was referenced Mar 5, 2021

Move common autograd utils functions from gen_variable_type.py to api/autograd.py. #53340

Closed

Move view handling logic to gen_inplace_or_view_type.py #53341

Closed

Move as_view/increment_version to its separate key. #53342

Closed

facebook-github-bot added the cla signed label Mar 5, 2021

Update on "[WIP]Implement public API AutoInferenceMode and its error …

30f85fc

…handling" [ghstack-poisoned]

ailzhang changed the title ~~Implement public API AutoInferenceMode and its error handling~~ [WIP]Implement public API AutoInferenceMode and its error handling Mar 5, 2021

Update on "[WIP]Implement public API AutoInferenceMode and its error …

bba050e

…handling" [ghstack-poisoned]

ailzhang pushed a commit that referenced this pull request Mar 5, 2021

[WIP]Implement public API AutoInferenceMode and its error handling

25116eb

ghstack-source-id: 4d70e5a Pull Request resolved: #53343

Update on "[WIP]Implement public API AutoInferenceMode and its error …

4f1a8f3

…handling" [ghstack-poisoned]

Update on "[WIP]Implement public API AutoInferenceMode and its error …

d67a1b9

…handling" [ghstack-poisoned]

ailzhang pushed a commit that referenced this pull request Mar 6, 2021

[WIP]Implement public API AutoInferenceMode and its error handling

3441f69

ghstack-source-id: 1323176 Pull Request resolved: #53343

Update on "[WIP]Implement public API AutoInferenceMode and its error …

0b99626

…handling" [ghstack-poisoned]

ailzhang pushed a commit that referenced this pull request Mar 8, 2021

[WIP]Implement public API AutoInferenceMode and its error handling

309cbbc

ghstack-source-id: ebd0d1a Pull Request resolved: #53343

Update on "[WIP]Implement public API AutoInferenceMode and its error …

bcb017c

…handling" [ghstack-poisoned]

ailzhang pushed a commit that referenced this pull request Mar 8, 2021

[WIP]Implement public API AutoInferenceMode and its error handling

6f255cd

ghstack-source-id: 86aeee4 Pull Request resolved: #53343

Update on "[WIP]Implement public API AutoInferenceMode and its error …

6f5f887

…handling" [ghstack-poisoned]

ailzhang pushed a commit that referenced this pull request Mar 9, 2021

[WIP]Implement public API AutoInferenceMode and its error handling

70fc976

ghstack-source-id: 97f46a8 Pull Request resolved: #53343

Update on "[WIP]Implement public API AutoInferenceMode and its error …

1ab6f47

…handling" [ghstack-poisoned]

ailzhang pushed a commit that referenced this pull request Mar 9, 2021

[WIP]Implement public API AutoInferenceMode and its error handling

7a80754

ghstack-source-id: b547d87 Pull Request resolved: #53343

Update on "[WIP]Implement public API AutoInferenceMode and its error …

ecd0367

…handling" [ghstack-poisoned]

ailzhang pushed a commit that referenced this pull request Mar 10, 2021

[WIP]Implement public API AutoInferenceMode and its error handling

1a1dc0b

ghstack-source-id: ed3f4b6 Pull Request resolved: #53343

Update on "[WIP]Implement public API AutoInferenceMode and its error …

ecc5086

…handling" [ghstack-poisoned]

Ailing Zhang added 3 commits March 25, 2021 01:54

Update on "Implement public API InferenceMode and its error handling"

80d80ac

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

Update on "Implement public API InferenceMode and its error handling"

2fbefe9

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

Update on "Implement public API InferenceMode and its error handling"

5c0ebd8

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

bhosmer approved these changes Mar 25, 2021

View reviewed changes

Ailing Zhang added 2 commits March 25, 2021 06:48

Update on "Implement public API InferenceMode and its error handling"

372cba8

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

Update on "Implement public API InferenceMode and its error handling"

1620cce

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

ailzhang requested a review from bdhirsh March 25, 2021 18:20

albanD reviewed Mar 25, 2021

View reviewed changes

Update on "Implement public API InferenceMode and its error handling"

6e5dece

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

ailzhang requested a review from albanD March 26, 2021 17:06

Update on "Implement public API InferenceMode and its error handling"

09f5448

RFC: pytorch/rfcs#17 Differential Revision: [D26973911](https://our.internmc.facebook.com/intern/diff/D26973911) [ghstack-poisoned]

albanD approved these changes Mar 26, 2021

View reviewed changes

facebook-github-bot closed this in 7caa464 Mar 27, 2021

facebook-github-bot added the Merged label Mar 27, 2021

facebook-github-bot added the Reverted label Mar 29, 2021

ailzhang mentioned this pull request Mar 30, 2021

Implement public API InferenceMode and its error handling #55008

Closed

facebook-github-bot deleted the gh/ailzhang/51/head branch March 31, 2021 14:17

	/// Handles correctly propagating CreationMeta when a new view is created from a previous view.
	/// In general, we don't want the new view to be _less_ restrictive than the previous view
	/// (it's okay to be _more_ restrictive). A CreationMeta value of DEFAULT is currently the least
	/// restrictive, as the behavior for all other CreationMeta values is to error out for in-place ops.
	/// If this changes, the logic here will need to be updated to properly handle the new semantics.
	inline CreationMeta propagate_creation_meta(CreationMeta prev_view_creation_meta, CreationMeta new_view_creation_meta) {
	return (new_view_creation_meta == CreationMeta::DEFAULT) ? prev_view_creation_meta : new_view_creation_meta;
	}

Conversation

ailzhang commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

ezyang commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhosmer left a comment

Choose a reason for hiding this comment

Uh oh!

bhosmer Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

ailzhang Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 27, 2021

Uh oh!

facebook-github-bot commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ailzhang commented Mar 5, 2021 •

edited

Loading

facebook-github-bot commented Mar 5, 2021 •

edited

Loading

ezyang commented Mar 24, 2021 •

edited

Loading