[WIP] Add host manager #2125

gcatron · 2018-12-05T20:50:57Z

Description: This is a work in progress to add the HostManager to glow.
The HostManager will serve as the entry point for execution calls and handle orchestrating the rest of the runtime components.
Testing: Not Yet
Documentation:

nadavrot · 2018-12-05T20:52:36Z

include/glow/Runtime/Host Manager/HostManager.h

@@ -0,0 +1,58 @@
+/**


Is there a space in the path name?

Maybe we can make it one word, like runtime or ExecutionEngine?

Agreed, space removed.

opti-mix · 2018-12-05T20:52:49Z

@gcatron Quick comment about naming. Please do not use spaces in the directory or file names.

nadavrot · 2018-12-05T20:54:40Z

ONNXifi is an example for one client that runs the multi-threaded inference. Correct? We should allow other clients (for example our unit tests and image-classifier) to also run inference concurrently.

gcatron · 2018-12-05T21:01:09Z

ONNXifi is an example for one client that runs the multi-threaded inference. Correct? We should allow other clients (for example our unit tests and image-classifier) to also run inference concurrently.

Agreed, The description was poorly worded. This would serve as the execution entry point for any client.

rdzhabarov · 2018-12-05T21:06:17Z

include/glow/Runtime/HostManager/HostManager.h

+  void clearHost();
+  /// Runs the network specified by \p networkID and \p functionName using the
+  /// provided \p context.
+  bool runNetwork(int networkID, llvm::StringRef functionName, Context context);


where function name is coming from? how is it used?

Can you expand on what each method does in details? (e.g., what removeNetwork actually does, etc)

rdzhabarov · 2018-12-05T21:07:00Z

include/glow/Runtime/HostManager/HostManager.h

+  /// provided \p context.
+  bool runNetwork(int networkID, llvm::StringRef functionName, Context context);
+  HostManager();
+  ~HostManager();


either virtual or class should be final

rdzhabarov · 2018-12-05T21:08:17Z

include/glow/Runtime/HostManager/HostManager.h

+  ~HostManager();
+};
+
+} // end namespace glow


no end here to be consistent with the rest of the codebase

rdzhabarov · 2018-12-05T21:09:22Z

Do we have a design doc shared on github?

jackm321 · 2018-12-05T21:27:57Z

include/glow/Runtime/HostManager/HostManager.h

+  /// Returns true if \p networkID is already added to the host.
+  bool networkAdded(int networkID);
+  /// Removes all networks from the host.
+  void clearHost();


also stops devices?

gcatron · 2018-12-05T23:12:48Z

Added a description of the design to #2045, the high level task tracking this work.

include/glow/Runtime/HostManager/HostManager.h

nadavrot · 2018-12-05T23:45:04Z

include/glow/Runtime/HostManager/HostManager.h

+class Executor {}; // temporary the Executor header will need to be included
+class Context;
+class HostManager final {
+  int activeCount_;


Why int and not unsigned? Can we have negative active tasks?

yinghai · 2018-12-06T00:42:20Z

What's the relationship between host manager and the onnxifi manager that @jackm321 is adding?

jackm321 · 2018-12-06T16:40:52Z

@yinghai good question, @gcatron and I were talking about about this offline a bit as well. Basically I think both of these things are fairly complementary and will be useful for their own tasks.
GlowOnnxifiManager will continue to manage things across the Onnxifi interface (especially things like onnxifi Events that we don't want to pull further into glow) and probably should continue to manage onnxifi Graphs too because it needs to manage not just the underlying glow graph (though this graph will probably be replaced with just the id returned from HostManager:: addNetwork) but it also will need to manage the context which can be updated by calls to onnxSetGraphIO(). Then a call to onnxRunGraph() become as simple as taking the onnxifi Graph and calling HostManager ::runNetwork() with the networkID generated by HostManager:: addNetwork and the context that it has stored for this graph in GlowOnnxifiManager.

yinghai · 2018-12-06T17:58:41Z

include/glow/Runtime/HostManager/HostManager.h

+class HostManager final {
+  int activeCount_;
+  int totalCount_;
+  std::unordered_map<int, DependencyGraph> networks_;


This is an overkill IMHO. Dense vector with index should be good. Better performance too.

@yinghai networks can be added and removed but their Ids (the keys in this map) are monotonically increasing so a vector could end up being sparse and unnecessarily wasting space

Btw, how do we initiate removal of network? We'd probably need high level ONNXIFI support for this.
I think use case is when model needs to be refreshed with new weights. There might be other use cases.

We remove network because high level application wants to retire certain model. From c2 level, onnxReleaseGraph is called and corresponding graph hit refcouter==0.

yinghai · 2018-12-06T17:59:40Z

include/glow/Runtime/HostManager/HostManager.h

+  /// Adds the network to the host and does the necessary setup work. This
+  /// includes partitioning, provisioning, compiling and initializing backends.
+  /// Returns the networkID of the network.
+  int addNetwork(Function *F);


Who compiles the Function? Where is weight sharing and partitioning done?

yinghai · 2018-12-06T18:00:15Z

lib/Runtime/HostManager/HostManager.cpp

+}
+
+HostManager::~HostManager() { clearHost(); }
+int HostManager::addNetwork(Function *F) {


Any of these methods should consider thread safety.

yinghai · 2018-12-06T18:36:37Z

@jackm321 @gcatron Thanks for the explanation. Let's put onnxifi manager to the glow runtime design doc so that we can see where it sits.

jackm321 · 2018-12-07T01:33:30Z

include/glow/Runtime/HostManager/HostManager.h

+class HostManager final {
+  /// Count of current networks being run.
+  std::atomic<unsigned int> activeCount_;
+  /// Count of networks initialized on this node.


"node" is kind of ambiguous here

Also totalCount_ is never decremented (because it's used to make networkIDs) so it's the total number of networks ever seen on the host not the total number loaded right now but this is confusing because activeCount_ sounds like it might be this however it is the number of in flight requests not networks (above it says "Count of current networks being run." but it could be multiple requests to the same network right so that should be changed?). I think it would be better to call these activeRequestsCount_ (same thing with activeRequestsLimit_) and something like nextNetworkId_ or something instead of totalCount_

jackm321 · 2018-12-07T01:47:42Z

lib/Runtime/HostManager/HostManager.cpp

+  auto result = promise.get_future();
+  executor_.runNetwork(networks_[networkID], functionName, context,
+                       [&promise](ResultCode id) { promise.set_value(id); });
+  result.wait();


Why is runNetwork() blocking when everything beneath it is async? Onnxifi's runGraph() is async and it will have to call runNetwork() so this should probably be async also or at least have an async version.

Actually since onnxifi has it's own thread pool this doesn't have to be async but it would be nice if we could just replace the onnxifi thread pool with this, otherwise we'll just have a bunch of onnxifi threads that are just sitting here blocking on this.

Maybe just chain the callback to signal the output event.

We should use one threadpool.

@yinghai yeah that's what I was thinking too

jackm321 · 2018-12-07T01:49:15Z

lib/Runtime/HostManager/HostManager.cpp

+  if (networks_.find(networkID) == networks_.end()) {
+    return FAILED;
+  }
+  if (activeCount_ > activeLimit_) {


What is the advantage of doing this over just blocking until activeCount_ < activeLimit_?

My thought is failing out would allow the request to get routed elsewhere. Blocking until available could lead to failure because too many requests have piled up.

jackm321 · 2018-12-07T01:51:06Z

lib/Runtime/HostManager/HostManager.cpp

+  if (networks_.find(networkID) == networks_.end()) {
+    return FAILED;
+  }
+  if (activeCount_ > activeLimit_) {


Also this should be if (activeCount_ >= activeLimit_)

jackm321 · 2018-12-07T02:03:48Z

lib/Runtime/HostManager/HostManager.cpp

+  int networkID = totalCount_;
+  auto dependencyGraph = partitioner_.partition(M);
+  auto executionDAG = provisioner_.provision(dependencyGraph);
+  networks_.emplace(networkID, executionDAG);


I think we should have a lock around networks_? Otherwise onnxifi layer would have to handle synchronization between calls to initGraph(), runGraph() etc

I think it would help it we specify which methods needs to handle concurrency in the host manager.

rdzhabarov · 2018-12-07T03:26:13Z

include/glow/Runtime/HostManager/HostManager.h

+}; // This will likely be defined in one common runtime place.
+class HostManager final {
+  /// Count of current networks being run.
+  std::atomic<unsigned int> activeCount_;


I do not understand why this is atomic, but there is no locking around other structs here.

yinghai · 2018-12-07T05:21:30Z

lib/Runtime/HostManager/HostManager.cpp

+}
+
+HostManager::~HostManager() { clearHost(); }
+unsigned int HostManager::addNetwork(Module *M) {


Question, if this takes in a module, how do we reuse weights in functions across module? @qcolombet told us that weights can be shared within module but not across module.

yinghai · 2018-12-07T05:24:11Z

lib/Runtime/HostManager/HostManager.cpp

+  auto result = promise.get_future();
+  executor_.runNetwork(networks_[networkID], functionName, context,
+                       [&promise](ResultCode id) { promise.set_value(id); });
+  result.wait();


Maybe just chain the callback to signal the output event.

yinghai · 2018-12-07T05:25:37Z

lib/Runtime/HostManager/HostManager.cpp

+  auto result = promise.get_future();
+  executor_.runNetwork(networks_[networkID], functionName, context,
+                       [&promise](ResultCode id) { promise.set_value(id); });
+  result.wait();


We should use one threadpool.

stale · 2019-01-11T01:33:30Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

stale · 2019-01-16T01:49:47Z

This PR has been automatically closed due to being stale for 5 days. Thank you for your contributions and feel free to reopen it in case of further progress.

jackm321 · 2019-01-18T05:51:16Z

@gcatron can we resurrect this? I think once we have this in I can get moving on moving all of the onnxifi stuff over to use host manager interface instead of ExecutionEngine which would be great.

gcatron · 2019-01-19T00:58:25Z

@gcatron can we resurrect this? I think once we have this in I can get moving on moving all of the onnxifi stuff over to use host manager interface instead of ExecutionEngine which would be great.

Absolutely, I just pushed some updates, looks like I can't reopen this because I did a force push. Opened #2284.

gcatron added 3 commits December 5, 2018 08:18

Added Host Manager class

7b88485

Stubbed out Host manager methods

2da9b69

updates to Host Manager

d4d6a91

gcatron added the WORK IN PROGRESS label Dec 5, 2018

facebook-github-bot added the CLA Signed label Dec 5, 2018

nadavrot reviewed Dec 5, 2018

View reviewed changes

remove space from directory name

2951fcd

rdzhabarov reviewed Dec 5, 2018

View reviewed changes

jackm321 reviewed Dec 5, 2018

View reviewed changes

Cleanup and comment improvements

2109c43

SplitInfinity changed the title ~~Add host manager~~ [WIP] Add host manager Dec 5, 2018

gcatron mentioned this pull request Dec 5, 2018

[Glow Runtime] Top Level Task #2045

Open

nadavrot reviewed Dec 5, 2018

View reviewed changes

include/glow/Runtime/HostManager/HostManager.h Outdated Show resolved Hide resolved

nadavrot reviewed Dec 5, 2018

View reviewed changes

yinghai reviewed Dec 6, 2018

View reviewed changes

Updated HostManager to match other component interfaces.

8919a52

jackm321 reviewed Dec 7, 2018

View reviewed changes

rdzhabarov reviewed Dec 7, 2018

View reviewed changes

yinghai reviewed Dec 7, 2018

View reviewed changes

Cleaned up variable names

7e52b55

stale bot added the stale_will_be_closed label Jan 11, 2019

stale bot closed this Jan 16, 2019

[WIP] Add host manager #2125

[WIP] Add host manager #2125

Conversation

gcatron commented Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opti-mix commented Dec 5, 2018

nadavrot commented Dec 5, 2018

gcatron commented Dec 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdzhabarov commented Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

gcatron commented Dec 5, 2018

Choose a reason for hiding this comment

yinghai commented Dec 6, 2018

jackm321 commented Dec 6, 2018

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

yinghai commented Dec 6, 2018

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

stale bot commented Jan 11, 2019

stale bot commented Jan 16, 2019

jackm321 commented Jan 18, 2019

gcatron commented Jan 19, 2019

gcatron commented Dec 5, 2018 •

edited

Loading

rdzhabarov commented Dec 5, 2018 •

edited

Loading