[JIT] Add basic aliasing checks for tensor inputs #79474

ttanpcs · 2022-06-13T23:19:32Z

Stack from ghstack (oldest at bottom):

Added aliasing checks in SchemaCheckMode that raise a RuntimeError if an argument aliases an output that is not specified by the matching schema
Added python bindings for isAliasOf: a method that checks if two tensors are aliases of each other, before_set: which returns the before sets and after_set: which returns the after sets.
Created IncorrectAliasTensor as a subclass of torch.Tensor that purposefully aliases incorrectly
Tested that alias checks in SchemaCheckMode work for both correct and incorrect situations

Note that the cases of inputs aliasing with each other, outputs aliasing with each other, and cases where the before and after sets are not equivalent (this should only occur when the after set is the wildcard set) are not implemented. These cases will be addressed in a later pr.

[ghstack-poisoned]

facebook-github-bot · 2022-06-13T23:19:39Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/79474
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

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

Expand to see more

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

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

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

davidberard98

Overall looks good! Just had one major comment, and a few minor suggestions

test/jit/test_schema_check.py

torch/csrc/jit/python/init.cpp

davidberard98 · 2022-06-13T23:44:47Z

torch/testing/_internal/schema_check_mode.py

+                after = tree_flatten(arguments.get(name))[0]
                if (any([has_mutated(i, j) for i, j in zip(before, after)]) and not argument.is_mutable):
                    raise RuntimeError(f"Argument {name} is not defined as mutable but was mutated")
+                for i in before:


very minor nit: i feel like i is usually the name of an index, maybe rename it b? totally not important though, you can ignore if you don't want to change it.

torch/testing/_internal/schema_check_mode.py

davidberard98 · 2022-06-13T23:51:39Z

test/jit/test_schema_check.py

+# This TorchDispatchTensor Subclass is used to test incorrectly aliasing ops
+# This is done by aliasing the output to the first argument for all ops in INCORRECT_OPS
+
+class IncorrectAliasTensor(torch.Tensor):


awesome! This could be pretty useful in the future too if we get around to fixing batch_norm (in which case we would need to find a new way to test mutation)

- Added aliasing checks in SchemaCheckMode that raise a RuntimeError if an argument aliases an output that is not specified by the matching schema - Added python bindings for isAliasOf: a method that checks if two tensors are aliases of each other, before_set: which returns the before sets and after_set: which returns the after sets. - Created IncorrectAliasTensor as a subclass of torch.Tensor that purposefully aliases incorrectly - Tested that alias checks in SchemaCheckMode work for both correct and incorrect situations **Note that the cases of inputs aliasing with each other, outputs aliasing with each other, and cases where the before and after sets are not equivalent (this should only occur when the after set is the wildcard set) are not implemented. These cases will be addressed in a later pr.** [ghstack-poisoned]

ghstack-source-id: 3dc05af Pull Request resolved: #79474

bdhirsh · 2022-06-16T18:08:50Z

torch/csrc/jit/python/init.cpp

          py::call_guard<py::gil_scoped_release>());
-
+  m.def("_is_alias_of", [](const at::Tensor& self, const at::Tensor& other) {
+    return IValue(self).isAliasOf(IValue(other));


nit: any reason why we're converting the tensors to IValues first? There's also a Tensor::is_alias_of method.

bdhirsh · 2022-06-16T18:13:42Z

test/jit/test_schema_check.py

                       "instead.")

+# This TorchDispatchTensor Subclass is used to test incorrectly aliasing ops
+# This is done by aliasing the output to the first argument for all ops in INCORRECT_OPS


minor nit: the comment says that this subclass is used to test incorrectly aliasing ops, but technically it's SchemaCheckMode that's responsible for that. It looks like the main purpose of this class is to test that SchemaCheckMode behaves as expected, by "simulating" an op with an incorrect schema and asserting that the mode picks up the error.

Do you mind updating the description a bit?

Can you make it a bit clearer in this comment that the purpose of this class is basically as a test case for schema_check_mode? It doesn't look like it's act

bdhirsh · 2022-06-16T18:14:07Z

test/jit/test_schema_check.py

+
+    # Tests that an exception is raised for a mismatching alias
+    def test_alias_check_fail(self):
+        with self.assertRaises(RuntimeError):


nit: for best practice, can you use self.assertRaisesRegex(RuntimeError, "my expected error message")?

- Added aliasing checks in SchemaCheckMode that raise a RuntimeError if an argument aliases an output that is not specified by the matching schema - Added python bindings for isAliasOf: a method that checks if two tensors are aliases of each other, before_set: which returns the before sets and after_set: which returns the after sets. - Created IncorrectAliasTensor as a subclass of torch.Tensor that purposefully aliases incorrectly - Tested that alias checks in SchemaCheckMode work for both correct and incorrect situations **Note that the cases of inputs aliasing with each other, outputs aliasing with each other, and cases where the before and after sets are not equivalent (this should only occur when the after set is the wildcard set) are not implemented. These cases will be addressed in a later pr.** [ghstack-poisoned]

ghstack-source-id: 39d5a14 Pull Request resolved: #79474

- Added aliasing checks in SchemaCheckMode that raise a RuntimeError if an argument aliases an output that is not specified by the matching schema - Added python bindings for isAliasOf: a method that checks if two tensors are aliases of each other, before_set: which returns the before sets and after_set: which returns the after sets. - Created IncorrectAliasTensor as a subclass of torch.Tensor that purposefully aliases incorrectly - Tested that alias checks in SchemaCheckMode work for both correct and incorrect situations **Note that the cases of inputs aliasing with each other, outputs aliasing with each other, and cases where the before and after sets are not equivalent (this should only occur when the after set is the wildcard set) are not implemented. These cases will be addressed in a later pr.** [ghstack-poisoned]

davidberard98

LGTM! just had one comment, please add that and then you can merge once CI is green

davidberard98 · 2022-06-17T17:18:48Z

torch/testing/_internal/schema_check_mode.py

+            return bool(len(lhs_argument.before_set & rhs_argument.before_set))
+
+        def unwrap(e):
+            if isinstance(e, torch.Tensor) and not type(e) == torch.Tensor:


nit: could you add a comment about why you need this? (e.g. to handle IncorrectAliasTensor)

ttanpcs · 2022-06-17T19:47:44Z

@pytorchbot merge

pytorchmergebot · 2022-06-17T19:51:46Z

@pytorchbot successfully started a merge job. Check the current status here

github-actions · 2022-06-17T19:52:25Z

Hey @goldenxuett.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Pull Request resolved: #79474 Approved by: https://github.com/davidberard98 Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/1432a3d6acc7ac58269e5ea7ef6b2373e9a3dbf6 Reviewed By: malfet Differential Revision: D37278870 Pulled By: goldenxuett fbshipit-source-id: 2316e8ac4e368fedc8c0d6dd9a4ffb28bd33571c

[JIT] Add basic aliasing checks for tensor inputs

ef158d4

[ghstack-poisoned]

ttanpcs mentioned this pull request Jun 13, 2022

[JIT] Change SchemaCheckTensor into SchemaCheckMode and fix global variable issues #79473

Closed

facebook-github-bot added the cla signed label Jun 13, 2022

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jun 13, 2022

ttanpcs requested review from Gamrix, Krovatkin, bdhirsh, davidberard98 and eellison and removed request for davidberard98 June 13, 2022 23:26

davidberard98 reviewed Jun 13, 2022

View reviewed changes

ttanpcs pushed a commit that referenced this pull request Jun 14, 2022

[JIT] Add basic aliasing checks for tensor inputs

cdc19c5

ghstack-source-id: 3dc05af Pull Request resolved: #79474

ttanpcs requested a review from davidberard98 June 14, 2022 00:37

ttanpcs mentioned this pull request Jun 16, 2022

[JIT] Bind AliasInfo to decrease differences in interfaces across languages #79661

Closed

bdhirsh reviewed Jun 16, 2022

View reviewed changes

ttanpcs pushed a commit that referenced this pull request Jun 16, 2022

[JIT] Add basic aliasing checks for tensor inputs

8aa56ab

ghstack-source-id: 39d5a14 Pull Request resolved: #79474

ttanpcs requested a review from bdhirsh June 17, 2022 04:24

davidberard98 approved these changes Jun 17, 2022

View reviewed changes

pytorchmergebot added the Merged label Jun 17, 2022

pytorchmergebot closed this in 1432a3d Jun 17, 2022

ttanpcs mentioned this pull request Jun 17, 2022

[JIT] Handle aliasing cases where two arguments alias in SchemaCheckMode #79814

Closed

[JIT] Add basic aliasing checks for tensor inputs #79474

[JIT] Add basic aliasing checks for tensor inputs #79474

Uh oh!

Conversation

ttanpcs commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

davidberard98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidberard98 Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidberard98 Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

davidberard98 left a comment

Choose a reason for hiding this comment

Uh oh!

davidberard98 Jun 17, 2022

Choose a reason for hiding this comment

Uh oh!

ttanpcs commented Jun 17, 2022

Uh oh!

pytorchmergebot commented Jun 17, 2022

Uh oh!

github-actions bot commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ttanpcs commented Jun 13, 2022 •

edited

Loading

facebook-github-bot commented Jun 13, 2022 •

edited

Loading