[coreml] delegate multiple outputs #88345

mcr229 · 2022-11-02T18:29:33Z

Summary:
https://www.internalfb.com/code/fbsource/[c0e4da0b5c7fff3b4e31e4611033c30cabdc6aef]/fbcode/caffe2/torch/csrc/jit/backends/backend_detail.cpp?lines=268-276

seems like the torchscript addition of
$unpack, = self.__backend.execute( ...

the comma after unpack forces the result of execute to have only one item. So for this fix now when the size of the outputs > 1, execute returns a List List of outputs (basically put the outputs in another list before putting it into the list we return)

[[output1, output2, output3, ...]]

instead of

[output1, output2, output3, ...]

Do we want to fix this in backend_detail? Or should we make the change in our delegate to accomadate the torchscript? Proposing this q here. Requesting cccclai, kimishpatel for approval here

Test Plan: unblocked models for chengxiangyin and models in pytorch playground all passing unit tests

Reviewed By: kimishpatel, cccclai

Differential Revision: D40328684

pytorch-bot · 2022-11-02T18:29:36Z

🔗 Helpful Links

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

📄 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

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Failures

As of commit eb75da0:

The following jobs have failed:

cuda11.6-py3.10-gcc7-sm86 / test (default, 2, 4, linux.g5.4xlarge.nvidia.gpu)

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot · 2022-11-02T18:30:15Z

This pull request was exported from Phabricator. Differential Revision: D40328684

Skylion007 · 2022-11-02T18:50:55Z

torch/csrc/jit/backends/coreml/objc/PTMCoreMLBackend.mm

@@ -90,6 +90,11 @@ GenericList pack_outputs(const std::vector<TensorSpec>& output_specs, id<MLFeatu
      count * sizeof(float));
    outputs.push_back(tensor);
  }
+  if(output_specs.size() > 1){
+    c10::List<c10::List<torch::Tensor>> output_res;
+    ouptut_res.push_back(outputs);


Suggested change

ouptut_res.push_back(outputs);

output_res.push_back(outputs);

facebook-github-bot · 2022-11-02T18:53:53Z

This pull request was exported from Phabricator. Differential Revision: D40328684

Summary: Pull Request resolved: pytorch#88345 https://www.internalfb.com/code/fbsource/[c0e4da0b5c7fff3b4e31e4611033c30cabdc6aef]/fbcode/caffe2/torch/csrc/jit/backends/backend_detail.cpp?lines=268-276 seems like the torchscript addition of `$unpack, = self.__backend.execute( ... ` the comma after unpack forces the result of execute to have only one item. So for this fix now when the size of the outputs > 1, execute returns a List List of outputs (basically put the outputs in another list before putting it into the list we return) ``` [[output1, output2, output3, ...]] ``` instead of ``` [output1, output2, output3, ...] ``` Do we want to fix this in backend_detail? Or should we make the change in our delegate to accomadate the torchscript? Proposing this q here. Requesting cccclai, kimishpatel for approval here Test Plan: unblocked models for chengxiangyin and models in pytorch playground all passing unit tests Reviewed By: kimishpatel, cccclai Differential Revision: D40328684 fbshipit-source-id: 19e4a87c5570df56a6db05942d3729cfcc86009b

Skylion007

One other nit

Skylion007

Also this one.

facebook-github-bot · 2022-11-02T19:03:29Z

This pull request was exported from Phabricator. Differential Revision: D40328684

Summary: Pull Request resolved: pytorch#88345 https://www.internalfb.com/code/fbsource/[c0e4da0b5c7fff3b4e31e4611033c30cabdc6aef]/fbcode/caffe2/torch/csrc/jit/backends/backend_detail.cpp?lines=268-276 seems like the torchscript addition of `$unpack, = self.__backend.execute( ... ` the comma after unpack forces the result of execute to have only one item. So for this fix now when the size of the outputs > 1, execute returns a List List of outputs (basically put the outputs in another list before putting it into the list we return) ``` [[output1, output2, output3, ...]] ``` instead of ``` [output1, output2, output3, ...] ``` Do we want to fix this in backend_detail? Or should we make the change in our delegate to accomadate the torchscript? Proposing this q here. Requesting cccclai, kimishpatel for approval here Test Plan: unblocked models for chengxiangyin and models in pytorch playground all passing unit tests Reviewed By: kimishpatel, cccclai Differential Revision: D40328684 fbshipit-source-id: 27154dbfde812388bfdb16ba77f9cc3a2d105b9a

Skylion007

Looks good, this piece of code could definitely be optimized a lot with more std::move and reserve() etc... for all the push_back, toList etc...

Skylion007 · 2022-11-02T19:06:36Z

torch/csrc/jit/backends/coreml/objc/PTMCoreMLBackend.mm

+    return c10::impl::toList(output_res);
+  }
  return c10::impl::toList(outputs);


Should also move here actually.

Suggested change

return c10::impl::toList(output_res);

}

return c10::impl::toList(outputs);

return c10::impl::toList(std::move(output_res));

}

return c10::impl::toList(std::move(outputs));

Skylion007 · 2022-11-02T19:08:00Z

torch/csrc/jit/backends/coreml/objc/PTMCoreMLBackend.mm

@@ -90,6 +90,11 @@ GenericList pack_outputs(const std::vector<TensorSpec>& output_specs, id<MLFeatu
      count * sizeof(float));
    outputs.push_back(tensor);


Suggested change

outputs.push_back(tensor);

outputs.push_back(std::move(tensor));

Also see the std::vector above and properly call .reserve() on it etc.

Summary: Pull Request resolved: pytorch#88345 https://www.internalfb.com/code/fbsource/[c0e4da0b5c7fff3b4e31e4611033c30cabdc6aef]/fbcode/caffe2/torch/csrc/jit/backends/backend_detail.cpp?lines=268-276 seems like the torchscript addition of `$unpack, = self.__backend.execute( ... ` the comma after unpack forces the result of execute to have only one item. So for this fix now when the size of the outputs > 1, execute returns a List List of outputs (basically put the outputs in another list before putting it into the list we return) ``` [[output1, output2, output3, ...]] ``` instead of ``` [output1, output2, output3, ...] ``` Do we want to fix this in backend_detail? Or should we make the change in our delegate to accomadate the torchscript? Proposing this q here. Requesting cccclai, kimishpatel for approval here Test Plan: unblocked models for chengxiangyin and models in pytorch playground all passing unit tests Reviewed By: kimishpatel, cccclai Differential Revision: D40328684 fbshipit-source-id: bfb85f3f0933b062a14c5d8cd3acb7db58c52944

facebook-github-bot · 2022-11-02T20:10:20Z

This pull request was exported from Phabricator. Differential Revision: D40328684

Skylion007

In the changed function, the std::vector output_shape should also allocate it's space using .reserve() before the for loop. Otherwise this code is looking much better and more performant.

facebook-github-bot · 2022-11-03T20:04:08Z

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

pytorchmergebot · 2022-11-03T20:05:48Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Summary: https://www.internalfb.com/code/fbsource/[c0e4da0b5c7fff3b4e31e4611033c30cabdc6aef]/fbcode/caffe2/torch/csrc/jit/backends/backend_detail.cpp?lines=268-276 seems like the torchscript addition of `$unpack, = self.__backend.execute( ... ` the comma after unpack forces the result of execute to have only one item. So for this fix now when the size of the outputs > 1, execute returns a List List of outputs (basically put the outputs in another list before putting it into the list we return) ``` [[output1, output2, output3, ...]] ``` instead of ``` [output1, output2, output3, ...] ``` Do we want to fix this in backend_detail? Or should we make the change in our delegate to accomadate the torchscript? Proposing this q here. Requesting cccclai, kimishpatel for approval here Test Plan: unblocked models for chengxiangyin and models in pytorch playground all passing unit tests Reviewed By: kimishpatel, cccclai Differential Revision: D40328684 Pull Request resolved: pytorch#88345 Approved by: https://github.com/jmdetloff, https://github.com/Skylion007

pytorch-bot bot added the release notes: jit release notes category label Nov 2, 2022

facebook-github-bot added the fb-exported label Nov 2, 2022

mcr229 requested review from jmdetloff, cccclai and kimishpatel November 2, 2022 18:30

Skylion007 reviewed Nov 2, 2022

View reviewed changes

mcr229 force-pushed the export-D40328684 branch from 65c10c9 to 6503551 Compare November 2, 2022 18:53

Skylion007 reviewed Nov 2, 2022

View reviewed changes

mcr229 force-pushed the export-D40328684 branch from 6503551 to 688a4c8 Compare November 2, 2022 19:03

Skylion007 reviewed Nov 2, 2022

View reviewed changes

jmdetloff approved these changes Nov 2, 2022

View reviewed changes

Skylion007 reviewed Nov 2, 2022

View reviewed changes

mcr229 force-pushed the export-D40328684 branch from 688a4c8 to eb75da0 Compare November 2, 2022 20:10

Skylion007 approved these changes Nov 2, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 3, 2022

pytorchmergebot added the Merged label Nov 3, 2022

pytorchmergebot closed this in 826b4a9 Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[coreml] delegate multiple outputs #88345

[coreml] delegate multiple outputs #88345

mcr229 commented Nov 2, 2022

pytorch-bot bot commented Nov 2, 2022 •

edited

facebook-github-bot commented Nov 2, 2022

Skylion007 Nov 2, 2022 •

edited

facebook-github-bot commented Nov 2, 2022

Skylion007 left a comment

Skylion007 left a comment

facebook-github-bot commented Nov 2, 2022

Skylion007 left a comment •

edited

Skylion007 Nov 2, 2022

Skylion007 Nov 2, 2022

Skylion007 Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

Skylion007 left a comment

facebook-github-bot commented Nov 3, 2022

pytorchmergebot commented Nov 3, 2022

	ouptut_res.push_back(outputs);
	output_res.push_back(outputs);

		@@ -90,6 +90,11 @@ GenericList pack_outputs(const std::vector<TensorSpec>& output_specs, id<MLFeatu
		count * sizeof(float));
		outputs.push_back(tensor);

	outputs.push_back(tensor);
	outputs.push_back(std::move(tensor));

[coreml] delegate multiple outputs #88345

[coreml] delegate multiple outputs #88345

Conversation

mcr229 commented Nov 2, 2022

pytorch-bot bot commented Nov 2, 2022 • edited

🔗 Helpful Links

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

❌ 1 Failures

facebook-github-bot commented Nov 2, 2022

Skylion007 Nov 2, 2022 • edited

Choose a reason for hiding this comment

facebook-github-bot commented Nov 2, 2022

Skylion007 left a comment

Choose a reason for hiding this comment

Skylion007 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 2, 2022

Skylion007 left a comment • edited

Choose a reason for hiding this comment

Skylion007 Nov 2, 2022

Choose a reason for hiding this comment

Skylion007 Nov 2, 2022

Choose a reason for hiding this comment

Skylion007 Nov 2, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Nov 2, 2022

Skylion007 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 3, 2022

pytorchmergebot commented Nov 3, 2022

Merge started

pytorch-bot bot commented Nov 2, 2022 •

edited

Skylion007 Nov 2, 2022 •

edited

Skylion007 left a comment •

edited