Function Calls for MethodValues (no Interpreter support) #18367

Krovatkin · 2019-03-22T22:51:39Z

This PR adds infrastructure for adding function calls for MethodValues. Interpreter doesn't know how to handle function calls yet.

Krovatkin · 2019-03-26T20:33:30Z

c10/util/StringUtil.h

There seems to be many different implementations of Split sprinkled all over the place. I'm not sure if we might want to consider unifying/merging them. Neither am I sure if this work needs to be a part of this PR.

This seems fine.

Krovatkin · 2019-03-26T20:35:10Z

torch/csrc/jit/script/compiler.cpp

I did consider iterators, but since we need to keep track of several iteration points, it looked rather clunky.

Krovatkin · 2019-03-26T20:40:59Z

torch/csrc/jit/script/compiler.cpp

This PR doesn't support expressions like modules[i].method1() or anything else except for selectors for now 😢

Krovatkin · 2019-03-26T20:43:09Z

torch/csrc/jit/script/compiler.cpp

DCE won't touch this, since it's marked as having side effects, so we have to remove it manually.

zdevito

Hi, I added some high-level comments. Let's chat more about how we will name functions, etc.

zdevito · 2019-04-03T18:51:50Z

aten/src/ATen/core/ivalue.h

Function rather than ModuleFunction might be more clear

zdevito · 2019-04-03T18:54:36Z

aten/src/ATen/core/jit_type.h

I'd expect FunctionType to have argument types and a return type, and maybe just have FunctionSchema (though with blank alias information). Is there a reason to go with just a blank Function object?

given that we are inlining right away we could s tart with a blank FunctionType and add more members lazily.

The simplest thing to do right now might be to make the FunctionType object contain the std::shared_ptrscript::Function object. That way we can still use prim::Constant, but it doesn't need any attributes. That should be an easy tweak to what is here now.

zdevito · 2019-04-03T18:55:12Z

c10/util/StringUtil.h

This seems fine.

zdevito · 2019-04-03T18:57:09Z

torch/csrc/jit/ir.h

Please do not expose this! I don't want it to be easy to do this because then we will end up with a ton of places where coders are lazy and didn't specify where something came from.

zdevito · 2019-04-03T19:00:28Z

torch/csrc/jit/script/compiler.cpp

I do not think this is the right approach. It replicates a bunch of the SugaredValue behavior locally just to try to get the fully-qualified name for a function relative to the module root. MethodValue should just track its fully qualified name itself. I would not expect to see many changes in compiler.cpp

zdevito · 2019-04-03T19:01:14Z

torch/csrc/jit/script/compiler.cpp

Separate file please. compiler.cpp is long enough. This will need to be detangled from things like the environment. Otherwise the PR is not really accomplishing its purpose.

zdevito

Nice progress. This is getting close, but there are a few restructuring things I suggest:

FunctionType can simply contain the script::Function* object
We do not need the ivalue object in this PR, we can just directly create prim::Constant with a FunctionType return type.
Logic for emitting the CallFunction should move into Graph::createCall, and the actual use of that function should be done in MethodValue::call. There is no need to put a special case in the compiler (and in fact, one reason for wanting first-class calls is to remove special casing in the compiler).

zdevito · 2019-04-17T16:08:38Z

aten/src/ATen/core/ivalue.cpp

I think it is premature to try to add an ivalue::Function object. It is not necessary for the core of this PR since it is only being temporarily used to create a function node from a constant.

zdevito · 2019-04-18T18:03:10Z

aten/src/ATen/core/jit_type.h

The simplest thing to do right now might be to make the FunctionType object contain the std::shared_ptrscript::Function object. That way we can still use prim::Constant, but it doesn't need any attributes. That should be an easy tweak to what is here now.

zdevito · 2019-04-18T18:05:14Z

torch/csrc/jit/passes/inliner.h

Can you comment in the code why the recurse flag exists? (not what it is but why it is)

zdevito · 2019-04-18T18:06:08Z

torch/csrc/jit/passes/lower_tuples.cpp

I am not sure what this is. Was this a bad merge? Doesn't seem related to this PR.

zdevito · 2019-04-18T18:07:00Z

torch/csrc/jit/script/compiler.cpp

It is important to catch the specific exception. Otherwise this will hide all sorts of bad errors and report them as method recursion.

zdevito · 2019-04-18T18:11:44Z

torch/csrc/jit/script/compiler.cpp

Following previous convention, I think it would be better if we added an insertCallFunction(Value * callee, std::vector<Value*> inputs) call to Graph. Like others, it should never do user-facing error reporting, but it should handle the details of what a CallFunction node looks like.

zdevito · 2019-04-18T18:14:04Z

torch/csrc/jit/script/sugared_value.cpp

Ok, I see the move of this code into the lower tuples pass. However, I don't think it does the same thing, because this only lowers the tuple if it is the only argument to prim::Print, while the other one appears to do it universally.

zdevito · 2019-04-18T18:16:18Z

torch/csrc/jit/script/compiler.cpp

This code should be in MethodValue::call and not inline in the compiler here.

zdevito · 2019-04-18T18:17:00Z

torch/csrc/jit/script/sugared_value.h

These shouldn't be necessary, MethodValue should handle its own call logic.

Also the current MethodValue::call is dead.

simple cases for function calls work unit tests passing all unit test passing remove schema remove dce cleanup remove std::cout and redundant headers and defs fix clang tidy smaller fixes address Zach's feedback ModuleFunction -> Function ripping out relative qualified names clean up align formatting fix tests after rebase with prints without prints remove debug prints

zdevito

Inline comments on a few places where this did not merge nicely with the current codebase.

zdevito · 2019-05-15T17:33:00Z

torch/csrc/jit/passes/dead_code_elimination.cpp

      if (hasSideEffects(node)) {
        mark(node);
      }
+


Passes don't typically descend into subgraphs. Is this still needed now that the Function* is actually stored in the type?

actually, I think I might be able to remove this particular one, but then Inliner will need to descend into subgraphs and clean up unused Function literals

zdevito · 2019-05-15T17:36:05Z

torch/csrc/jit/passes/inliner.cpp

+    }
+  }
+
+  void unifyIfs(Block* b)


Why is this here? It is breaking loose coupling for inlining to need to think about how it would affect other statements.

type unification is typically done by compiler.cpp which now doesn't have the full inlined graph (hence, no precise types) since we inline later and even later in emitModuleHook we compile the inlined graph which now will have more precise types, so we end up with different graphs again.

zdevito · 2019-05-15T17:38:46Z

torch/csrc/jit/passes/inliner.cpp

+        next = cur->next();
+        cur->destroy();
+      } else {
+        if (cur->hasAttribute(attr::Subgraph)) {


Optimization of subgraphs should be handled by the thing inserting the subgraph. Most of our passes do not descend into subgraphs, which is why this code needs to keep modifying passes. I don't want to start mixing typical behavior and have some passes look at subgraphs but others that do not.

I think that inliner is a very special case. It is more similar to compiler.cpp rather than a regular optimization. Let me check if making compiler.cpp calling Inline is good enough. I think it was still breaking some tests if Inliner didn't expect subgraphs.

zdevito · 2019-05-15T17:42:39Z

torch/csrc/jit/script/compiler.cpp

      body_block->registerOutput(fn_simple_output);
      node_output = fork_node->output()->setType(
          FutureType::create(fn_simple_output->type()));
+      Inline(body_block);


Why? runCleanupPasses will run on the subgraph below and do the inlining.

lambdaLiftFork does sort of DCE i.e. it only lifts values that are used in a subgraph. If we leave a function call in place we end up passing in more values than really necessary and when we recompile the same function in emitModuleHook which will contain an already inlined subgraph we end up with different graphs. It's actually a very common theme with all these test failures. We can't do inlining early enough to not hinder some of optimizations so we have to re-do them.

i.e. running it on a lifted subgraph is too late.

zdevito · 2019-05-15T17:43:42Z

torch/csrc/jit/script/sugared_value.cpp

+  auto& schema = callee.getSchema();
+
+  std::vector<NamedValue> inputsWithSelf(inputs.begin(), inputs.end());
+  auto self_val = self_.value(*caller.graph());


Self values are always present. This was refactored a few weeks ago.

Also, this does not replace FunctionValue::call which the root place where function calls get inserted. MethodValue should be unmodified in this patch. This code should be in FunctionValue::call

This also seems like the wrong place to put this code. We have a method on Function with the name emit_call_to. I feel like this implementation should be there. This also explains why this code seems so long: it is copying all of the logic that already exists in that function to a new place without deleting it.

okay... let me see if I can move stuff around

zdevito · 2019-05-15T17:45:41Z

torch/csrc/jit/script/sugared_value.cpp

+        << "multiple returns aren't yet supported";
+  }
+
+  auto fun_constant = graph.create(prim::Constant);


Code to correctly construct CallFunction should be in ir.h:

::createFunctionCall(Value* fn, at::ArrayRef inputs);

Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates.

First class functions in IR, inlined eagerly Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates. gh-metadata: pytorch pytorch 21052 gh/zdevito/40/head

Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates. gh-metadata: pytorch pytorch 21052 gh/zdevito/40/head

First class functions in IR, inlined eagerly Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates. gh-metadata: pytorch pytorch 21052 gh/zdevito/40/head

Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates. gh-metadata: pytorch pytorch 21052 gh/zdevito/40/head

First class functions in IR, inlined eagerly Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates. gh-metadata: pytorch pytorch 21052 gh/zdevito/40/head

Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates. gh-metadata: pytorch pytorch 21052 gh/zdevito/40/head

Krovatkin · 2019-05-29T20:27:52Z

Closing in favour of #21052

First class functions in IR, inlined eagerly Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates. gh-metadata: pytorch pytorch 21052 gh/zdevito/40/head stuff

First class functions in IR, inlined eagerly Summary: This is a continuation of #18367 that rebases it and cleans up some of the issues that were popping up. Notes: 1. removed special handling for Python 2 print statements. Its implementation was buggy (treated inlined tuples as literal tuples), and leaving it out just means that in Python 2 printing multiple values will print different in script and python. This is ok, because it is minor formatting differences and only in Python 2. 2. --accepted internal loss of ONNX type information. We are only required to have correct input and output types for ONNX not for intermediates. gh-metadata: pytorch pytorch 21052 gh/zdevito/40/head stuff no

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

Krovatkin force-pushed the krovatkin/calls branch from 746c517 to 56c51ad Compare March 26, 2019 06:25

Krovatkin requested a review from zdevito March 26, 2019 19:53

Krovatkin commented Mar 26, 2019

View reviewed changes

Krovatkin requested a review from suo April 1, 2019 22:32

zdevito reviewed Apr 3, 2019

View reviewed changes

Krovatkin changed the title ~~[WIP] [DO NOT MERGE] Function Calls for MethodValues (no Interpreter support)~~ [WIP] [Exploratory] Function Calls for MethodValues (no Interpreter support) Apr 3, 2019

Krovatkin requested a review from zdevito April 15, 2019 17:06

Krovatkin force-pushed the krovatkin/calls branch 2 times, most recently from 73507a6 to c340af9 Compare April 18, 2019 16:36

Krovatkin changed the title ~~[WIP] [Exploratory] Function Calls for MethodValues (no Interpreter support)~~ Function Calls for MethodValues (no Interpreter support) Apr 18, 2019

zdevito reviewed Apr 18, 2019

View reviewed changes

Krovatkin added 2 commits May 14, 2019 11:48

fix comp failures

df71843

Krovatkin force-pushed the krovatkin/calls branch from b9eb6fc to df71843 Compare May 14, 2019 19:47

pytorchbot added caffe2 module: build Build system issues module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries labels May 14, 2019

Krovatkin added 4 commits May 14, 2019 14:12

cleaning up part1

986f3df

add an assert

d1169fd

formatting fixes

483801e

extra ws

85a6eb6

Krovatkin requested a review from zdevito May 14, 2019 23:00

zdevito reviewed May 15, 2019

View reviewed changes

zdevito mentioned this pull request May 29, 2019

First class functions in IR, inlined eagerly #21052

Closed

Krovatkin closed this May 29, 2019

Function Calls for MethodValues (no Interpreter support) #18367

Function Calls for MethodValues (no Interpreter support) #18367

Uh oh!

Conversation

Krovatkin commented Mar 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment