Refactor Executor and OpGraph #540

klecki · 2019-02-14T15:13:43Z

Check some constraints on OpGraph separately from Executor
processing the OpGraph

Add static traits for OpGraph constraints

Unify OpNodes processing for different OpType

Executor "owns" buffer for corresponding TensorNodes,
using data factory and related types

next: #551

mzient · 2019-02-14T15:22:42Z

dali/common.h

+    case DALIOpType::SUPPORT:
+      return "SUPPORT";
+    default:
+      return "INVALID OP TYPE";


I'd prefer something that doesn't contain spaces and is obviously wrong - like "<INVALID>". Motivation for not having spaces is easier parsing, should it ever be necessary.

mzient · 2019-02-14T15:28:04Z

dali/pipeline/executor/executor.h

+    workspace_owner_t op_data;
+
+    void Resize(int support, int cpu, int mixed, int gpu) {
+      std::get<static_cast<int>(DALIOpType::SUPPORT)>(op_data).resize(support);


This hurts my eyes, really. If it were not an enum class the same line would look like this:
std::get<DALI_OP_SUPPORT>(op_data).resize(support)

mzient · 2019-02-14T15:34:37Z

dali/pipeline/workspace/workspace_data_factory.h

+
+template <typename Backend>
+bool IsPinned(HostWorkspace::output_t<Backend> &t) {
+  bool is_pinned = true;


I see a contradiction here. On one hand, you assume that pinned is default, because empty workspace is implicitly pinned. On the other hand, you implement all_of predicate here, making it no-so-default after all (it takes just one non-pinned tensor in the workspace to mark it as non-pinned).

The thing is, currently we pin the memory for the whole batch, and I wanted to be able to call SetPinned(any_of_workspace_output_types, true) in consistent manner, instead of having to write

device_output->set_pinned(...)

and

for (auto t in host_output) t->set_pinned(...)

jantonguirao · 2019-02-14T15:32:22Z

dali/pipeline/executor/executor.h

-      mixed_op_data.clear();
-      gpu_op_data.clear();
-      support_op_data.clear();
+      std::get<0>(op_data).clear();


shouldn't those be enum values?

They are, in some next PR, forgot to propagate it here.

so maybe good idea to add it here before merging

jantonguirao · 2019-02-14T15:33:48Z

dali/pipeline/executor/executor.h

+// We instantiate the operation of adding the input only for parent op_type and device
+// that are specifically allowed
+template <DALIOpType op_type, DALIOpType producer_type, DALITensorDevice device>
+en_if_t<allows_op_input<op_type>(producer_type) && allows_tensor_input<op_type>(device)> add_input(


I suppose that en_if_t means enable_if_t. I'd rather splurge on having the extra characters :)

...or make it if_t ;)

mzient · 2019-02-14T15:41:56Z

dali/pipeline/workspace/workspace_data_factory.h

+//     std::tuple<storage_gen_t<0>, storage_gen_t<1>, storage_gen_t<2>, storage_gen_t<3>,
+//                storage_gen_t<4>, storage_gen_t<5>, storage_gen_t<6>, storage_gen_t<7>>;
+
+using storage_owner_t =


I had to read through a lot of code to grasp what this "storage owner" is. Consider renaming it to something less generic, e.g. WorkspaceDataStore.

Sure. I'm happy for good type name suggestions. :)

I went with tensor_data_store_t as it is more about covering the TensorNodes in graph. Also I want to differentiate from workspace_store_t.

mzient · 2019-02-14T15:45:57Z

dali/pipeline/workspace/workspace_data_factory.h

+    DALI_ENFORCE(device == DALITensorDevice::CPU, "Only CPU outputs allowed");
+    // Allocate `batch_size` Tensors for this ops
+    // results and add them to the workspace.
+    storage_gen_t<GetStorageIndex(DALIOpType::CPU, device)> output(batch_size, nullptr);


This deserves an alias:

template <DALIOpType op_type, DALITensorDevice device> using WorkspaceStorage = storage_gen_t<GetStorageIndex(op_type, device)>;

mzient · 2019-03-07T18:38:34Z

dali/pipeline/graph/op_graph.h

+
+  std::vector<OpNode> op_nodes_;
+  std::vector<TensorNode> tensor_nodes_;
+  std::vector<std::vector<OpNodeId>> op_paritions_;


Suggested change

std::vector<std::vector<OpNodeId>> op_paritions_;

std::vector<std::vector<OpNodeId>> op_partitions_;

Amazing, what code completion can propagate...

mzient · 2019-03-07T18:42:33Z

dali/pipeline/graph/graph_descr.cc

+void CheckOpConstraints(const OpSpec &spec) {
+  const OpSchema &schema = SchemaRegistry::GetSchema(spec.name());
+
+  bool allows_multiple_inputs = schema.AllowsMultipleInputSets();


Suggested change

bool allows_multiple_inputs = schema.AllowsMultipleInputSets();

bool allows_multiple_input_sets = schema.AllowsMultipleInputSets();

klecki · 2019-03-07T23:15:35Z

dali/pipeline/graph/op_graph_verifier.cc

@@ -0,0 +1,150 @@
+// Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.


TODO: Format this document after changing DALIOpType to OpType.

klecki · 2019-03-08T16:05:17Z

Build 665966

jantonguirao · 2019-03-08T16:39:57Z

dali/pipeline/executor/executor.h

 #include "dali/pipeline/util/event_pool.h"
 #include "dali/pipeline/util/stream_pool.h"
 #include "dali/pipeline/util/thread_pool.h"

+


no need for those two empty lines

jantonguirao · 2019-03-08T16:40:45Z

dali/pipeline/executor/executor.h

-      mixed_op_data.clear();
-      gpu_op_data.clear();
-      support_op_data.clear();
+      std::get<0>(op_data).clear();


so maybe good idea to add it here before merging

jantonguirao · 2019-03-08T16:44:06Z

dali/pipeline/executor/executor.h

+
+template <>
+inline void Executor::SetupStreamsAndEvents<OpType::MIXED>(MixedWorkspace &ws,
+                                                               const OpGraph &graph,


jantonguirao · 2019-03-08T16:44:17Z

dali/pipeline/executor/executor.h

+
+template <>
+inline void Executor::SetupStreamsAndEvents<OpType::GPU>(DeviceWorkspace &ws,
+                                                             const OpGraph &graph,


jantonguirao · 2019-03-08T16:47:31Z

dali/pipeline/graph/op_graph_storage.cc

+namespace dali {
+
+std::vector<tensor_data_store_t> CreateBackingStorageForTensorNodes(const OpGraph &op_graph,
+                                                                int batch_size) {


jantonguirao · 2019-03-08T16:47:50Z

dali/pipeline/graph/op_graph_storage.h

+namespace dali {
+
+std::vector<tensor_data_store_t> CreateBackingStorageForTensorNodes(const OpGraph &op_graph,
+                                                                int batch_size);


jantonguirao · 2019-03-08T16:50:07Z

dali/pipeline/graph/op_graph_verifier.cc

+void CheckArgumentInputConstraints(const OpGraph& op_graph, const OpNode& op) {
+  static const auto allows_argument_input = ArgumentInputConstraints();
+  bool arg_in_allowed = allows_argument_input[static_cast<int>(op.op_type)];
+  if (!arg_in_allowed) {


make sure that arg inputs are allowed for mixed ops as well

I will do so in a followup PR, do not want to introduce changes that can break something unless it is to fix a bug.

klecki · 2019-03-08T21:22:41Z

Build: 666123 (it's stuck due to not enough free workers).

Check some constraints on OpGraph separately from Executor processing the OpGraph Add static traits for OpGraph constraints Unify OpNodes processing for different OpType Executor "owns" buffer for corresponding TensorNodes, using data factory and related types Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2019-03-11T11:47:36Z

Build: 670584

Check some constraints on OpGraph separately from Executor processing the OpGraph Add static traits for OpGraph constraints Unify OpNodes processing for different OpType Executor "owns" buffer for corresponding TensorNodes, using data factory and related types Allow for Argument Inputs in Mixed Ops Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

mzient reviewed Feb 14, 2019

View reviewed changes

jantonguirao reviewed Feb 14, 2019

View reviewed changes

mzient reviewed Feb 14, 2019

View reviewed changes

klecki force-pushed the executor-refactor branch 2 times, most recently from 5d7f9cd to 1196ec0 Compare February 18, 2019 17:28

klecki mentioned this pull request Feb 18, 2019

Store Queues of Buffers for corresponding TensorNodes #551

Merged

klecki force-pushed the executor-refactor branch from 1196ec0 to a0c931c Compare March 5, 2019 19:04

klecki changed the title ~~[WIP] Refactor Executor and OpGraph~~ Refactor Executor and OpGraph Mar 5, 2019

klecki force-pushed the executor-refactor branch 5 times, most recently from b7bc3c4 to f3f022f Compare March 6, 2019 18:57

This was referenced Mar 7, 2019

Add WorkspaceDataFactory with traits for Tuples and WS #602

Merged

Generalize Executor Test #609

Merged

Separate Executor Queues - Generalize Executors #577

Merged

mzient reviewed Mar 7, 2019

View reviewed changes

klecki force-pushed the executor-refactor branch 2 times, most recently from fe3b08d to c99e211 Compare March 7, 2019 21:10

klecki commented Mar 7, 2019

View reviewed changes

klecki force-pushed the executor-refactor branch 2 times, most recently from f4ca954 to c059e65 Compare March 8, 2019 14:39

klecki force-pushed the executor-refactor branch from c059e65 to f6c77c6 Compare March 8, 2019 16:32

jantonguirao approved these changes Mar 8, 2019

View reviewed changes

awolant approved these changes Mar 8, 2019

View reviewed changes

klecki force-pushed the executor-refactor branch from f6c77c6 to e8bdb49 Compare March 8, 2019 17:29

JanuszL added this to the Release_0.8.0 milestone Mar 11, 2019

klecki added 2 commits March 11, 2019 12:06

Allow for Argument Inputs in Mixed ops

b83c0cd

klecki force-pushed the executor-refactor branch from e8bdb49 to b83c0cd Compare March 11, 2019 11:09

klecki merged commit 58dceff into NVIDIA:master Mar 11, 2019

	std::vector<std::vector<OpNodeId>> op_paritions_;
	std::vector<std::vector<OpNodeId>> op_partitions_;

	bool allows_multiple_inputs = schema.AllowsMultipleInputSets();
	bool allows_multiple_input_sets = schema.AllowsMultipleInputSets();

		@@ -0,0 +1,150 @@
		// Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.

Refactor Executor and OpGraph #540

Refactor Executor and OpGraph #540

Conversation

klecki commented Feb 14, 2019 • 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

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

klecki commented Mar 8, 2019

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

klecki Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

klecki commented Mar 8, 2019

klecki commented Mar 11, 2019

klecki commented Feb 14, 2019 •

edited

Loading

klecki Mar 8, 2019 •

edited

Loading