Prepare to stop using attributes in the JIT #9505

apaszke · 2018-07-17T18:33:50Z

This PR adds machinery to cache the schema in an IR node, and allows lookups of (possibly) constant inputs by their names (instead of position). The new methods are:

at::optional<T> get<T>(Symbol name) - if the argument called name is a constant, then casts it to type T and returns it. If it's not constant returns nullopt. Raises an error if there's no argument with that name.
at::optional<IValue> get<T>(Symbol name) - like above, but packs the result in an IValue
Value* getValue(Symbol name) - retrieves a Value* for an argument (no need to know its position).

All above functions currently inspect the attributes as well, but that's only so that I could start using them in other places in the JIT without disrupting our current functionality. I wanted this diff to be a preparation that doesn't change the semantics too much, and so both the tracer and script create nodes with attributes. The next PR will put that to a stop, and hopefully the changes we need to make to other components will be simpler thanks to what I did here.

One more thing I'd like to do before actually stopping creating the non-attributed nodes is to have a convenient way of creating a schema programmatically, matching nodes against it, and creating them without having to pack inputs into flat argument lists (which is quite error prone).

@zdevito

zdevito

This this good. I noted one place where I think we are missing invalidations for schema_, which should be fixed.

The syntax n.get<std::vector<int64>>(attr::value).value() is pretty wordy compared to n.is(attr::value). Not sure I have a good suggestion though.

torch/csrc/jit/fusion_compiler.cpp

@@ -190,6 +191,15 @@ std::string valueName(Value * n) {
  return "n" + std::to_string(n->unique());
 }

+std::string valueNameOrConstant(Value * n) {


torch/csrc/jit/ir.h

@@ -415,6 +427,7 @@ struct Node : public Attributes<Node> {
  // Result:  %3 = f(%1, %2, %4)
  Value* addInput(Value * node) {
    JIT_ASSERT(graph_ == node->owningGraph());
+    schema_ = nullptr;


torch/csrc/jit/ir.cpp

@@ -566,6 +568,135 @@ Value* Value::setUniqueName(const std::string & name) {
  return this;
 }

+template<typename T>


torch/csrc/jit/passes/shape_analysis.cpp

@@ -150,6 +150,7 @@ void PropagateShapeOnNode(Node * node, bool insert_expands) {
  }
  std::vector<TensorType*> types;
  bool present;
+  // TODO (apaszke): This currently assumes that inputs are pretty much tensor-only. Need to fix that.


facebook-github-bot

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

facebook-github-bot

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

apaszke · 2018-07-20T00:59:55Z

@pytorchbot retest this please

facebook-github-bot

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

facebook-github-bot

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

bddppq · 2018-07-20T03:14:27Z

@pytorchbot retest this please

apaszke · 2018-07-20T12:35:40Z

@pytorchbot retest this please

Summary: This PR adds machinery to cache the schema in an IR node, and allows lookups of (possibly) constant inputs by their names (instead of position). The new methods are: - `at::optional<T> get<T>(Symbol name)` - if the argument called name is a constant, then casts it to type `T` and returns it. If it's not constant returns `nullopt`. Raises an error if there's no argument with that name. - `at::optional<IValue> get<T>(Symbol name)` - like above, but packs the result in an IValue - `Value* getValue(Symbol name)` - retrieves a `Value*` for an argument (no need to know its position). All above functions currently inspect the attributes as well, but that's only so that I could start using them in other places in the JIT without disrupting our current functionality. I wanted this diff to be a preparation that doesn't change the semantics too much, and so both the tracer and script create nodes with attributes. The next PR will put that to a stop, and hopefully the changes we need to make to other components will be simpler thanks to what I did here. One more thing I'd like to do before actually stopping creating the non-attributed nodes is to have a convenient way of creating a schema programmatically, matching nodes against it, and creating them without having to pack inputs into flat argument lists (which is quite error prone). zdevito Pull Request resolved: pytorch#9505 Reviewed By: ezyang Differential Revision: D8915496 Pulled By: apaszke fbshipit-source-id: 39d14fc9a9d73d8494f128367bf70357dbba83f5

apaszke requested review from colesbury, ezyang, gchanan, soumith and zdevito as code owners July 17, 2018 18:33

zdevito approved these changes Jul 18, 2018

View reviewed changes

apaszke force-pushed the jit_accessor branch from 98b5d6b to a0d098a Compare July 19, 2018 14:41

facebook-github-bot reviewed Jul 19, 2018

View reviewed changes

apaszke requested review from anderspapitto, bddppq, dzhulgakov, houseroad, jamesr66a, smessmer and Yangqing as code owners July 19, 2018 22:14

facebook-github-bot reviewed Jul 20, 2018

View reviewed changes

apaszke added 7 commits July 19, 2018 19:28

Allow lookups of constant positional arguments by name

2c44719

Make the JIT code more robust to attribute removal

7060f22

Code review, CI is overreacting

1019c8f

Add missing inline

913a3ad

Add an overload for int

9cbdfac

Make old clang happy

29f69b5

Fix ONNX tests

a10111f

apaszke force-pushed the jit_accessor branch from 6bd291f to a10111f Compare July 20, 2018 02:31

facebook-github-bot reviewed Jul 20, 2018

View reviewed changes

ezyang approved these changes Jul 20, 2018

View reviewed changes

facebook-github-bot closed this in 1d4d9fc Jul 20, 2018

apaszke deleted the jit_accessor branch July 20, 2018 17:57

ezyang added open source merged labels Jun 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prepare to stop using attributes in the JIT #9505

Prepare to stop using attributes in the JIT #9505

Uh oh!

apaszke commented Jul 17, 2018

Uh oh!

zdevito left a comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Uh oh!

facebook-github-bot left a comment

Uh oh!

apaszke commented Jul 20, 2018

Uh oh!

facebook-github-bot left a comment

Uh oh!

facebook-github-bot left a comment

Uh oh!

bddppq commented Jul 20, 2018

Uh oh!

apaszke commented Jul 20, 2018

Uh oh!

Uh oh!

Prepare to stop using attributes in the JIT #9505

Prepare to stop using attributes in the JIT #9505

Uh oh!

Conversation

apaszke commented Jul 17, 2018

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

apaszke commented Jul 20, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

bddppq commented Jul 20, 2018

Uh oh!

apaszke commented Jul 20, 2018

Uh oh!

Uh oh!