[WIP] Adding RuntimeTypes.h #2161

gcatron · 2018-12-11T22:48:19Z

Description: Adds RuntimeTypes.h which contains data structures common to the Runtime Components.
Testing: Not yet
Documentation: NA
Related to #2045

beicy · 2018-12-11T23:00:42Z

include/glow/Runtime/RuntimeTypes.h

+  /// Mapping of Module * to a vector of it's dependancies.
+  std::unordered_map<Module *, std::vector<Module *>> dependencies;
+  /// Mapping of Module * to a vector of networks that depend on it.
+  std::unordered_map<Module *, std::vector<Module *>> dependents;


Is this a revert DAG ?

yes, after talking with @SplitInfinity we decided it would be helpful to have both directions. If it turns out this can be removed we can pull it later.

beicy · 2018-12-11T23:02:32Z

include/glow/Runtime/RuntimeTypes.h

+  /// Mapping of Module * to a vector of output placeholder names for the
+  /// network.
+  std::unordered_map<Module *, std::vector<std::string>> outputs;
+};


Each module contains its placeholder and constants. So what the input and output for?

They are for runtime, at runtime the modules are no longer around but the executor will need to know how to pass inputs and outputs when executing the network.

Removed from DependencyDAG, they will by generated by the HM and added to the executionDAG.

beicy · 2018-12-11T23:05:54Z

include/glow/Runtime/RuntimeTypes.h

+  /// Total useable memory on device in MB
+  double maximumUsableMemory;
+};
+


For Partitioner, we need the DeviceInfo contains: memory, number of devices, later the communication constraints.

Would the communication contraints be at a system level or device level? The thought here is the Partitioner would be given a vector of these DeviceInfo objects, one per device. Number of devices would be the length of the vector.

"Would the communication contraints be at a system level or device level?"
-- device level. Should be device specific.
It is OK to put it here, but for Partitioner, I don't think we need "double availableMemory;". Please correct me if I miss anything.

You are correct, we don't need availableMemory right now for Partitioner, but it could be used for provisioner and potentially later in Partitioner.

bertmaher · 2018-12-12T00:10:34Z

include/glow/Runtime/RuntimeTypes.h

+
+/// Data structure containing the output from the Partitioner. It is consumed by
+/// the Provisioner and used to generate an executionDAG.
+struct DependancyDAG {


I prefer the spelling "dependency", but Wiktionary informs me that "dependancy" is an acceptable if archaic form. So thou mayest leave the spelling alone if thou whilst :-D

Good catch I prefer dependency myself, consider it changed.

bertmaher · 2018-12-12T00:13:49Z

include/glow/Runtime/RuntimeTypes.h

+
+struct DeviceMemoryInfo {
+  /// Available memory on device in MB
+  double availableMemory;


I'd use size_t for memory, since it's not really a floating-point quantity. I also have a slight preference for bytes (rather than a bigger unit like MB), but I also know it's kind of annoying to look at quantities like 536,870,912 instead of "512", so your discretion there.

Actually, size_t might be a little awkward since it's based on the host's address space. In practice I don't think it would ever matter, but maybe a uint64_t would be better?

changed to uint64_t.

nadavrot · 2018-12-12T06:04:38Z

include/glow/Runtime/RuntimeTypes.h

+namespace glow {
+namespace runtime {
+
+using NetworkID = size_t;


This is not the device id. It is the device id type. Please call it NetworkIDTy

nadavrot · 2018-12-12T06:04:56Z

include/glow/Runtime/RuntimeTypes.h

+/// and runtime.
+enum ResultCode { READY, EXECUTED, FAILED, CANCELLED };
+
+struct DeviceMemoryInfo {


beicy · 2018-12-28T04:33:15Z

include/glow/Runtime/RuntimeTypes.h

+/// Individual Node in the DAG for a given network. This contains all the
+/// information needed to run the sub-network at inference time.
+struct DAGNode {
+  /// The children of this node, these are nodes that depend on the current


So, after graph partitioning, each function should have a DAGNode, right? Please correct me if I misunderstand something... If so, we should have some field in this struct to point the function ? or the "name" in DAGNode is the function name?

You are correct, name in the DAGNode is the function name.

beicy · 2018-12-28T17:31:48Z

Looks like ci failed due to the head file is not used by any other file... Anyway to avoid this failure?

beicy · 2018-12-28T17:32:34Z

include/glow/Backends/BackendUtils.h

@@ -30,6 +30,10 @@ struct RuntimeSymbolInfo {
  size_t offset;
  /// Type of symbol.
  Type type;
+  /// Is the symbol an input for the function.


please run "format.sh fix"

Format.sh fix didn't change anything? Is it the indentation?

Yes, I would like the indentation should be the consistent with the rest of file.

beicy · 2019-01-10T21:03:43Z

include/glow/Runtime/RuntimeTypes.h

+  /// Pointers to the parents of this node. This is used by the executor for
+  /// determining if a given node has all dependencies met.
+  std::vector<DAGNode *> parents;
+  /// ID of the deviceManager that this network is assigned to.


The "children" is with the type of "std::vector" and "parents" is with the type of "std::vector<DAGNode*>". Actually, this would cause some complexity when creating the DAGNode. For each DAGNode of each sub-function, if we declare it as a local var, we have no idea how to push_back its pointer to its children's "parents" filed. If we new a DAGNode for each function, the "children" field is improper. After discussed with @gcatron , can we use "std::vector<DAGNode*> children;" instead of "std::vector children;", and add an Destructor of DAGNode? @nickgg

stale · 2019-01-15T22:56:47Z

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-20T23:16:37Z

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.

Adding RuntimeTypes.h to contain runtime common data structures.

d1ad7ea

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

beicy reviewed Dec 11, 2018

View reviewed changes

bertmaher reviewed Dec 12, 2018

View reviewed changes

gcatron added 2 commits December 11, 2018 16:15

Dependancy -> dependency

f00f8fa

Changed to bytes removed inputs/outputs from DependencyDAG

2078cf6

nadavrot reviewed Dec 12, 2018

View reviewed changes

gcatron added 3 commits December 12, 2018 11:12

Added comments updated type names.

514a459

Added moduleSize to DependencyDAG

e35408b

Removed Maxmemory from DeviceMemoryInfo and renamed to DeviceInfo

4f5acea

bertmaher added the WORK IN PROGRESS label Dec 22, 2018

Updated RuntimeTypes to use new DAGNode, added input bool to symbolInfo

a7bfe84

gcatron force-pushed the create_runtime_types branch from e1637c3 to a7bfe84 Compare December 28, 2018 00:52

Added output to symbolInfo

b796396

beicy reviewed Dec 28, 2018

View reviewed changes

beicy reviewed Jan 10, 2019

View reviewed changes

nickgg mentioned this pull request Jan 10, 2019

DeviceManager interface and CPUDeviceManager #2249

Merged

stale bot added the stale_will_be_closed label Jan 15, 2019

stale bot closed this Jan 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Adding RuntimeTypes.h #2161

[WIP] Adding RuntimeTypes.h #2161

gcatron commented Dec 11, 2018

beicy Dec 11, 2018

gcatron Dec 11, 2018

beicy Dec 11, 2018

gcatron Dec 11, 2018

gcatron Dec 12, 2018

beicy Dec 11, 2018

gcatron Dec 11, 2018 •

edited

Loading

beicy Dec 12, 2018

gcatron Dec 12, 2018

bertmaher Dec 12, 2018

gcatron Dec 12, 2018

bertmaher Dec 12, 2018

bertmaher Dec 12, 2018

gcatron Dec 12, 2018

nadavrot Dec 12, 2018

nadavrot Dec 12, 2018

beicy Dec 28, 2018

gcatron Dec 28, 2018

beicy commented Dec 28, 2018

beicy Dec 28, 2018

gcatron Dec 28, 2018

beicy Dec 28, 2018

beicy Jan 10, 2019

stale bot commented Jan 15, 2019

stale bot commented Jan 20, 2019

[WIP] Adding RuntimeTypes.h #2161

[WIP] Adding RuntimeTypes.h #2161

Conversation

gcatron commented Dec 11, 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

gcatron Dec 11, 2018 • edited Loading

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

beicy commented Dec 28, 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

stale bot commented Jan 15, 2019

stale bot commented Jan 20, 2019

gcatron Dec 11, 2018 •

edited

Loading