[Partitioner] User-defined partition #3237

beicy · 2019-07-16T07:14:25Z

Summary:
This PR added user-defined partition flow. Basically, a struct "PartitionConfig" containing the partition info is passed into Partitioner to enable this flow. Now we let users have the full control of how to do the partitioning.
To use this flow, users can write their helper function to generate PartitionConfig, and call Partitioner directly.
In the following PR, we will add passing PartitionConfig through HostManager from a yaml file.

Related to #2298
Documentation:

[Optional Fixes #issue]

Test Plan:
Added unittest. ninja test.

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

lib/Partitioner/Partitioner.cpp

tests/unittests/PartitionerTest.cpp

bertmaher · 2019-07-16T21:11:56Z

lib/Partitioner/Partitioner.cpp

@@ -52,6 +50,8 @@ static llvm::cl::opt<bool>
                  llvm::cl::desc("Enable dumping the graph of each partitions"),
                  llvm::cl::init(false), llvm::cl::cat(PartitionerCat));

+#include <fstream>


It doesn't look like fstream is used. (If it is, put it with the other #includes above.

bertmaher

I just have a handful of nits before landing. Thanks for prioritizing this :)

bertmaher · 2019-07-16T21:19:30Z

lib/Partitioner/Partitioner.cpp

@@ -1235,6 +1250,11 @@ llvm::Error Partitioner::Partition(CompilationContext &cctx) {
  // algorithm.
  F_ = selectRepFunc(module_, memSize_);

+  if (partitionConfig_.enabled()) {


Can this go before selectRepFunc (and maybe before getBackendMap)? It seems like it would circumvent the whole rest of the partitioning process so it'd be good to not do any irrelevant work.

will do it in front of selectRepFunc. However, we do need to run getBackendMap for later partition validation and logging.

bertmaher · 2019-07-16T21:20:45Z

lib/Partitioner/Partitioner.cpp

@@ -1348,27 +1368,97 @@ llvm::Error Partitioner::Partition(CompilationContext &cctx) {
    dumpDAG("DAG.dot");
  }

-  int i = 0;
  for (Function *subF : funcList) {
    (void)subF;


Only semi-related to this diff but I don't think (void)subF; is needed anymore, since subF is legitimately used below, even in Debug mode.

bertmaher · 2019-07-16T21:23:01Z

include/glow/Runtime/RuntimeTypes.h

+  /// a partition. 2. For i-th (0 <= i < m) partition, the nodes mapped to this
+  /// partition id are not in this map, and the nodes mapped to other partitions
+  /// ids must be in this map.
+  llvm::StringMap<size_t> nodeToParitition;


sp: Paritition -> Partition

bertmaher · 2019-07-16T21:27:00Z

lib/Partitioner/Partitioner.cpp

+    }
+  }
+
+  // Memory usage Validatian.


nit: sp and inconsistent caps. // Validate memory usage. would read better.

bertmaher · 2019-07-16T21:27:22Z

lib/Partitioner/Partitioner.cpp

+  }
+  RETURN_IF_ERR(memoryUsageValidation(partitionMap));
+
+  // Logical device ID validataion.


sp: validation

bertmaher · 2019-07-16T21:27:35Z

lib/Partitioner/Partitioner.cpp

+  logicalDeviceID_ = assignLogicalDeviceID(partitionMap);
+  RETURN_IF_ERR(logicalDevicesValidation(partitionMap));
+
+  // TODO : loop-free validataion.


sp: validation

artemrakhov-glow · 2019-07-16T22:18:33Z

include/glow/Runtime/RuntimeTypes.h

@@ -171,6 +171,28 @@ struct HostConfig {
  size_t executorThreads{3};
 };

+/// This is struct is for user defined partition.


Suggested change

/// This is struct is for user defined partition.

/// This is struct for user defined partition.

artemrakhov-glow · 2019-07-16T22:18:51Z

include/glow/Runtime/RuntimeTypes.h

@@ -171,6 +171,28 @@ struct HostConfig {
  size_t executorThreads{3};
 };

+/// This is struct is for user defined partition.
+struct PartitionConfig {
+  /// The name of the functiton to be partitioned.


Suggested change

/// The name of the functiton to be partitioned.

/// The name of the function to be partitioned.

artemrakhov-glow · 2019-07-16T22:20:14Z

include/glow/Runtime/RuntimeTypes.h

+struct PartitionConfig {
+  /// The name of the functiton to be partitioned.
+  std::string funcName;
+  /// The number of user defined partitions. The partition ids are [0,


Suggested change

/// The number of user defined partitions. The partition ids are [0,

/// The number of user defined partitions.

/// The partition ids are between 0 and numOfPartitions - 1, inclusive.

artemrakhov-glow · 2019-07-16T22:21:37Z

include/glow/Runtime/RuntimeTypes.h

+  /// a partition. 2. For i-th (0 <= i < m) partition, the nodes mapped to this
+  /// partition id are not in this map, and the nodes mapped to other partitions
+  /// ids must be in this map.
+  llvm::StringMap<size_t> nodeToParitition;


Suggested change

llvm::StringMap<size_t> nodeToParitition;

llvm::StringMap<size_t> nodeToPartition;

artemrakhov-glow · 2019-07-16T22:25:27Z

include/glow/Runtime/RuntimeTypes.h

+  std::vector<std::string> backendNames;
+  /// The name for each partition. partitionNames.size() == numOfPartitions.
+  std::vector<std::string> partitionNames;
+  /// The mapping between nodes and Partition ids. Assume there are n nodes and


Is this a map between node's name and unsigned partition number? It wasn't clear to me initially that name is used here. Also, need to clarify that Glow mangles names, meaning that Glow's node name is never the same as name from Caffe2 protobuf.

artemrakhov-glow · 2019-07-16T22:37:14Z

lib/Partitioner/Partitioner.cpp

+  NodeToFunctionMap partitionMap;
+  std::vector<Function *> funcList;
+  std::unordered_set<size_t> unUsed;
+  std::vector<NodesSet> nodesSets;


Suggested change

std::vector<NodesSet> nodesSets;

std::vector<NodesSet> nodesSets(partitionConfig_.numOfPartitions);

Then you won't need nodesSets.push_back(NodesSet{}); later.

artemrakhov-glow · 2019-07-16T22:38:36Z

lib/Partitioner/Partitioner.cpp

+    }
+  }
+
+  // If there is un used partition and un-mapped nodes, map those nodes to the


Suggested change

// If there is un used partition and un-mapped nodes, map those nodes to the

// If there is unused partition and unmapped nodes, map those nodes to the

artemrakhov-glow · 2019-07-16T22:39:16Z

lib/Partitioner/Partitioner.cpp

+      // unMaped list.
+      unMapped.push_back(&node);
+    } else {
+      size_t partitionID = iter->second;


Please check that partitionID is < partitionConfig_.numOfPartitions

artemrakhov-glow · 2019-07-16T22:40:19Z

lib/Partitioner/Partitioner.cpp

+  }
+
+  // If there is un used partition and un-mapped nodes, map those nodes to the
+  // un-used partition.


Suggested change

// un-used partition.

// unused partition.

artemrakhov-glow · 2019-07-16T22:41:19Z

lib/Partitioner/Partitioner.cpp

+  // If there is un used partition and un-mapped nodes, map those nodes to the
+  // un-used partition.
+  if (unMapped.size()) {
+    assert(unUsed.size() && "Missing node to partition mapping.");


Suggested change

assert(unUsed.size() && "Missing node to partition mapping.");

assert(unUsed.size() == 1 && "There must be exactly 1 unused partition.");

rdzhabarov · 2019-07-17T02:18:47Z

include/glow/Partitioner/Partitioner.h

@@ -209,6 +209,9 @@ class Partitioner {
  // heterogeneous partition.
  bool optimized_;

+  // The struct contain user-defined partition info.


doxygen style and above.

rdzhabarov · 2019-07-17T04:52:33Z

include/glow/Runtime/RuntimeTypes.h

+  /// The number of user defined partitions.
+  /// The partition ids are between 0 and numOfPartitions - 1, inclusive.
+  size_t numOfPartitions;
+  /// The backend for each partition. backendNames.size() == numOfPartitions.


is backendNames.size() == numOfPartitions. Why do we need numOfPartitions at all?

This means backendNames.size() should be equal to numOfPartitions. Otherwise, there is an error.

Can't we just have backendNames alone and the size tells how many partitions.

It also works. The reason I added this separate field is: I think it is clear for people just use a number directly as a reference. The we compare the size of partition names and backends in case something is missing by accident.

rdzhabarov · 2019-07-17T04:57:24Z

lib/Partitioner/Partitioner.cpp

+
+llvm::Error Partitioner::PartitionFromConfig() {
+  Function *F = module_->getFunction(partitionConfig_.funcName);
+  RETURN_ERR_IF_NOT(F, "Can't find the function in current module.");


please, add function name to the error message.

rdzhabarov · 2019-07-17T04:57:42Z

lib/Partitioner/Partitioner.cpp

+
+  NodeToFunctionMap partitionMap;
+  std::vector<Function *> funcList;
+  std::unordered_set<size_t> unUsed;


lib/Partitioner/Partitioner.cpp

+  // Create partitions based on the given number and names.
+  for (size_t i = 0; i < partitionConfig_.numOfPartitions; i++) {
+    Function *newF =
+        module_->createFunction(partitionConfig_.partitionNames[i]);


rdzhabarov · 2019-07-17T04:59:20Z

lib/Partitioner/Partitioner.cpp

+      unMapped.push_back(&node);
+    } else {
+      size_t partitionID = iter->second;
+      assert(partitionID < partitionConfig_.numOfPartitions &&


no asserts. DCHECK, add partition id to the error message.

rdzhabarov · 2019-07-17T04:59:45Z

lib/Partitioner/Partitioner.cpp

+  // If there is unused partition and unmapped nodes, map those nodes to the
+  // unused partition.
+  if (unMapped.size()) {
+    assert(unUsed.size() == 1 && "There must be exactly 1 unused partition.");


rdzhabarov · 2019-07-17T05:00:19Z

tests/unittests/PartitionerTest.cpp

@@ -886,3 +886,28 @@ TEST_F(PartitionerTest, memoryUsageValidation1) {
  auto err = myPartitioner.Partition(cctx);
  EXPECT_TRUE(errToBool(std::move(err)));
 }
+
+/// This one tests partition from a user-defined config.
+TEST_F(PartitionerTest, partitionFromConfig1) {


nit: partitionFromConfig

facebook-github-bot

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

gcatron · 2019-07-17T20:37:23Z

lib/Partitioner/Partitioner.cpp

@@ -1211,7 +1225,6 @@ llvm::Error Partitioner::QuantizationProfilingPartition(
  module_->eraseFunction(F_);


Is it possible that the function can be renamed and will no longer match the name in partitionConfig?

No. If partitionConfig is set, Partitioner will go to user-defined flow without considering other options like "-load-profiling". So later renaming in the rest flow won't affect the function name.

artemrakhov-glow · 2019-07-17T21:28:32Z

include/glow/Runtime/RuntimeTypes.h

+  /// mapped to this partition id are not in this map, and the nodes mapped to
+  /// other partitions ids must be in this map. The node's name should be the
+  /// name in Glow function and may be different from the original name from
+  /// models. Since Glow will mangle names to make them unique.


It's not only to make them unique. We also e.g. replace / with _. Maybe something else.
You may write: Since Glow will mangle names according to its own conventions.

artemrakhov-glow · 2019-07-17T21:36:31Z

include/glow/Partitioner/Partitioner.h

  Partitioner(Module *parent, const std::vector<DeviceInfo> &devices,
-              bool saturateHost = false, bool optimized = false);
+              bool saturateHost = false, bool optimized = false,
+              PartitionConfig partitionConfig = PartitionConfig());


You can see that it's already bad API. Let's say caller 1 has only partitionConfig. But now it has to pass false, false for some other 2 flags. Caller 1 has to figure out what is saturation, and what to pass for corresponding flag to disable.

It would be a good practice to make sure that incompatible flags are not passed together. E.g. saturation=false when user defined partition. But these checks can quickly explode if we have more flags/modes.

What if we later introduce another feature, third mode for partitioner? Say, PyTorch-defined partition? Then caller 3 will have to figure out what to pass for partitionConfig argument to disable user-defined partition feature.

This is why I think we should never accept a whole all-inclusive list of args in one function (in this case, partitioner constructor). I was going to propose create different method for user defined partition. But if you are going to refactor everything later into different classes, I'm also fine with that.

artemrakhov-glow · 2019-07-17T21:39:15Z

lib/Partitioner/Partitioner.cpp

+      partitionMap.add(node, funcList[partitionID]);
+      nodesSets[partitionID].insert(node);
+    }
+  }


else {
what if there are unused partitions? Meaning that some partitions are empty. Is it valid?

There can be at most 1 unused partition (there is the check). Otherwise, it is invalid. Since it means we have empty functions in the module and will cause problems later.

will cause problems later

It is better to nail these problems early. Here we still can report what exactly is wrong: no nodes were mapped to a partitions #i, #j, #k... Later it will be difficult to track down where the empty function is coming from. In fact, it may fail with obscure error, so first debugging step would be to figure out that the function is empty, and only after that find out why it's empty. I really suggest to report as many problems with the partition as we can find.

Yes, I agree. So far, if there is empty function, it will cause the assertion and stop. But you are right, the error message is not enough. Will improve the debugging ability. Thanks!

gcatron

LGTM Great work!

beicy · 2019-07-17T22:37:27Z

@artemrakhov Thanks a lot for the suggestion on the interface. Let's have more discussion about the interface offline.

facebook-github-bot · 2019-07-18T01:03:22Z

@beicy merged this pull request in 809a3e8.

facebook-github-bot added the CLA Signed label Jul 16, 2019

beicy force-pushed the auto1 branch from e1d9e26 to c2ca40b Compare July 16, 2019 07:16

rdzhabarov reviewed Jul 16, 2019

View reviewed changes

lib/Partitioner/Partitioner.cpp Outdated Show resolved Hide resolved

beicy force-pushed the auto1 branch from c2ca40b to 2054a24 Compare July 16, 2019 17:23

rdzhabarov reviewed Jul 16, 2019

View reviewed changes

tests/unittests/PartitionerTest.cpp Outdated Show resolved Hide resolved

bertmaher reviewed Jul 16, 2019

View reviewed changes

beicy force-pushed the auto1 branch from 2054a24 to 4dcceeb Compare July 16, 2019 21:29

bertmaher requested changes Jul 16, 2019

View reviewed changes

artemrakhov-glow suggested changes Jul 16, 2019

View reviewed changes

beicy force-pushed the auto1 branch 2 times, most recently from bfb931b to c21d412 Compare July 16, 2019 23:14

rdzhabarov reviewed Jul 17, 2019

View reviewed changes

beicy force-pushed the auto1 branch from c21d412 to 212196b Compare July 17, 2019 07:18

[Partitioner] User-defined partition

6637109

beicy force-pushed the auto1 branch from 212196b to 6637109 Compare July 17, 2019 19:23

bertmaher approved these changes Jul 17, 2019

View reviewed changes

facebook-github-bot reviewed Jul 17, 2019

View reviewed changes

gcatron reviewed Jul 17, 2019

View reviewed changes

artemrakhov-glow reviewed Jul 17, 2019

View reviewed changes

artemrakhov-glow approved these changes Jul 17, 2019

View reviewed changes

gcatron approved these changes Jul 17, 2019

View reviewed changes

facebook-github-bot closed this in 809a3e8 Jul 18, 2019

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

beicy deleted the auto1 branch July 25, 2019 22:08

	/// This is struct is for user defined partition.
	/// This is struct for user defined partition.

	/// The name of the functiton to be partitioned.
	/// The name of the function to be partitioned.

	/// The number of user defined partitions. The partition ids are [0,
	/// The number of user defined partitions.
	/// The partition ids are between 0 and numOfPartitions - 1, inclusive.

	llvm::StringMap<size_t> nodeToParitition;
	llvm::StringMap<size_t> nodeToPartition;

	std::vector<NodesSet> nodesSets;
	std::vector<NodesSet> nodesSets(partitionConfig_.numOfPartitions);

	// If there is un used partition and un-mapped nodes, map those nodes to the
	// If there is unused partition and unmapped nodes, map those nodes to the

	assert(unUsed.size() && "Missing node to partition mapping.");
	assert(unUsed.size() == 1 && "There must be exactly 1 unused partition.");

		@@ -1211,7 +1225,6 @@ llvm::Error Partitioner::QuantizationProfilingPartition(
		module_->eraseFunction(F_);

[Partitioner] User-defined partition #3237

[Partitioner] User-defined partition #3237

Conversation

beicy commented Jul 16, 2019

Choose a reason for hiding this comment

bertmaher 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

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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beicy Jul 17, 2019 • edited

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

gcatron left a comment

Choose a reason for hiding this comment

beicy commented Jul 17, 2019

facebook-github-bot commented Jul 18, 2019

beicy Jul 17, 2019 •

edited