[PyTorch Edge] Support default args with out arg, flag off #63540

cccclai · 2021-08-19T00:34:13Z

Stack from ghstack:

-> [PyTorch Edge] Support default args with out arg, flag off #63540

Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag.
Add two unittests to cover this type of operators.

Differential Revision: D30414156

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) [ghstack-poisoned]

facebook-github-bot · 2021-08-19T00:34:16Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/63540

💊 CI failures summary and remediations

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

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

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.

Click here to manually regenerate this comment.

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) ghstack-source-id: 136170423 Pull Request resolved: #63540

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) [ghstack-poisoned]

Pull Request resolved: #63540 ghstack-source-id: 136217678 Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/)

iseeyuan · 2021-08-19T16:55:06Z

torch/csrc/jit/mobile/function.cpp

-        for (size_t i = num_specified_args.value(); i < args.size(); ++i) {
-          stack.push_back(args[i].default_value());
+        c10::optional<IValue> out_arg;
+        if (!args.empty() && args.back().is_out()) {


Is there always zero or one out arg? What happens if there are 2 or more out args?

Oh I didn't know it's possible. My understanding is that out arg is an optional argument to store the output to. The output from inference can only be one. What about having a TORCH_CHECK to check only 1 out arg and we can extend it when we know there might be more than one out arg?

I think there can be more than 1 out arg. We definitely are coding the out arg as a counter not a bool (plus it makes sense it can be > 1).

Update to cover multiple args.

raziel

Thanks. A few comments, but in particular about why the support in the interpreter is under a flag?

test/cpp/jit/test_lite_interpreter.cpp

raziel · 2021-08-19T18:44:46Z

torch/csrc/jit/mobile/function.cpp

-        for (size_t i = num_specified_args.value(); i < args.size(); ++i) {
-          stack.push_back(args[i].default_value());
+        c10::optional<IValue> out_arg;
+        if (!args.empty() && args.back().is_out()) {


I think there can be more than 1 out arg. We definitely are coding the out arg as a counter not a bool (plus it makes sense it can be > 1).

torch/csrc/jit/runtime/interpreter.cpp

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag. Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

1. Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag. 2. Add two unittests to cover this type of operators. Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

tugsbayasgalan · 2021-08-21T20:03:06Z

torch/csrc/jit/mobile/function.cpp

+          out_args.push_back(stack.back());
+          stack.pop_back();
+        }
+        size_t start_index = num_specified_args.value() - out_args.size();


@iseeyuan @raziel @cccclai @gmagogsfm I had to do this calculation many places, so I modified CalculateNecessaryArgs utility method to return two things (pair<int, int>) where the first one is number of default args and the second one is number of out args. Do you think I should communicate this number of args to mobile by either of following ways:

change num_specified_args: int to num_specified_args: pair<int, int>

add another map that says op_to_num_out_args.

@tugsbayasgalan I think one number, num_specified_args would be sufficient, because you've already provided is_out. All the out args can be popped and default values can be pushed. Any reason num_out_args is needed in mobile?

yeahh I guess you still have to loop from the end to pop out args. Yeah, I confirm mobile doesn't need num_out_args.

@tugsbayasgalan Thanks for the confirmation! At least we don't need to change the bytecode schema :D

torch/csrc/jit/runtime/interpreter/code_impl.h

iseeyuan

Left a few comments. @cccclai For checking outputs in tests and TORCH_CHECK default_value, please let me know if they make sense.

@tugsbayasgalan Please verify that we don't need explicit num_out_args.

test/cpp/jit/test_lite_interpreter.cpp

iseeyuan · 2021-08-22T00:58:01Z

torch/csrc/jit/mobile/function.cpp

+          out_args.push_back(stack.back());
+          stack.pop_back();
+        }
+        size_t start_index = num_specified_args.value() - out_args.size();


@tugsbayasgalan I think one number, num_specified_args would be sufficient, because you've already provided is_out. All the out args can be popped and default values can be pushed. Any reason num_out_args is needed in mobile?

iseeyuan · 2021-08-22T01:01:19Z

torch/csrc/jit/mobile/function.cpp

+        size_t start_index = num_specified_args.value() - out_args.size();
+        for (size_t i = (start_index >= 0 ? start_index : 0); i < args.size();
+             ++i) {
+          if (args[i].default_value().has_value()) {


It should be a TORCH_CHECK. args[i] must have default value, otherwise the element# in the stack can mismatch.

Resolve in the latest commit.

1. Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag. 2. Add two unittests to cover this type of operators. Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

tugsbayasgalan · 2021-08-27T19:36:19Z

torch/csrc/jit/runtime/interpreter/code_impl.h

        insertInstruction(OP, operator_table_.size());
      }
      operator_table_.emplace_back(op.getOperation(node));
    }
  }

  bool emit_default_input_instructions_;
+  bool support_default_args_before_out_;


should we clean up these flags later on? @cccclai @iseeyuan @raziel @gmagogsfm I feel like this will just keep adding more and more edge cases (2^x) to cover if we keep having flags like these.

Yes, after a couple more releases, let's clean them up.

Good point. I add some comments about these two flags in the latest commit, so it's easier to clean up.

We could clean them up when we don't need the backport functions.

1. Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag. 2. Add two unittests to cover this type of operators. Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

1. Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag. 2. Add two unittests to cover this type of operators. Pull Request resolved: #63540 ghstack-source-id: 136887019 Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/)

iseeyuan

LGTM. Thanks!

iseeyuan · 2021-08-29T01:00:13Z

test/cpp/jit/test_lite_interpreter.cpp

+
+  const int N = 28;
+  auto input = torch::range(1, N * N, 1);
+  input[0] = 1; // a more stable matrix


import torch N = 3 input = torch.range(1, N * N, 1) print(input)

tensor([1., 2., 3., 4., 5., 6., 7., 8., 9.])

input[0] = 1 does not change input and the condition number of this matrix is Inf (very unstable) and the solution could vary in different platforms.

##############################
edit:
Chen: Thanks. Address in the latest commit.

iseeyuan · 2021-08-29T01:05:33Z

torch/csrc/jit/mobile/function.cpp

+        for (size_t i = (start_index >= 0 ? start_index : 0); i < args.size();
+             ++i) {
+          auto default_val = args[i].default_value();
+          TORCH_CHECK(


@raziel Got it. Thanks!

iseeyuan · 2021-08-29T01:08:54Z

torch/csrc/jit/runtime/interpreter/code_impl.h

        insertInstruction(OP, operator_table_.size());
      }
      operator_table_.emplace_back(op.getOperation(node));
    }
  }

  bool emit_default_input_instructions_;
+  bool support_default_args_before_out_;


We could clean them up when we don't need the backport functions.

1. Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag. 2. Add two unittests to cover this type of operators. Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

raziel

Thanks!

1. Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag. 2. Add two unittests to cover this type of operators. Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

1. Allow consuming operators with defaults arguments and out arguments. Flag is off to keep the same behavior as v6, in pr 63651, turn on the flag. 2. Add two unittests to cover this type of operators. Pull Request resolved: #63540 ghstack-source-id: 137211562 Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/)

facebook-github-bot · 2021-09-02T08:36:43Z

This pull request has been merged in 8d5b950.

[PyTorch Edge] Support default args with out arg

51425b6

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) [ghstack-poisoned]

facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Aug 19, 2021

cccclai pushed a commit that referenced this pull request Aug 19, 2021

[PyTorch Edge] Support default args with out arg

a999af7

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) ghstack-source-id: 136170423 Pull Request resolved: #63540

Update on "[PyTorch Edge] Support default args with out arg"

8d2e5c5

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) [ghstack-poisoned]

This was referenced Aug 19, 2021

Detect out argument in the operator schema #62755

Closed

Enhancement for smart serialization for operator schemas with out arg #63096

Closed

cccclai pushed a commit that referenced this pull request Aug 19, 2021

[PyTorch Edge] Support default args with out arg

0a1d042

Pull Request resolved: #63540 ghstack-source-id: 136217678 Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/)

iseeyuan reviewed Aug 19, 2021

View reviewed changes

raziel reviewed Aug 19, 2021

View reviewed changes

cccclai mentioned this pull request Aug 20, 2021

[PyTorch Edge] Backport function for defaults args with out args, flag on #63651

Closed

Update on "[PyTorch Edge] Support default args with out arg, flag off"

2419cca

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

cccclai changed the title ~~[PyTorch Edge] Support default args with out arg~~ [PyTorch Edge] Support default args with out arg, flag off Aug 21, 2021

chenlai added 2 commits August 20, 2021 17:34

Update on "[PyTorch Edge] Support default args with out arg, flag off"

3094d68

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

Update on "[PyTorch Edge] Support default args with out arg, flag off"

4210085

Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156/) Differential Revision: [D30414156](https://our.internmc.facebook.com/intern/diff/D30414156) [ghstack-poisoned]

cccclai requested review from gmagogsfm and tugsbayasgalan August 21, 2021 00:54

tugsbayasgalan reviewed Aug 21, 2021

View reviewed changes

iseeyuan requested changes Aug 22, 2021

View reviewed changes

chenlai and others added 2 commits August 26, 2021 10:10

tugsbayasgalan mentioned this pull request Aug 26, 2021

[WIP] add forward compatibility test #64053

Closed

tugsbayasgalan approved these changes Aug 27, 2021

View reviewed changes

cccclai added this to the 1.10.0 milestone Aug 28, 2021

iseeyuan approved these changes Aug 29, 2021

View reviewed changes

gmagogsfm approved these changes Aug 30, 2021

View reviewed changes

raziel approved these changes Aug 30, 2021

View reviewed changes

chenlai added 11 commits August 30, 2021 20:57

facebook-github-bot closed this in 8d5b950 Sep 2, 2021

facebook-github-bot added the Merged label Sep 2, 2021

facebook-github-bot deleted the gh/cccclai/103/head branch September 5, 2021 14:18

[PyTorch Edge] Support default args with out arg, flag off #63540

[PyTorch Edge] Support default args with out arg, flag off #63540

Uh oh!

Conversation

cccclai commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

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!

raziel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

iseeyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

iseeyuan left a comment

Choose a reason for hiding this comment

Uh oh!

iseeyuan Aug 29, 2021 • edited by cccclai Loading Uh oh! There was an error while loading. Please reload this page.

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!

raziel left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 2, 2021

Uh oh!

Uh oh!

cccclai commented Aug 19, 2021 •

edited

Loading

facebook-github-bot commented Aug 19, 2021 •

edited

Loading

iseeyuan Aug 29, 2021 •

edited by cccclai

Loading