[jit] fix side-effects and aliasing for custom ops #18711

suo · 2019-04-02T00:38:56Z

Stack from ghstack:

[jit] fix side-effects and aliasing for custom ops #18711 [jit] fix side-effects and aliasing for custom ops

Previously we didn't track aliasing, mutation, or side effects for
custom ops. This PR adds in guards with the most conservative
assumptions possible: the op will

have side effects,
write to everything
produce a wildcard.

In order to tell whether a given operator is a custom op, this PR introduces
the concept of a "reserved" namespace (basically all our builtin namespaces).
Custom ops live in non-reserved namespaces, so a check on the namespace
is sufficient to tell whether a schema/node is "custom" or not.

This is just to get things correct for now. Follow-ups to this:

Users should be able to specify aliasing/mutability without having to learn
the whole alias annotation schema.
Relax assumptions a bit. In particular outputs can only alias input tensors,
they don't have to be wildcards.

Fixes #18490

Differential Revision: D14730978

Previously we didn't track aliasing, mutation, or side effects for custom ops. This PR adds in guards with the most conservative assumptions possible: the op will 1) have side effects, 2) write to everything, and 3) produce a wildcard. We can reign in those restrictions later, but want to get something correct for now. It's a little weird because we have no way of knowing whether a given node represents a custom op--I added it to the schema but I'm open to suggestions for a better place. Follow-ups to this: - Users should be able to specify aliasing annotations/side effects to get better perf somehow. - Relax assumptions a bit.

eellison

Have a few comments but is just about ready I think

aten/src/ATen/core/function_schema.h

torch/csrc/jit/passes/alias_analysis.cpp

aten/src/ATen/core/function_schema.h

test/cpp/jit/test_custom_operators.h

suo · 2019-04-02T06:53:44Z

Okay, comments are addressed. This is only a short term fix to get things to be correct—I think in the long run, we should just use the options that #18589 introduces.

eellison

Looks good!

eellison · 2019-04-02T16:20:33Z

aten/src/ATen/core/function_schema.h

-          const auto& aliasInfo = arg.alias_info();
-          return aliasInfo && aliasInfo.value().isWrite();
-        });
+    return is_inferred_from_custom_op_ ||


So to be clear we are saying that it is mutable even if it doesn't have any inputs, add comment maybe ?

test/cpp/jit/test_custom_operators.h

apaszke · 2019-04-02T17:35:35Z

Can we just assume that aten::/prim::/... namespaces are not custom, everything else is?

suo · 2019-04-02T17:39:07Z

Can we just assume that aten::/prim::/... namespaces are not custom, everything else is?

That's an interesting idea…I don't think namespaces are restricted right now for custom ops, but it's easy to add. And we already have this distinction when emitting builtins.

apaszke · 2019-04-02T17:40:26Z

Yeah, we could expose a special subclass of RegisterOperator to the users (still aliased to RegisterOperator), which disallows registering ops under those namespaces.

eellison · 2019-04-02T18:25:56Z

Can we just assume that aten::/prim::/... namespaces are not custom, everything else is?

This would require changing some of our tests which use the test namespace (probably among others) and in general I think would be hard to maintain

suo · 2019-04-02T18:29:34Z

@eellison I think the idea is to designate "reserved" namespaces that users can't define custom ops in. I think this is good idea anyway (right now I think you could create an aten op that would participate in builtin schema resolution). Conveniently, we can also use the fact that a op has a non-reserved namespace to denote whether it's custom or not

eellison

Different implementation, have to re-review

just trying to undo accept

suo · 2019-04-02T20:11:57Z

test/cpp/jit/test_custom_operators.h

+  }
+  {
+    RegisterOperators reg(
+        {createOperator("foo::no_input", []() -> double { return 4.0; })});


this test causes windows build to fail

eellison · 2019-04-02T20:56:23Z

I think the most recent commit will make it harder to move forward to a future api where users declare ops as functional, because we will have ops that are in user space which we do want to assume mutate the inputs.

Operators in c10/caffe2/TVM will also no longer be able to create an operator with specified aliasing, because the namespace will override any information that has been given.

suo · 2019-04-02T21:17:11Z

Right now there is no way (short of writing alias annotations manually) for the user to specify the purity of their custom op, so we need to insert guards to make sure things are correct. I expect that when we give users the ability to specify purity, the guards will have to change.

zdevito

Code seems bug free, but see inline comments. I strongly suggest we switch this to whitelist aten and prim rather than special-case all primitive ops.

zdevito · 2019-04-03T19:03:55Z

aten/src/ATen/core/function_schema.h

+              return aliasInfo && aliasInfo.value().isWrite();
+            });
  }
+  bool is_custom_op() const {


For this PR, I think it would be safer to white-list prim and aten into the stronger alias behavior. I do not like the idea of making a publically-exposed distinction between custom ops and our ops, even for just a few commits.

zdevito · 2019-04-03T19:08:49Z

aten/src/ATen/core/interned_strings.h

+  // Does this symbol live in one of the above built-in namespaces?
+  // If not, that means that this symbol came from the user, through the
+  // defintiion of a custom operator.
+  bool is_builtin_ns() const;


Please do not commit this. I do not want to have people be able to easily special case stuff for built in ops. This will create huge long-term tech debt even if commit for a little while.

zdevito · 2019-04-03T19:10:07Z

torch/csrc/jit/custom_operator.h

      std::tuple_size<ArgumentTuple>::value;

  auto schema = torch::jit::detail::inferAndCheckSchema<Traits>(schemaOrName);
+  Symbol name = Symbol::fromQualString(schema.name());


I don't like this. I want to move our operator registration over to this API and this guard will prevent it. This will need to be removed in the future commit.

zdevito · 2019-04-03T19:10:48Z

torch/csrc/jit/ir.cpp

+    case prim::TimePoint:
      return true;
  }
+  // Custom ops may have arbitrary side effects


if (kind.is_aten() || kind.is_print()) return false

[jit] fix side-effects and aliasing for custom ops Previously we didn't track aliasing, mutation, or side effects for custom ops. This PR adds in guards with the most conservative assumptions possible: the op will 1) have side effects, 2) write to everything, and 3) produce a wildcard. We can reign in those restrictions later, but want to get something correct for now. It's a little weird because we have no way of knowing whether a given node represents a custom op--I added it to the schema but I'm open to suggestions for a better place. Follow-ups to this: - Users should be able to specify aliasing annotations/side effects to get better perf somehow. - Relax assumptions a bit. gh-metadata: pytorch pytorch 18711 gh/suo/15/head

Summary: Pull Request resolved: pytorch/pytorch#18711 ghimport-source-id: c9caedc0660b2b7ba3730cd0e1a2e0e9c3cf422b Stack from [ghstack](https://github.com/ezyang/ghstack): * **#18711 [jit] fix side-effects and aliasing for custom ops** Previously we didn't track aliasing, mutation, or side effects for custom ops. This PR adds in guards with the most conservative assumptions possible: the op will 1) have side effects, 2) write to everything 3) produce a wildcard. In order to tell whether a given operator is a custom op, this PR introduces the concept of a "reserved" namespace (basically all our builtin namespaces). Custom ops live in non-reserved namespaces, so a check on the namespace is sufficient to tell whether a schema/node is "custom" or not. This is just to get things correct for now. Follow-ups to this: - Users should be able to specify aliasing/mutability without having to learn the whole alias annotation schema. - Relax assumptions a bit. In particular outputs can only alias input tensors, they don't have to be wildcards. Fixes #18490 Differential Revision: D14730978 fbshipit-source-id: 540b47a24ccf24145051609bdcc99c97e46e0fe0

facebook-github-bot · 2019-04-05T19:41:47Z

This pull request has been merged in fefa6d3.

Summary: Pull Request resolved: #18712 ghimport-source-id: e435150 Stack from [ghstack](https://github.com/ezyang/ghstack): * **#18712 [jit][easy] remove unused func** * #18711 [jit] fix side-effects and aliasing for custom ops as title Differential Revision: D14730979 fbshipit-source-id: 381d16ea2a45779bf6d5fc6d90a4f8585461e902

Summary: Pull Request resolved: pytorch#18711 ghimport-source-id: c9caedc Stack from [ghstack](https://github.com/ezyang/ghstack): * **pytorch#18711 [jit] fix side-effects and aliasing for custom ops** Previously we didn't track aliasing, mutation, or side effects for custom ops. This PR adds in guards with the most conservative assumptions possible: the op will 1) have side effects, 2) write to everything 3) produce a wildcard. In order to tell whether a given operator is a custom op, this PR introduces the concept of a "reserved" namespace (basically all our builtin namespaces). Custom ops live in non-reserved namespaces, so a check on the namespace is sufficient to tell whether a schema/node is "custom" or not. This is just to get things correct for now. Follow-ups to this: - Users should be able to specify aliasing/mutability without having to learn the whole alias annotation schema. - Relax assumptions a bit. In particular outputs can only alias input tensors, they don't have to be wildcards. Fixes pytorch#18490 Differential Revision: D14730978 fbshipit-source-id: 540b47a24ccf24145051609bdcc99c97e46e0fe0

Summary: Pull Request resolved: pytorch#18712 ghimport-source-id: e435150 Stack from [ghstack](https://github.com/ezyang/ghstack): * **pytorch#18712 [jit][easy] remove unused func** * pytorch#18711 [jit] fix side-effects and aliasing for custom ops as title Differential Revision: D14730979 fbshipit-source-id: 381d16ea2a45779bf6d5fc6d90a4f8585461e902

suo mentioned this pull request Apr 2, 2019

[jit][easy] remove unused func #18712

Closed

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Apr 2, 2019

suo requested review from eellison and zdevito April 2, 2019 00:40

suo added 4 commits April 1, 2019 17:43

Update on "[jit] fix side-effects and aliasing for custom ops"

989f2db

Update on "[jit] fix side-effects and aliasing for custom ops"

d9c0d0b

Update on "[jit] fix side-effects and aliasing for custom ops"

8b1a746

Update on "[jit] fix side-effects and aliasing for custom ops"

99c1353

eellison reviewed Apr 2, 2019

View reviewed changes

Update on "[jit] fix side-effects and aliasing for custom ops"

bff581d

eellison approved these changes Apr 2, 2019

View reviewed changes

Update on "[jit] fix side-effects and aliasing for custom ops"

c463ee6

eellison previously requested changes Apr 2, 2019

View reviewed changes

suo commented Apr 2, 2019

View reviewed changes

Update on "[jit] fix side-effects and aliasing for custom ops"

0797b97

zdevito requested changes Apr 3, 2019

View reviewed changes

Update on "[jit] fix side-effects and aliasing for custom ops"

70aa282

suo requested a review from zdevito April 4, 2019 22:17

zdevito approved these changes Apr 4, 2019

View reviewed changes

facebook-github-bot closed this in fefa6d3 Apr 5, 2019

facebook-github-bot added the merged label Apr 5, 2019

suo deleted the gh/suo/15/head branch April 6, 2019 07:50

[jit] fix side-effects and aliasing for custom ops #18711

[jit] fix side-effects and aliasing for custom ops #18711

Uh oh!

Conversation

suo commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suo commented Apr 2, 2019

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

eellison Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apaszke commented Apr 2, 2019

Uh oh!

suo commented Apr 2, 2019

Uh oh!

apaszke commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eellison commented Apr 2, 2019

Uh oh!

suo commented Apr 2, 2019

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

suo Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

eellison commented Apr 2, 2019

Uh oh!

suo commented Apr 2, 2019

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

zdevito Apr 3, 2019

Choose a reason for hiding this comment

Uh oh!

zdevito Apr 3, 2019

Choose a reason for hiding this comment

Uh oh!

zdevito Apr 3, 2019

Choose a reason for hiding this comment

Uh oh!

zdevito Apr 3, 2019

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

suo commented Apr 2, 2019 •

edited

Loading

apaszke commented Apr 2, 2019 •

edited

Loading