Extend shape support in DALI Dataset for TF #1723

klecki · 2020-02-07T19:30:16Z

Signed-off-by: Krzysztof Lecki klecki@nvidia.com

Why we need this PR?

Improve the DALIDataset for TF.

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

What solution was applied:
[
Replace ShapeFn with ScalarShape (as do other datasets).
Implement matching between user set shape and
the one returned by DALI in the C++ api of Dataset.
]
Affected modules and functionalities:
[ DALIDataset for TF ]
Key points relevant for the review:
[
Can the API be changed. I can revert or add a wrapper.
]
Validation and testing:
[ Test was added, some additional local test. ]
Documentation (including examples):
[ Docs were adjusted, now it matches the from_generator approach more. ]

JIRA TASK: [DALI-1259]

klecki · 2020-02-07T19:30:49Z

!build

dali-automaton · 2020-02-07T19:35:26Z

CI MESSAGE: [1115663]: BUILD STARTED

dali-automaton · 2020-02-07T19:50:50Z

CI MESSAGE: [1115663]: BUILD FAILED

JanuszL · 2020-02-11T01:09:37Z

.clang-format

@@ -94,10 +94,6 @@ PenaltyBreakString: 1000
 PenaltyExcessCharacter: 1000000
 PenaltyReturnTypeOnItsOwnLine: 200
 PointerAlignment: Left
-RawStringFormats:


They changed something in clang 9 and I hoped to sneak it in.

JanuszL · 2020-02-11T11:47:37Z

dali_tf_plugin/dali_shape_helper.h

+
+#include "tensorflow/core/framework/tensor_shape.h"
+
+// namespace dali-tf-impl {


Cleaned it up.

dali_tf_plugin/dali_dataset_op.cc

JanuszL · 2020-02-11T11:49:44Z

dali_tf_plugin/dali_dataset_op.cc

@@ -227,6 +229,8 @@ class DALIDatasetOp : public DatasetOpKernel {

      Status GetNextInternal(IteratorContext *context, std::vector<Tensor> *out_tensors,
                             bool *end_of_sequence) override {
+        // static int called_times = 0;


Debug leftover

dali/python/nvidia/dali/plugin/tf.py

JanuszL · 2020-02-11T13:46:40Z

dali/python/nvidia/dali/plugin/tf.py

+      else:
+        return True
+
+    def _handle_deprecation(self, tuple_arg, list_arg, arg_name):


JanuszL · 2020-02-11T13:49:29Z

dali/test/python/test_dali_tf_dataset_mnist.py

-        (BATCH_SIZE)]
-    dtypes = [
+    shapes = (
+        (64, 24, 24),


So you can remove IMAGE_SIZE, but I would stick to usage of BATCH_SIZE here as it is used in other places and needs to match.

This is leftover from testing, will revert.

JanuszL · 2020-02-11T13:53:50Z

dali/test/python/test_dali_tf_dataset_shape.py

+file_list_path = os.path.join(data_path, 'image_list.txt')
+
+def dali_pipe_batch_1(shapes, types, as_single_tuple = False):
+    target_sizes = [(853, 1280, 3),


I don't like this strong bound between the content of DALI extra and test itself.
I would run a standalone DALI pipeline next to it and get the actual dimension from it and then compare with one returned by DALIDataset.
Otherwise any change to 'db/single/jpeg/' will break this test.

awolant

Maybe it would be good to apply the same shape inference rules to daliop? If so, probably in the second PR though.

JanuszL · 2020-02-12T19:49:07Z

Maybe it would be good to apply the same shape inference rules to daliop? If so, probably in the second PR though.

Cannot agree more.

Replace ShapeFn with ScalarShape (as do other datasets). Implement matching between user set shape and the one returned by DALI. Adjust docs. Adjust order of arguments and naming to fith with from_generator dataset arguments in TF. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2020-02-24T15:41:49Z

!build

dali-automaton · 2020-02-24T15:45:44Z

CI MESSAGE: [1147110]: BUILD STARTED

lgtm-com · 2020-02-24T15:49:08Z

This pull request fixes 1 alert when merging c61cd08 into 9e08b4d - view on LGTM.com

fixed alerts:

1 for Asserting a tuple

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2020-02-24T15:58:52Z

!build

dali-automaton · 2020-02-24T16:00:27Z

CI MESSAGE: [1147136]: BUILD STARTED

lgtm-com · 2020-02-24T16:07:17Z

This pull request fixes 1 alert when merging 6a8b2d8 into 9e08b4d - view on LGTM.com

fixed alerts:

1 for Asserting a tuple

dali-automaton · 2020-02-24T19:33:52Z

CI MESSAGE: [1147136]: BUILD PASSED

klecki · 2020-02-26T16:06:41Z

!build

dali-automaton · 2020-02-26T16:10:31Z

CI MESSAGE: [1152603]: BUILD STARTED

lgtm-com · 2020-02-26T16:15:40Z

This pull request fixes 1 alert when merging 785e4d0 into e5ea7ef - view on LGTM.com

fixed alerts:

1 for Asserting a tuple

dali-automaton · 2020-02-26T16:56:30Z

CI MESSAGE: [1152603]: BUILD FAILED

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2020-02-27T11:35:52Z

!build

dali-automaton · 2020-02-27T11:40:34Z

CI MESSAGE: [1154903]: BUILD STARTED

lgtm-com · 2020-02-27T11:44:42Z

This pull request fixes 1 alert when merging 0a2e14e into 3b2b02f - view on LGTM.com

fixed alerts:

1 for Asserting a tuple

jantonguirao · 2020-02-27T15:00:48Z

dali/c_api/c_api.h

+   * stored at position `n` in the pipeline.
+   * This function may only be called after
+   * calling Output function.
+   * @remarks Same as calling daliShapeAt(pipe_handle, n)


what do you mean with same as calling... ?

That it will give you the same result, because samples have the same type as TensorList

Eh. It was supposed to by daliTypeAt. I'm blind.
Janusz wants to leave only one.

Just an ask, not a strong blocker if you think that this make API consistent.

jantonguirao · 2020-02-27T15:02:38Z

dali/python/nvidia/dali/plugin/tf.py

+        return False
+
+    def _handle_deprecation(self, supported_arg, deprecated_arg, name):
+      if deprecated_arg is not None:


Suggested change

if deprecated_arg is not None:

if deprecated_arg is not None and supported_arg is not None:

raise ...

dali/c_api/c_api.cc

JanuszL · 2020-02-27T15:17:51Z

dali/c_api/c_api.cc

+}
+
+dali_data_type_t daliTypeAtSample(daliPipelineHandle* pipe_handle, int n) {
+  return daliTypeAt(pipe_handle, n);


What is the value in having daliTypeAt?

To be able to access the type. And for the API to be complete

But it is only passing through arguments to daliTypeAtSample. So I don't see much value in having two functions.

Damn, forgot to remove it from here.

dali/c_api/c_api.h

jantonguirao · 2020-02-27T15:03:16Z

dali/python/nvidia/dali/plugin/tf.py

+        return False
+
+    def _handle_deprecation(self, supported_arg, deprecated_arg, name):
+      if deprecated_arg is not None:


Suggested change

if deprecated_arg is not None:

if deprecated_arg is not None and supported_arg is not None:

raise ...

jantonguirao · 2020-02-27T15:03:43Z

dali/python/nvidia/dali/plugin/tf.py

+        return False
+
+    def _handle_deprecation(self, supported_arg, deprecated_arg, name):
+      if deprecated_arg is not None:


Suggested change

if deprecated_arg is not None:

if deprecated_arg is not None and supported_arg is not None:

raise ...

if deprecated_arg is not None and supported_arg is not None: raise ... elif deprecated_arg is not None: warn ...

I'm not sure that it's any better.

jantonguirao · 2020-02-27T15:19:17Z

dali_tf_plugin/dali_dataset_op.cc

-    }
-    return Status::OK();
-  })
+  .SetShapeFn(shape_inference::ScalarShape)


a comment here would help to understand (why are we setting to ScalarShape?)

Because all the TF datasets are doing the same. I can add a comment, it's because the dataset is basically returning a opaque pointer to a Tensor instead of the Tensor itself.

jantonguirao · 2020-02-27T15:20:37Z

dali_tf_plugin/dali_dataset_op.cc

+        for (int i = 0; i < required_shape.dims(); i++) {
+          result.AddDim(0);
+        }
+        // Non-trivial batch size case, diffrent dims


Suggested change

// Non-trivial batch size case, diffrent dims

// Non-trivial batch size case, different dims

jantonguirao · 2020-02-27T15:23:12Z

dali_tf_plugin/dali_dataset_op.cc

+          }
+        }
+        int matches = CountShapeMatches(result, required_shape, dali_shape);
+        if (matches != 1) {


Suggested change

if (matches != 1) {

if (matches > 1) {

don't you mean that?

When there is 0 matches we still don't have a valid match.

jantonguirao · 2020-02-27T15:24:11Z

dali_tf_plugin/dali_dataset_op.cc

+
+
+      bool DimSizeMatch(int64_t required, int64_t dali) {
+        if (required < 0 || required == dali) {


so basically:

return required < 0 || required == dali;

I keep doing that. Done

jantonguirao · 2020-02-27T15:35:30Z

dali_tf_plugin/dali_dataset_op.cc

+        return false;
+      }
+
+      int CountShapeMatches(TensorShape &result, const PartialTensorShape &required_shape, const TensorShape &dali_shape,


seems convoluted enough that I'd like to see a bunch of unit tests covering this function

I tried covering most of the cases with dataset test itself. We don't have any non-python unit tests for TF plugin and if you want to add something that functionality is out of the scope for this PR - we probably would need to adjust the build system and the test jobs for this.

jantonguirao · 2020-02-27T15:37:53Z

dali_tf_plugin/dali_dataset_op.cc

+        return false;
+      }
+
+      int CountShapeMatches(TensorShape &result, const PartialTensorShape &required_shape, const TensorShape &dali_shape,


The name is a bit misleading, you are calculating a tensor shape, not only counting matches

dali-automaton · 2020-02-27T16:32:56Z

CI MESSAGE: [1154903]: BUILD FAILED

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2020-02-27T19:52:15Z

!build

dali-automaton · 2020-02-27T19:55:52Z

CI MESSAGE: [1155927]: BUILD STARTED

lgtm-com · 2020-02-27T20:00:09Z

This pull request fixes 1 alert when merging c2c5043 into b25864e - view on LGTM.com

fixed alerts:

1 for Asserting a tuple

dali-automaton · 2020-02-28T03:00:13Z

CI MESSAGE: [1155927]: BUILD PASSED

klecki · 2020-02-28T12:11:27Z

!build

dali-automaton · 2020-02-28T12:15:31Z

CI MESSAGE: [1158153]: BUILD STARTED

lgtm-com · 2020-02-28T12:18:20Z

This pull request fixes 1 alert when merging 7983f23 into b48e757 - view on LGTM.com

fixed alerts:

1 for Asserting a tuple

dali-automaton · 2020-02-29T00:53:50Z

CI MESSAGE: [1158153]: BUILD PASSED

klecki requested review from awolant, JanuszL and jantonguirao February 7, 2020 19:30

JanuszL reviewed Feb 11, 2020

View reviewed changes

dali_tf_plugin/dali_dataset_op.cc Show resolved Hide resolved

JanuszL reviewed Feb 11, 2020

View reviewed changes

dali/python/nvidia/dali/plugin/tf.py Outdated Show resolved Hide resolved

JanuszL reviewed Feb 11, 2020

View reviewed changes

awolant reviewed Feb 12, 2020

View reviewed changes

klecki added 2 commits February 24, 2020 13:25

Review fixes

c61cd08

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki force-pushed the dataset-shape branch from 902718f to c61cd08 Compare February 24, 2020 15:41

klecki mentioned this pull request Feb 24, 2020

Return images with different sizes from TF plugin #1690

Closed

adjust a comment

6a8b2d8

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki force-pushed the dataset-shape branch from 35ce032 to 6a8b2d8 Compare February 24, 2020 15:58

NVIDIA deleted a comment from dali-automaton Feb 24, 2020

Add skipping incompatible tests

0a2e14e

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

jantonguirao reviewed Feb 27, 2020

View reviewed changes

JanuszL reviewed Feb 27, 2020

View reviewed changes

dali/c_api/c_api.cc Show resolved Hide resolved

JanuszL reviewed Feb 27, 2020

View reviewed changes

dali/c_api/c_api.h Show resolved Hide resolved

jantonguirao reviewed Feb 27, 2020

View reviewed changes

Some review fixes

c2c5043

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Remove leftover

7983f23

JanuszL approved these changes Feb 28, 2020

View reviewed changes

jantonguirao approved these changes Mar 5, 2020

View reviewed changes

klecki merged commit 58a8fd8 into NVIDIA:master Mar 5, 2020


		#include "tensorflow/core/framework/tensor_shape.h"

		// namespace dali-tf-impl {

	if deprecated_arg is not None:
	if deprecated_arg is not None and supported_arg is not None:
	raise ...

	// Non-trivial batch size case, diffrent dims
	// Non-trivial batch size case, different dims



		bool DimSizeMatch(int64_t required, int64_t dali) {
		if (required < 0 \|\| required == dali) {

Extend shape support in DALI Dataset for TF #1723

Extend shape support in DALI Dataset for TF #1723

Conversation

klecki commented Feb 7, 2020

Why we need this PR?

What happened in this PR?

klecki commented Feb 7, 2020

dali-automaton commented Feb 7, 2020

dali-automaton commented Feb 7, 2020

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

awolant left a comment

Choose a reason for hiding this comment

JanuszL commented Feb 12, 2020

klecki commented Feb 24, 2020

dali-automaton commented Feb 24, 2020

lgtm-com bot commented Feb 24, 2020

klecki commented Feb 24, 2020

dali-automaton commented Feb 24, 2020

lgtm-com bot commented Feb 24, 2020

dali-automaton commented Feb 24, 2020

klecki commented Feb 26, 2020

dali-automaton commented Feb 26, 2020

lgtm-com bot commented Feb 26, 2020

dali-automaton commented Feb 26, 2020

klecki commented Feb 27, 2020

dali-automaton commented Feb 27, 2020

lgtm-com bot commented Feb 27, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Feb 27, 2020

klecki commented Feb 27, 2020

dali-automaton commented Feb 27, 2020

lgtm-com bot commented Feb 27, 2020

dali-automaton commented Feb 28, 2020

klecki commented Feb 28, 2020

dali-automaton commented Feb 28, 2020

lgtm-com bot commented Feb 28, 2020

dali-automaton commented Feb 29, 2020