[Partitioner] Heterogeneous partition testing #3197

beicy · 2019-06-28T23:15:42Z

Summary:
Provide an easier way for using heterogeneous partition : user can define which ops not running on which type of backends.
related to #2814
Documentation:

[Optional Fixes #issue]

Test Plan:
added unittest;

wangm-mbp:buildR wangm$ ./bin/image-classifier tests/images/imagenet/cat_285.png -image-mode=0to1 -m=resnet50 -model-input-name=gpu_0/data -cpu-memory=100000 -load-device-configs="tests/runtime_test/heterogeneousConfigs.yaml" 
Model: resnet50
Running 1 thread(s).
tests/runtime_test/heterogeneousConfigs.yaml
WARNING: Logging before InitGoogleLogging() is written to STDERR
I0628 16:12:13.164857 43896832 Partitioner.cpp:1038] The number of partitions is : 3, and the DAG is dumped into DAG.dot file.
I0628 16:12:13.165649 43896832 Partitioner.cpp:58] Writing dotty graph for DAG after graph partitioning: DAG.dot
I0628 16:12:13.165989 43896832 Partitioner.cpp:1047] 	 Partition 0:
		 Name :	resnet50_part1_part1
		 BackendKind :	CPU
		 Memory :	94929408
		 LogicalDeviceIDs :	1
I0628 16:12:13.166002 43896832 Partitioner.cpp:1047] 	 Partition 1:
		 Name :	resnet50_part2_part1
		 BackendKind :	Interpreter
		 Memory :	409600
		 LogicalDeviceIDs :	2
I0628 16:12:13.166026 43896832 Partitioner.cpp:1047] 	 Partition 2:
		 Name :	resnet50_part3_part1
		 BackendKind :	CPU
		 Memory :	8208200
		 LogicalDeviceIDs :	0
 File: tests/images/imagenet/cat_285.png	Label-K1: 285 (probability: 0.5823)

Please see a detailed explanation of how to fill out the fields in the relevant sections in PULL_REQUEST.md.

opti-mix · 2019-06-28T23:23:26Z

@beicy Nice! Could you also add an option to specify the opposite?

The following should mean that only AvgPool is supported on the specific backend and everything else should be considered not supported by the backend.

parameters: |
    "supportedOp":"AvgPool"

I hope adding this options should be trivial.

opti-mix · 2019-06-28T23:24:59Z

include/glow/Backends/DeviceManager.h

+  /// supported by this DeviceManger. It doesn't mean that those nodes kind are
+  /// not supported by this device. It just means user don't want these kind of
+  /// nodes running on this device.
+  llvm::StringRef getNonSupprtedNodes() const {


It is probably better to implement getParamByName(llvm::StringRef name) to make it more reusable. And then you can just call it like getParamByName("nonSupportedOp")

opti-mix · 2019-06-28T23:30:18Z

lib/Partitioner/PartitionerUtils.cpp

+std::set<Kinded::Kind> generateNonSupportedNodes(llvm::StringRef names) {
+  std::set<Kinded::Kind> ret;
+  while (names.find(',') != std::string::npos) {
+    std::pair<llvm::StringRef, llvm::StringRef> newSplit = names.split(',');


Probably not very important, but could it be that split allocates new strings? Please have a look at the similar function for splitting comma-separated input here:

https://llvm.org/doxygen/CommandLine_8cpp_source.html#l00607

/me wishes for folly::split (but of course we don't want to pull in folly, alas)

opti-mix

Thanks for adding supportedNodes!

Please address a couple of minor comments below and the PR should be good to merge!

opti-mix · 2019-07-01T23:57:11Z

lib/Partitioner/Partitioner.cpp

+      // Step 3: Check if the node is actually supported in this backend, if so,
+      // assign it to this backend and now we find the first backend that
+      // supports this node (The order of backends is important) and break.
+      // Otherwise goto Step 4.


This Step 3 sounds odd. I mean this part:
if so, assign it to this backend and now we find the first backend that supports this node. Could you rephrase?

opti-mix · 2019-07-01T23:59:46Z

lib/Partitioner/Partitioner.cpp

+      // Step 1:
+      auto nonSupported =
+          backendMap_[backend->getBackendName()].nonSupportedNodesKind;
+      if (nonSupported.find(N.getKind()) != nonSupported.end()) {


Small nit: The following could be a bit shorter:

if (nonSupported.count(N.getKind()) { continue; }

opti-mix · 2019-07-02T00:01:06Z

include/glow/Partitioner/Partitioner.h

@@ -38,6 +38,10 @@ struct BackendInfo {
  uint64_t memSize;
  /// Backend pointer.
  Backend *backend = nullptr;
+  /// The non-supported nodes kind.
+  std::set<Kinded::Kind> nonSupportedNodesKind;


You store multiple kinds in the set, right? So, should it be called nonSupportedNodesKinds? Same for supportedNodesKinds?

opti-mix · 2019-07-02T00:01:44Z

lib/Partitioner/Partitioner.cpp

+      // assigned to the next backend.
+
+      // Step 1:
+      auto nonSupported =


auto nonSupportedNodesKinds?

opti-mix · 2019-07-02T00:02:01Z

lib/Partitioner/Partitioner.cpp

+        continue;
+      }
+      // Step 2:
+      auto supported =


supportedNodesKinds?

opti-mix · 2019-07-02T00:03:27Z

lib/Partitioner/Partitioner.cpp

+      // Step 2:
+      auto supported =
+          backendMap_[backend->getBackendName()].supportedNodesKind;
+      if (supported.size() > 0 &&


How about a simpler form?

if (!supprted.empty() && supported.count(N.getKind())) { // Continue. }

opti-mix · 2019-07-02T00:04:25Z

lib/Partitioner/PartitionerUtils.cpp

@@ -286,4 +286,16 @@ GraphMemInfo getGraphMemInfo(const NodesSetTy &nodes) {
  return ret;
 }

+std::set<Kinded::Kind> generateNodeKindsSet(llvm::StringRef names) {
+  std::set<Kinded::Kind> ret;


ret -> nodeKindsSet?

opti-mix · 2019-07-02T00:05:07Z

tests/runtime_test/heterogeneousConfigs.yaml

+- name:     Device1
+  backendName: CPU
+  parameters: |
+    "nonSupportedNodes":"AvgPool"


Can you add supportedNodes example as well, because it needs to be tested?

The "Device3" in this file has the "supportedNodes" params. Since there are many ops in resnet50, I just let Interpreter supports all. In unit test, we tested the supported nodes set.

bertmaher · 2019-07-02T05:19:28Z

include/glow/Partitioner/Partitioner.h

@@ -38,6 +38,10 @@ struct BackendInfo {
  uint64_t memSize;
  /// Backend pointer.
  Backend *backend = nullptr;
+  /// The non-supported nodes kind.
+  std::set<Kinded::Kind> nonSupportedNodesKind;


Unless order matters here, prefer llvm::DenseSet or maybe std::unordered_set.

bertmaher · 2019-07-02T05:21:19Z

include/glow/Runtime/RuntimeTypes.h

@@ -51,6 +51,14 @@ struct DeviceInfo {
  uint64_t availableMemory;
  /// Backend Type.
  std::string backendName;
+  /// A string contains the node names(e.g. Add, Div) which are seperated by


sp: separated

bertmaher · 2019-07-02T05:33:34Z

lib/Partitioner/Partitioner.cpp

-      // is important.
+      // is important. The check flow is :
+
+      // Step 1: If a node is in pre-defined non-supported nodes set, it can not


Maybe it's just my preference but here I would find it easier to read the "step N" comments inline with the code, rather than as a separate block that I need to scroll up to reference. And that way we could get rid of the "step N" tags themselves, which have to be kept up to date over time.

bertmaher · 2019-07-02T05:35:00Z

lib/Partitioner/Partitioner.cpp

+      // assigned to the next backend.
+
+      // Step 1:
+      auto nonSupported =


Probably want const auto &nonSupported = ... here, or this is going to invoke copy assignment.

bertmaher · 2019-07-02T05:35:10Z

lib/Partitioner/Partitioner.cpp

+      auto nonSupported =
+          backendMap_[backend->getBackendName()].nonSupportedNodesKind;
+      if (nonSupported.find(N.getKind()) != nonSupported.end()) {
+        // This op is on the pre-definded non-supported op list:


sp: pre-defined

bertmaher · 2019-07-02T05:36:36Z

lib/Partitioner/PartitionerUtils.cpp

+std::set<Kinded::Kind> generateNonSupportedNodes(llvm::StringRef names) {
+  std::set<Kinded::Kind> ret;
+  while (names.find(',') != std::string::npos) {
+    std::pair<llvm::StringRef, llvm::StringRef> newSplit = names.split(',');


/me wishes for folly::split (but of course we don't want to pull in folly, alas)

opti-mix

The implementation part is fine now! Thanks!

All of my comments this time are about tests.

opti-mix · 2019-07-02T15:22:52Z

tests/unittests/PartitionerTest.cpp

-        // heterogeneous partiton.
-        ASSERT_EQ(node->logicalDevices.size(), 1);
-      }
+  createSimpleModule(mod_);


I'm not sure what exactly you test here. You set up the backends, but you do not expect anything specific from partitions. Or do you?

Those backends are mock backends which are subclass of "Interpreter" or "CPU". They overload the isOpsupported() function to eliminate some nodes for Heterogeneous testing. Previously, we didn't have such "nonSupportedNodesKinds" nor "supportedNodesKinds" config. Therefore, we need to write mock backends for unittest test.

opti-mix · 2019-07-02T15:23:23Z

tests/unittests/PartitionerTest.cpp

+  CompilationContext cctx;
+  auto err = partitioner.Partition(cctx);
+  EXPECT_FALSE(errToBool(std::move(err)));
+  DAGListTy myList = std::move(partitioner.getPartitionResult());


Can you give this variable a more meaningful name than myList? ;-)

opti-mix · 2019-07-02T15:23:34Z

tests/unittests/PartitionerTest.cpp

+  CompilationContext cctx;
+  auto err = partitioner.Partition(cctx);
+  EXPECT_FALSE(errToBool(std::move(err)));
+  DAGListTy myList = std::move(partitioner.getPartitionResult());


Can you give this variable a more meaningful name than myList? ;-)

opti-mix · 2019-07-02T15:24:50Z

tests/unittests/PartitionerTest.cpp

+}
+
+/// Test pre-defined non-supported ops used for choosing backend in
+/// Heterogeneous Partition.


It is not obvious to me from the code and comments below how the non-supported ops are tested.

The config yaml file is only loaded in "loader" flow (I added resnet50 test in tests/images/run.sh to test loading and parsing the yaml file). For Partitioner unnitest, we just pass the pre-defined nodes list to DeviceInfo used in Partitioner. Added comments in the tests.

opti-mix · 2019-07-02T15:25:30Z

tests/unittests/PartitionerTest.cpp

+
+/// Test pre-defined non-supported ops used for choosing backend in
+/// Heterogeneous Partition.
+TEST_F(PartitionerTest, ConfigHeterogeneousPartitioning1) {


Could you give this test a more meaningful name reflecting what it does?

opti-mix · 2019-07-02T15:25:58Z

tests/unittests/PartitionerTest.cpp

+
+/// Test pre-defined supported ops used for choosing backend in
+/// Heterogeneous Partition.
+TEST_F(PartitionerTest, ConfigHeterogeneousPartitioning2) {


Could you give this test a more meaningful name reflecting what it does?

opti-mix · 2019-07-02T15:26:24Z

tests/unittests/PartitionerTest.cpp

+}
+
+/// Test pre-defined supported ops used for choosing backend in
+/// Heterogeneous Partition.


It is not obvious to me from the code and comments below how the supported ops are tested.

opti-mix · 2019-07-02T15:26:35Z

tests/unittests/PartitionerTest.cpp

+  CompilationContext cctx;
+  auto err = partitioner.Partition(cctx);
+  EXPECT_FALSE(errToBool(std::move(err)));
+  DAGListTy myList = std::move(partitioner.getPartitionResult());


Can you give this variable a more meaningful name than myList? ;-)

opti-mix · 2019-07-02T20:30:08Z

lib/Partitioner/Partitioner.cpp

@@ -981,6 +1003,10 @@ void Partitioner::getBackendMap(
      // is the same.
      // TODO : will improve the algorithm for different memory size.
      backendInfo.memSize = deviceInfo_[i].availableMemory;
+      backendInfo.nonSupportedNodesKinds =
+          generateNodeKindsSet(deviceInfo_[i].nonSupportedNodes);


Maybe std::move(generateNodeKindsSet(deviceInfo_[i].nonSupportedNodes)) to avoid copying the set?

opti-mix · 2019-07-02T20:32:41Z

tests/unittests/PartitionerTest.cpp

+  ASSERT_EQ(dagList.size(), 1);
+  ASSERT_TRUE(checkSaveNode(mod_));
+
+  int numOfInterpreter = 0;


The names sounds odd. Maybe numOfInterpreterNodes and numOfCPUNode?

Could you also check that it is exactly the kinds of nodes (or the nodes) you expected to be on CPU and Interpreter respectively?

opti-mix · 2019-07-02T20:33:02Z

tests/unittests/PartitionerTest.cpp

+  ASSERT_EQ(mod_.getFunctions().size(), 3);
+  ASSERT_EQ(dagList.size(), 1);
+  ASSERT_TRUE(checkSaveNode(mod_));
+  int numOfInterpreter = 0;


Same comment here about the names.

opti-mix

Looking good!

facebook-github-bot

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

facebook-github-bot · 2019-07-03T01:06:08Z

@beicy merged this pull request in dd135c9.

facebook-github-bot added the CLA Signed label Jun 28, 2019

opti-mix reviewed Jun 28, 2019

View reviewed changes

beicy force-pushed the rafatory branch 2 times, most recently from 5920f3d to a5eacc1 Compare July 1, 2019 23:17

opti-mix reviewed Jul 2, 2019

View reviewed changes

bertmaher reviewed Jul 2, 2019

View reviewed changes

beicy force-pushed the rafatory branch 4 times, most recently from efab6ca to ba8aa97 Compare July 2, 2019 07:41

opti-mix reviewed Jul 2, 2019

View reviewed changes

beicy force-pushed the rafatory branch 2 times, most recently from 06a9659 to d7f0189 Compare July 2, 2019 20:25

opti-mix reviewed Jul 2, 2019

View reviewed changes

beicy force-pushed the rafatory branch from cffede5 to 38c8973 Compare July 2, 2019 21:39

opti-mix approved these changes Jul 2, 2019

View reviewed changes

[Partitioner] Heterogeneous partition testing

34188ff

beicy force-pushed the rafatory branch from 38c8973 to 34188ff Compare July 2, 2019 22:02

facebook-github-bot reviewed Jul 2, 2019

View reviewed changes

facebook-github-bot closed this in dd135c9 Jul 2, 2019

facebook-github-bot added the Merged label Jul 3, 2019

beicy mentioned this pull request Jul 3, 2019

Heterogeneous partitioning example code #2814

Closed

[Partitioner] Heterogeneous partition testing #3197

[Partitioner] Heterogeneous partition testing #3197

Conversation

beicy commented Jun 28, 2019

opti-mix commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opti-mix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opti-mix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opti-mix left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 3, 2019