Adjust tf code for experimental Dataset with inputs #2993

klecki · 2021-05-26T15:06:43Z

Add placeholder Dataset
Expose it in documentation
Adjust the TF Dataset tests for custom pipeline/dataset
definition.

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

Why we need this PR?

Refactoring as a first step to add inputs to TF dataset

What happened in this PR?

What solution was applied:
Dynamic nvidia.dali.plugin.tf.experimental module added for the placeholder Dataset.
Tests extended to allow for custom pipeline/dataset pair to be specified.
Affected modules and functionalities:
tf api & docs, tf-dataset tests
Key points relevant for the review:
Mostly the tf api "changes"?
Validation and testing:
CI, local testing, 👀 for docs
Documentation (including examples):
Yes

JIRA TASK: [Use DALI-XXXX or NA]

* Add dynamic .experimental module to nvidia.dali.plugin.tf * Add placeholder Dataset * Expose it in documentation * Adjust the TF Dataset tests for custom pipeline/dataset definition. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2021-05-26T15:07:26Z

!build

dali-automaton · 2021-05-26T15:11:33Z

CI MESSAGE: [2410986]: BUILD STARTED

lgtm-com · 2021-05-26T15:20:16Z

This pull request introduces 1 alert when merging 7bd651d into fdf10f6 - view on LGTM.com

new alerts:

1 for Unreachable code

klecki · 2021-05-26T15:29:01Z

This pull request introduces 1 alert when merging 7bd651d into fdf10f6 - view on LGTM.com

new alerts:
* 1 for Unreachable code

The alert reported here is for explicit NotImplementedError for the placeholder Dataset which I will be supporting in the follow-up PRs.

dali-automaton · 2021-05-26T16:28:44Z

CI MESSAGE: [2410986]: BUILD FAILED

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

klecki · 2021-05-26T17:13:36Z

!build

dali-automaton · 2021-05-26T17:16:38Z

CI MESSAGE: [2411439]: BUILD STARTED

lgtm-com · 2021-05-26T17:26:26Z

This pull request introduces 1 alert when merging a79586d into fdf10f6 - view on LGTM.com

new alerts:

1 for Unreachable code

dali-automaton · 2021-05-26T18:56:32Z

CI MESSAGE: [2411439]: BUILD PASSED

jantonguirao · 2021-05-27T10:38:58Z

dali/test/python/test_utils_tensorflow.py

+            if device == 'gpu':
+                shard_outputs.append(
+                    tuple(result.as_cpu().as_array() for result in pipe_outputs))
+            else:
+                shard_outputs.append(
+                    tuple(result.as_array() for result in pipe_outputs))


Suggested change

if device == 'gpu':

shard_outputs.append(

tuple(result.as_cpu().as_array() for result in pipe_outputs))

else:

shard_outputs.append(

tuple(result.as_array() for result in pipe_outputs))

shard_outputs.append(tuple(test_utils.to_array(result) for result in pipe_outputs))

jantonguirao · 2021-05-27T10:51:53Z

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

+
+Each of the input datasets must be mapped to a :meth:`~nvidia.dali.fn.external_source` operator
+that will represent the input to the DALI pipeline. The input is represented by the ``name``
+parameter of :meth:`~nvidia.dali.fn.external_source` and needs to be provided for corresponding


Suggested change

parameter of :meth:`~nvidia.dali.fn.external_source` and needs to be provided for corresponding

parameter of :meth:`~nvidia.dali.fn.external_source` and needs to be provided to the corresponding

jantonguirao · 2021-05-27T10:52:27Z

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

+
+
+.. warning::
+    This version of the class is just API placeholder and the functionality is not yet implemented.


Suggested change

This version of the class is just API placeholder and the functionality is not yet implemented.

This version of the class is just an API placeholder and the functionality is not yet implemented.

jantonguirao · 2021-05-27T10:53:21Z

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

+    This version of the class is just API placeholder and the functionality is not yet implemented.
+
+The operator adds additional parameters to the ones supported by the
+:class:`~nvidia.dali.plugin.tf.DALIDataset`:


I think we should use double back quotes (``). It is used earlier in the string but starting from here I see only single back quotes

This is a different use - it is to insert a link to the DALIDataset, so it has to be single AFAIK.

As for the Parameters below, I kept it uniform with the current Dataset doc, we could skip them completely there, but I think it should be just one PR cleaning our docs (we have a mix of those).

I can wrap in double backticks the names used in the text of the doc. 👍

jantonguirao · 2021-05-27T10:55:03Z

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

+        as well to provide the mapping of input datasets to `External Source` nodes.
+    `input_names` : `str` or `tuple` of `str`, optional, default = None
+        names of inputs to the DALI Pipeline. Must match arity of the `input_datasets`.
+        `input_datasets[i]` will be provided to `External Source` of the name `input_names[i]`.


Suggested change

`input_datasets[i]` will be provided to `External Source` of the name `input_names[i]`.

`input_datasets[i]` will be provided to the external source instance with the name `input_names[i]`.

jantonguirao · 2021-05-27T10:57:06Z

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

+        Sample mode means that every input dataset is treated as if providing individual samples.
+        DALIDataset will query the inputs dataset `batch_size`-times to build a batch that would
+        be fed into the DALI Pipeline.
+        In sample mode, each sample produced by the input dataset can have different shape,


Suggested change

In sample mode, each sample produced by the input dataset can have different shape,

In sample mode, each sample produced by the input dataset can have a different shape,

jantonguirao · 2021-05-27T10:57:18Z

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

+        DALIDataset will query the inputs dataset `batch_size`-times to build a batch that would
+        be fed into the DALI Pipeline.
+        In sample mode, each sample produced by the input dataset can have different shape,
+        but not number of dimensions.


Suggested change

but not number of dimensions.

but not a different number of dimensions.

jantonguirao · 2021-05-27T10:58:42Z

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

+        class DALIDatasetWithInputs(dataset_ops._OptionsDataset):
+            # @functools.wraps(_DALIDatasetV2.__init__)
+            def __init__(self, pipeline, **kwargs):
+                raise NotImplementedError("DALIDatasetWithInputs is only a placeholder operator.")


Suggested change

raise NotImplementedError("DALIDatasetWithInputs is only a placeholder operator.")

raise NotImplementedError("DALIDatasetWithInputs is not implemented.")

just a suggestion

The error type is already NotImplemented, I would keep some explanation, it will be removed soon. (I hope).

jantonguirao · 2021-05-27T11:01:32Z

dali/test/python/test_utils_tensorflow.py

+# Respective signatures:
+# get_pipeline_desc(batch_size, num_threads, device, device_id, shard_id, num_shards,
+#                  def_for_dataset) -> nvidia.dali.pipeline, shapes, dtypes
+# `def_for_dataset` - indicates if this function is invoked to create a basaline standalone


Suggested change

# `def_for_dataset` - indicates if this function is invoked to create a basaline standalone

# `def_for_dataset` - indicates if this function is invoked to create a baseline standalone

jantonguirao · 2021-05-27T11:08:20Z

dali/test/python/test_utils_tensorflow.py

+# ################################################################################################ #
+
+def get_image_pipeline(batch_size, num_threads, device, device_id=0, shard_id=0, num_shards=1,
+        def_for_dataset=False):


It's hard for me to grasp the meaning of def_for_dataset without an implementation.

I found a need to have a slightly different definition for a pipeline that is run standalone vs the one used in TF (namely the need to do:

fn.external_source(name="...") # tf-wrapped one fn.external_source(source=...) # DALI-only one

I could probably introduce explicit feed-inputs step as well, but this is easier and probably more generic.

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

lgtm-com · 2021-05-28T10:08:29Z

This pull request introduces 1 alert when merging d0fa545 into 4f4bf48 - view on LGTM.com

new alerts:

1 for Unreachable code

klecki · 2021-05-28T14:24:49Z

!build

dali-automaton · 2021-05-28T14:31:50Z

CI MESSAGE: [2419541]: BUILD STARTED

dali-automaton · 2021-05-28T15:44:56Z

CI MESSAGE: [2419541]: BUILD FAILED

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

klecki · 2021-05-28T17:40:31Z

!build

dali-automaton · 2021-05-28T17:48:02Z

CI MESSAGE: [2420154]: BUILD STARTED

lgtm-com · 2021-05-28T17:54:07Z

This pull request introduces 1 alert when merging e413353 into 30e9705 - view on LGTM.com

new alerts:

1 for Unreachable code

dali-automaton · 2021-05-28T19:28:30Z

CI MESSAGE: [2420154]: BUILD FAILED

dali-automaton · 2021-05-31T09:08:12Z

CI MESSAGE: [2420154]: BUILD PASSED

Fix function rename

a79586d

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

jantonguirao assigned awolant and jantonguirao May 27, 2021

jantonguirao reviewed May 27, 2021

View reviewed changes

klecki added 2 commits May 28, 2021 11:46

Review part 1

9488770

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

Review part 2

d0fa545

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

awolant approved these changes May 28, 2021

View reviewed changes

klecki mentioned this pull request May 28, 2021

Add experimental input support to TF DALIDataset #2997

Merged

3 tasks

klecki commented May 28, 2021

View reviewed changes

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

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

e413353

jantonguirao approved these changes May 31, 2021

View reviewed changes

klecki merged commit 2006ae8 into NVIDIA:main May 31, 2021

	parameter of :meth:`~nvidia.dali.fn.external_source` and needs to be provided for corresponding
	parameter of :meth:`~nvidia.dali.fn.external_source` and needs to be provided to the corresponding



		.. warning::
		This version of the class is just API placeholder and the functionality is not yet implemented.

	This version of the class is just API placeholder and the functionality is not yet implemented.
	This version of the class is just an API placeholder and the functionality is not yet implemented.

	`input_datasets[i]` will be provided to `External Source` of the name `input_names[i]`.
	`input_datasets[i]` will be provided to the external source instance with the name `input_names[i]`.

	In sample mode, each sample produced by the input dataset can have different shape,
	In sample mode, each sample produced by the input dataset can have a different shape,

	but not number of dimensions.
	but not a different number of dimensions.

	raise NotImplementedError("DALIDatasetWithInputs is only a placeholder operator.")
	raise NotImplementedError("DALIDatasetWithInputs is not implemented.")

	# `def_for_dataset` - indicates if this function is invoked to create a basaline standalone
	# `def_for_dataset` - indicates if this function is invoked to create a baseline standalone

Adjust tf code for experimental Dataset with inputs #2993

Adjust tf code for experimental Dataset with inputs #2993

Conversation

klecki commented May 26, 2021

Why we need this PR?

What happened in this PR?

klecki commented May 26, 2021

dali-automaton commented May 26, 2021

lgtm-com bot commented May 26, 2021

klecki commented May 26, 2021

dali-automaton commented May 26, 2021

klecki commented May 26, 2021

dali-automaton commented May 26, 2021

lgtm-com bot commented May 26, 2021

dali-automaton commented May 26, 2021

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

lgtm-com bot commented May 28, 2021

klecki commented May 28, 2021

dali-automaton commented May 28, 2021

dali-automaton commented May 28, 2021

klecki commented May 28, 2021

dali-automaton commented May 28, 2021

lgtm-com bot commented May 28, 2021

dali-automaton commented May 28, 2021

dali-automaton commented May 31, 2021