[Partitioner] Partitioner refactoring 3 #3384

beicy · 2019-08-05T22:23:26Z

Summary:
In this PR:

Added class ParititionerBase which contains interfaces and common functions used for partitioner. and the class Partitioner is now derived from class ParititionerBase .
In class Partitioner, we have 3 partition approaches:

llvm::Error HeterogeneousPartition(CompilationContext &cctx);
llvm::Error QuantizationProfilingPartition(CompilationContext &cctx);
llvm::Error PartitionFromConfig(PartitionConfig &partitionConfig);

The above 3 method can be called directly by users, or chosen by
llvm::Error Partition(CompilationContext &cctx)

Note: loadBalancedPartitioning is still included in HeterogeneousPartition since it uses the result of current partitioner approach. Will take it from HeterogeneousPartition in next PR and make it as the 4th partition approach.

Documentation:

[Optional Fixes #issue]
#2298
Test Plan:
Ninja test, ./tests/image/run.sh
Please see a detailed explanation of how to fill out the fields in the relevant sections in PULL_REQUEST.md.

rdzhabarov · 2019-08-06T21:32:19Z

include/glow/Partitioner/PartitionerBase.h

+using namespace runtime;
+/// Given a module, partitions each of the its functions into multiple ones
+/// based on memory constraints and minimizes the communication cost.
+class PartitionerBase {


let's clean up this interface a bit as discussed offline.

bertmaher · 2019-08-06T23:24:06Z

include/glow/Partitioner/PartitionerBase.h

+
+  /// Decompose each function in a module. \p cctx is used in function
+  /// optimization. \returns whether there was an error encountered.
+  virtual llvm::Error Partition(CompilationContext &cctx) {


Nit: should be lowercase p to match our coding conventions.

bertmaher · 2019-08-06T23:24:38Z

include/glow/Partitioner/Partitioner.h

-  /// among different type of devices. \p cctx is used during optimization of
-  /// the Function. \returns whether there was an error encountered.
-  llvm::Error Partition(CompilationContext &cctx);
+  llvm::Error PartitionFromConfig(PartitionConfig &partitionConfig);


Same initial-caps nit, here and a few places below.

facebook-github-bot

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

jfix71

BTW while you're refactoring, how do you feel about dropping the Ty suffix from the partitioner/runtime types? I think @bertmaher has mentioned this before. E.g. DAGListTy, DeviceIDTy, etc.

jfix71 · 2019-08-07T16:46:39Z

include/glow/Partitioner/Partitioner.h

  /// The representative function used for partition. We choose the function who
  /// has the largest memory size.
  Function *F_;

+  /// The partition result.
+  DAGListTy partitions_;


Would it make sense for this to be moved to PartitionerBase? PartitionerBase::partition() will always return a reference to this DAGListTy, right?

"PartitionerBase::partition() will always return a reference to this DAGListTy, right?"
-- Yes.
"Would it make sense for this to be moved to PartitionerBase? "
-- After the offline discussion with @rdzhabarov, we would like to make the PartitionerBase "stateless".

Question: do we really need to keep DAGListTy partitions_; in partitioner.

Role of the partitioner is to generate DAGList and return it back to the caller (once generated it's never accessed in partitioner).
What do we gain by keeping partitions_ here?

Removed DAGListTy partitions_; in partitioner.

jfix71 · 2019-08-07T16:47:17Z

include/glow/Partitioner/PartitionerBase.h

+  virtual ~PartitionerBase() = default;
+
+  /// Decompose each function in a module. \p cctx is used in function
+  /// optimization. \returns whether there was an error encountered.


nit: Think this was copy+paste -- but I don't think it's correct anymore to say it returns whether there was an error encountered. In normal circumstances it actually returns the DAGList.

beicy · 2019-08-07T17:11:33Z

BTW while you're refactoring, how do you feel about dropping the Ty suffix from the partitioner/runtime types? I think @bertmaher has mentioned this before. E.g. DAGListTy, DeviceIDTy, etc.

For "Ty", if we decide to move that, I think it is better to get a separate PR, since other parts of Runtime (e.g. Provisioner) use those date type as well.

jfix71 · 2019-08-07T17:33:52Z

include/glow/Partitioner/Partitioner.h

-  /// need to be created, we use \p backendsHolder to hold them for memory
-  /// purpose.
-  void getBackendMap(std::map<std::string, BackendInfo> &backendMap,
+  /// Greate the map between the backend name and the concrete backend info


typo: Greate -> Create

facebook-github-bot

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

rdzhabarov · 2019-08-07T22:07:24Z

include/glow/Partitioner/Partitioner.h


-  FunctionToBackendNameMap
-  backendBasedPartition(Function *F, std::vector<Backend *> &backends,
+  /// Do the partition based on backend types.


could you add some description to the doxygen, e.g., what F is and what's stored in functobackend map etc.

rdzhabarov · 2019-08-07T22:08:24Z

include/glow/Partitioner/Partitioner.h

-  /// the Function. \returns whether there was an error encountered.
-  llvm::Error Partition(CompilationContext &cctx);
+  llvm::Expected<DAGListTy>
+  partitionFromConfig(PartitionConfig &partitionConfig);


why config is not a const here?

rdzhabarov · 2019-08-07T22:10:49Z

include/glow/Partitioner/Partitioner.h

-                                             Function *F,
-                                             std::vector<Backend *> backends);
+  llvm::Expected<DAGListTy>
+  quantizationProfilingPartition(CompilationContext &cctx);


why all these special methods are part of the public interface of partitioner?

Discussed with @artemrakhov before. It allows users to call those method directly. For example, user-defined partitioner can be called directly without setting params, e.g "optimized_" and "saturateHost_". @artemrakhov could you please also help to double check the interface? Thanks!

Yes, this is the right direction! It is very useful to be able to call just one method.

But Roman wanted to bring a different question to your attention. Can we create more Partitioner-based (inherited) classes? I.e. we could further introduce UserDefinedPartitioner, SaturatingPartitioner, and so on. Whatever makes sense. That way, Partitioner class itself won't implement any "special" partitions. It will be a very general class.

Yes, this is the right direction! It is very useful to be able to call just one method.

But Roman wanted to bring a different question to your attention. Can we create more Partitioner-based (inherited) classes? I.e. we could further introduce UserDefinedPartitioner, SaturatingPartitioner, and so on. Whatever makes sense. That way, Partitioner class itself won't implement any "special" partitions. It will be a very general class.

We now have "class PartitionerBase" and new partitioner class can be derived from that. The reason we have current partition flows in Partitioner because they shares lots of common data types, internal members. We would like to keep "class PartitionerBase" stateless.

We can do further inheritance. For example: PartitionerBase -> Partitioner -> UserDefinedPartitioner

rdzhabarov · 2019-08-07T22:14:39Z

lib/Partitioner/Partitioner.cpp

@@ -723,30 +542,20 @@ llvm::Error Partitioner::QuantizationProfilingPartition(
        << "Profiling a model to be partitioned cross different backends. Each "
           "sub-network will be optimized and run on cpu backend.\n";
  }
-  return llvm::Error::success();
+  return partitions;


while refactoring can you replace asserts with DCHECK and add more detail to error message where applicable.

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

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

jfix71

LGTM! Just a series of doxygen/comment stuff.

jfix71 · 2019-08-09T21:00:00Z

include/glow/Partitioner/Partitioner.h

@@ -83,10 +83,13 @@ class Partitioner {
                                     llvm::StringRef backendName);

  /// Duplicates all networks in the module order to saturate the Host.


nit: Update doxygen to mention the method parameters.

jfix71 · 2019-08-09T21:00:42Z

include/glow/Partitioner/Partitioner.h

+  /// Partition a function \p F based on backends \p backends. \returns the
+  /// final result and a map between partitions and backend names.
+  llvm::Expected<DAGListTy>
+  backendBasedPartition(FunctionToBackendNameMap &FuncToBackend, Function *F,


nit: uncapitalize FuncToBackend (matches the cpp definition)

jfix71 · 2019-08-09T21:01:07Z

include/glow/Partitioner/Partitioner.h

-  FunctionToBackendNameMap
-  backendBasedPartition(Function *F, std::vector<Backend *> &backends,
+  /// Partition a function \p F based on backends \p backends. \returns the
+  /// final result and a map between partitions and backend names.


nit: Update doxygen with all method params

jfix71 · 2019-08-09T21:01:46Z

include/glow/Partitioner/Partitioner.h

-  /// partition result.
-  void doPartitioning(llvm::StringRef funcName, std::vector<Function *>,
-                      NodeToFunctionMap &mapping, bool saveDAG);
-
  /// If there is no need to do any partition, just generate the DAGNode based
  /// on current functions in this module for backend \p backendName found in \p
  /// backendMap. \p cctx is used during optimization of the Function. \returns
  /// whether there was an error encountered.


nit: This doesn't return if an error was encountered, it returns the DAG list or else an error.

jfix71 · 2019-08-09T21:02:17Z

include/glow/Partitioner/Partitioner.h

-  /// the Function. \returns whether there was an error encountered.
-  llvm::Error Partition(CompilationContext &cctx);
+  llvm::Expected<DAGListTy>
+  partitionFromConfig(const PartitionConfig &partitionConfig);

  /// This partition approach is used in Glow Quantization Profiling flow. The
  /// backendBasedPartition is applied first in case there are heterogeneous
  /// backends. Then each sub-function will be compiled and run in CPU backend
  /// for profiling.


nit: \returns ... and mention cctx.

jfix71 · 2019-08-09T21:23:57Z

include/glow/Partitioner/PartitionerBase.h

+  void dumpDAG(llvm::StringRef dotFilename, const DAGListTy &partitions) const;
+
+protected:
+  /// Given the node-function mapping, do the actual partitioning. If \p saveDAG


nit: \p mapping etc. and \returns instead of partitions_.

jfix71 · 2019-08-09T21:25:13Z

lib/Partitioner/Partitioner.cpp

@@ -237,13 +178,14 @@ NodeToFunctionMap Partitioner::selectPartitions(Function *F,
 /// Duplicate the network to saturate the number of devices. For example: If a


Looks like this doxygen can be moved to the header?

jfix71 · 2019-08-09T21:26:15Z

lib/Partitioner/Partitioner.cpp

@@ -825,7 +641,7 @@ llvm::Error Partitioner::Partition(CompilationContext &cctx) {
  }

  // Step 5 : do the real partitioning for the function list.
-  doPartitioning(origName, funcs, mapping, true);
+  partitions = doPartitioning(origName, funcs, module_, mapping, true);


nit: /* saveDAG */ true

jfix71 · 2019-08-09T21:26:22Z

lib/Partitioner/Partitioner.cpp

@@ -931,15 +751,15 @@ llvm::Error Partitioner::PartitionFromConfig() {
  // TODO : loop-free validation.

  // Do partition.
-  doPartitioning(F->getName(), {F}, partitionMap, true);
+  partitions = doPartitioning(F->getName(), {F}, module_, partitionMap, true);


nit: /* saveDAG */ true

jfix71 · 2019-08-09T21:27:24Z

lib/Partitioner/PartitionerBase.cpp

+using namespace glow;
+using llvm::isa;
+
+/// Current only partition the representative function.


nit: Don't need doxygen in cpp files.

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-08-12T23:32:30Z

@beicy merged this pull request in 3706588.

facebook-github-bot added the CLA Signed label Aug 5, 2019

beicy force-pushed the ref2 branch 2 times, most recently from cbe8afa to f59a8a8 Compare August 6, 2019 21:18

rdzhabarov reviewed Aug 6, 2019

View reviewed changes

bertmaher reviewed Aug 6, 2019

View reviewed changes

beicy force-pushed the ref2 branch from f59a8a8 to 8ce9a71 Compare August 7, 2019 04:44

facebook-github-bot reviewed Aug 7, 2019

View reviewed changes

jfix71 reviewed Aug 7, 2019

View reviewed changes

beicy force-pushed the ref2 branch from 8ce9a71 to 44040a3 Compare August 7, 2019 17:20

jfix71 reviewed Aug 7, 2019

View reviewed changes

beicy force-pushed the ref2 branch 2 times, most recently from 0c52c95 to 8c56fbc Compare August 7, 2019 20:41

facebook-github-bot reviewed Aug 7, 2019

View reviewed changes

beicy force-pushed the ref2 branch 2 times, most recently from 64b05ce to c69a38b Compare August 7, 2019 21:31

rdzhabarov reviewed Aug 7, 2019

View reviewed changes

beicy force-pushed the ref2 branch 4 times, most recently from ecebbe7 to 3f969aa Compare August 8, 2019 04:19

facebook-github-bot reviewed Aug 8, 2019

View reviewed changes

beicy force-pushed the ref2 branch 4 times, most recently from 49de964 to b0842f1 Compare August 9, 2019 17:14

facebook-github-bot reviewed Aug 9, 2019

View reviewed changes

jfix71 approved these changes Aug 9, 2019

View reviewed changes

[Partitioner] Partitioner refactoring 3

8b5c54f

beicy force-pushed the ref2 branch from b0842f1 to 8b5c54f Compare August 12, 2019 19:08

facebook-github-bot reviewed Aug 12, 2019

View reviewed changes

facebook-github-bot closed this in 3706588 Aug 12, 2019

facebook-github-bot added the Merged label Aug 12, 2019

chadrosier mentioned this pull request Sep 12, 2019

Unchecked Expected introduced in 3706588 #3505

Closed

		@@ -83,10 +83,13 @@ class Partitioner {
		llvm::StringRef backendName);

		/// Duplicates all networks in the module order to saturate the Host.

		@@ -237,13 +178,14 @@ NodeToFunctionMap Partitioner::selectPartitions(Function *F,
		/// Duplicate the network to saturate the number of devices. For example: If a

[Partitioner] Partitioner refactoring 3 #3384

[Partitioner] Partitioner refactoring 3 #3384

Uh oh!

Conversation

beicy commented Aug 5, 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!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

jfix71 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!

rdzhabarov Aug 7, 2019 • edited 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!

beicy commented Aug 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

rdzhabarov Aug 7, 2019 • edited 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!

rdzhabarov Aug 7, 2019 • edited 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!

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!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

jfix71 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!

rdzhabarov Aug 7, 2019 •

edited

Loading

rdzhabarov Aug 7, 2019 •

edited

Loading

rdzhabarov Aug 7, 2019 •

edited

Loading