CPU argument input #1423

mzient · 2019-10-25T09:31:18Z

Why we need this PR?

Refactoring to improve remove the support operators from DALI.
To make DALI simple and more flexible the current Support operators can be reworked to regular CPU operators. To make this working, the argument inputs had to be reworked to accept CPU operator outputs.

What happened in this PR?

Graph verification had to be modified to allow CPU operators to be plugged into the input arguments.
Argument input is now a TensorVector not a TensorList. This change was made to be consistent with the type of CPU operators outputs.
Existing operators that use argument input had to be reworked to conform to the new api.
Existing support operators (CoinFlip, Uniform) were changed to CPU operators.
Some mechanical changes in the tests had to be done.
Total removal of SupportBackend, SupportWorkspace, etc
When using separated queue policy, a MakeContiguous node is inserted before passing ArgumentInputs to GPU ops.
TensorVector can now be created as a "view" at a TensorList, with ability to be updated afterwards to track changes to the underlying TensorList.

There will be a follow-up PR that removes any notion of support operators from DALI code and docs.

JIRA TASK: [DALI-687]

mzient · 2019-10-25T09:42:40Z

!build

* Change type of existing support operators * Remove support device from tests * Review fixes Author: Rafal Banas <Banas.Rafal97@gmail.com> Signed-off-by: Rafal Banas <Banas.Rafal97@gmail.com>

… ArgumentInputs of GPU operators. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

- Update TensorVectors in ArgumentWorkspace when created from TensorLists. - Make TensorVector `UpdateViews` a public function. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

dali-automaton · 2019-10-25T10:13:47Z

CI MESSAGE: [961040]: BUILD FAILED

* Rename AcquireTensorArgument to GetPerSampleArgument * Make GetPerSampleArgument work with TensorVector Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

dali-automaton · 2019-10-25T12:10:38Z

CI MESSAGE: [961155]: BUILD STARTED

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

klecki · 2019-10-25T12:36:11Z

dali/pipeline/data/tensor_test.cc

@@ -368,11 +368,11 @@ TYPED_TEST(TensorTest, TestShareData) {
 }

 TYPED_TEST(TensorTest, TestCopyToTensorList) {
-  std::vector<Tensor<TypeParam>> tensors(16);
+  TensorVector<CPUBackend> tensors(16);


Why change from parametrized backend to only CPUBackend here?
Also, as you changed the std::vector to TensorVector it would be good to test both modes of TensorVector, the contiguous (backed by TL) and noncontiguous (separate Tensors).

Reverting to TypeParam.
Testing contiguous copy seems a bit excessive when there's no special code path for such case. When we added, it's going to be useful, though.

…ces to "support" device. Restore backend parameter in TestCopyToTensorList. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

dali-automaton · 2019-10-25T12:55:07Z

CI MESSAGE: [961194]: BUILD STARTED

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

dali-automaton · 2019-10-25T13:26:41Z

CI MESSAGE: [961232]: BUILD STARTED

klecki · 2019-10-25T13:29:07Z

dali/pipeline/operator/operator.h

+        bool is_valid_shape = shape.tensor_shape(0) == TensorShape<1>{batch_size_};
+
+        DALI_ENFORCE(is_valid_shape,
+          make_string("`", argument_name, "` must be a 1xN or Nx1 (N = ", batch_size_,


Nitpick, the make_string adds spaces automatically, so you will get (N_=__10) etc

If that's the only thing, then I'll do it in another PR.

dali/operators/color/brightness_contrast.h

JanuszL · 2019-10-25T14:18:46Z

dali/pipeline/executor/workspace_policy.h

-    auto tensor = queue[idxs[OpType::SUPPORT]];
-    ws.AddArgumentInput(tensor, arg_pair.first);
+    auto &parent_node = graph.Node(graph.Tensor(tid).producer.node);
+    auto parent_op_type = parent_node.op_type;


Suggested change

auto parent_op_type = parent_node.op_type;

auto parent_op_idx = idxs[parent_node.op_type];

JanuszL · 2019-10-25T14:19:02Z

dali/pipeline/executor/workspace_policy.h

+      "Argument Inputs must be stored in CPU memory");
+
+    auto add_arg_input = [&](auto &queue) {
+      auto tensor = queue[idxs[parent_op_type]];


Suggested change

auto tensor = queue[idxs[parent_op_type]];

auto tensor = queue[parent_op_idx];

klecki · 2019-10-25T14:31:08Z

dali/pipeline/workspace/workspace.h

  }

 protected:
+  struct ArgumentInputDesc {
+    shared_ptr<TensorVector<CPUBackend>> tvec;
+    bool should_update = false;


Can you add a comment describing what and why should be updated?

dali-automaton · 2019-10-26T02:41:14Z

CI MESSAGE: [961232]: BUILD PASSED

* Make input arguments accept TensorVector * Change type of existing support operators * Remove support device from tests * Remove SupportWorkspace and SupportBackend. Insert MakeContiguous for ArgumentInputs of GPU operators. * Make TensorVector track changes in underlying TensorList. * Update TensorVectors in ArgumentWorkspace when created from TensorLists. * Make TensorVector `UpdateViews` a public function. * Deprecate \"support\" device. Co-authored-by: Rafal Banas <Banas.Rafal97@gmail.com> Signed-off-by: Rafal Banas <Banas.Rafal97@gmail.com> Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient requested review from jantonguirao, szalpal, klecki, banasraf, JanuszL and awolant October 25, 2019 09:31

mzient force-pushed the cpu-argument-input branch 3 times, most recently from 5592773 to ebc0e3e Compare October 25, 2019 09:42

banasraf and others added 4 commits October 25, 2019 11:56

* Make input arguments accept TensorVector

0849b3d

* Change type of existing support operators * Remove support device from tests * Review fixes Author: Rafal Banas <Banas.Rafal97@gmail.com> Signed-off-by: Rafal Banas <Banas.Rafal97@gmail.com>

Remove SupportWorkspace and SupportBackend. Insert MakeContiguous for…

9038b06

… ArgumentInputs of GPU operators. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Fix TensorVector for TensorList constructor.

c48430c

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

- Make TensorVector track changes in underlying TensorList.

e94d68c

- Update TensorVectors in ArgumentWorkspace when created from TensorLists. - Make TensorVector `UpdateViews` a public function. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

* Rebase

5abb306

* Rename AcquireTensorArgument to GetPerSampleArgument * Make GetPerSampleArgument work with TensorVector Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient force-pushed the cpu-argument-input branch from ebc0e3e to 5abb306 Compare October 25, 2019 12:00

mzient changed the title ~~Cpu argument input~~ CPU argument input Oct 25, 2019

Deprecate \"support\" device.

24d1cc1

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

klecki reviewed Oct 25, 2019

View reviewed changes

Replace "support" with "cpu" at AddOperator. Remove all other referen…

70dc7bc

…ces to "support" device. Restore backend parameter in TestCopyToTensorList. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient force-pushed the cpu-argument-input branch from 08d9d19 to 70dc7bc Compare October 25, 2019 12:53

mzient requested a review from klecki October 25, 2019 12:56

Revert changes to RN50 test.

f5f11cd

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

JanuszL requested a review from ptrendx October 25, 2019 13:21

JanuszL mentioned this pull request Oct 25, 2019

Support operator as graph output #213

Closed

klecki reviewed Oct 25, 2019

View reviewed changes

JanuszL mentioned this pull request Oct 25, 2019

Dali0.7 error: TypeError: '<' not supported between instances of 'EdgeReference' and 'float' #493

Closed

JanuszL reviewed Oct 25, 2019

View reviewed changes

dali/operators/color/brightness_contrast.h Show resolved Hide resolved

jantonguirao approved these changes Oct 25, 2019

View reviewed changes

JanuszL reviewed Oct 25, 2019

View reviewed changes

klecki reviewed Oct 25, 2019

View reviewed changes

klecki approved these changes Oct 25, 2019

View reviewed changes

JanuszL approved these changes Oct 25, 2019

View reviewed changes

mzient merged commit 835405b into NVIDIA:master Oct 27, 2019

mzient mentioned this pull request Oct 27, 2019

How to use CoinFlip as input of PythonFunction? #1283

Closed

JanuszL mentioned this pull request Oct 27, 2019

How to return ops.Uniform() as Tensor #1312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPU argument input #1423

CPU argument input #1423

mzient commented Oct 25, 2019

mzient commented Oct 25, 2019

dali-automaton commented Oct 25, 2019

dali-automaton commented Oct 25, 2019

klecki Oct 25, 2019

mzient Oct 25, 2019

dali-automaton commented Oct 25, 2019

dali-automaton commented Oct 25, 2019

klecki Oct 25, 2019 •

edited

Loading

mzient Oct 25, 2019

JanuszL Oct 25, 2019

JanuszL Oct 25, 2019

klecki Oct 25, 2019

dali-automaton commented Oct 26, 2019

	auto parent_op_type = parent_node.op_type;
	auto parent_op_idx = idxs[parent_node.op_type];

	auto tensor = queue[idxs[parent_op_type]];
	auto tensor = queue[parent_op_idx];

CPU argument input #1423

CPU argument input #1423

Conversation

mzient commented Oct 25, 2019

Why we need this PR?

What happened in this PR?

mzient commented Oct 25, 2019

dali-automaton commented Oct 25, 2019

dali-automaton commented Oct 25, 2019

klecki Oct 25, 2019

Choose a reason for hiding this comment

mzient Oct 25, 2019

Choose a reason for hiding this comment

dali-automaton commented Oct 25, 2019

dali-automaton commented Oct 25, 2019

klecki Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

mzient Oct 25, 2019

Choose a reason for hiding this comment

JanuszL Oct 25, 2019

Choose a reason for hiding this comment

JanuszL Oct 25, 2019

Choose a reason for hiding this comment

klecki Oct 25, 2019

Choose a reason for hiding this comment

dali-automaton commented Oct 26, 2019

klecki Oct 25, 2019 •

edited

Loading