Generalized handling of operator arguments. #2393

mzient · 2020-10-23T18:02:00Z

Signed-off-by: Michał Zientkiewicz mzient@gmail.com

Why we need this PR?

Pick one, remove the rest

It adds new feature needed because it makes passing non-scalar constants to argument inputs easier

What happened in this PR?

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

What solution was applied:
- Try to call Constant on unrecognized arguments in operator's __call__
- Add more elaborate distinction between call and init arguments in fn wrappers.
- Handle numpy scalars in types.Constant
- Promote non-data-nodes to constant nodes on pipeline output
- Add a function to separate keyword arguments to call/init args, use it in __init__, __call__ and fn wrappers.
- Add Compose function :)
Affected modules and functionalities:
- ops, types, fn, pipeline
Key points relevant for the review:
- N/A
Validation and testing:
- Unit test in python + existing tests treated as regression test
Documentation (including examples):
- N/A

JIRA TASK: N/A

mzient · 2020-10-23T18:02:14Z

!build

dali-automaton · 2020-10-23T18:05:57Z

CI MESSAGE: [1729172]: BUILD STARTED

dali-automaton · 2020-10-23T19:39:51Z

CI MESSAGE: [1729172]: BUILD FAILED

mzient · 2020-10-24T12:56:06Z

!build

dali-automaton · 2020-10-24T13:00:32Z

CI MESSAGE: [1731279]: BUILD STARTED

dali-automaton · 2020-10-24T14:24:51Z

CI MESSAGE: [1731279]: BUILD PASSED

jantonguirao · 2020-10-26T09:54:36Z

dali/python/nvidia/dali/fn.py

+                return True
+            if value is None:
+                return False
+            if isinstance(value, (bool, int, float, str, list, tuple, nvidia.dali.types.ScalarConstant)):


are this all the types we need to check? I am thinking of things like np.int64, etc

Good point.

jantonguirao · 2020-10-26T10:00:13Z

dali/python/nvidia/dali/types.py

+        if value_dtype is not None:
+            dali_type = to_dali_type(value.dtype)
+            if dali_type in _int_types:
+                value = int(value)


is this int32 or int64? are we OK with changing types?

Python's int is 64-bit, so it should cover everything except UINT64 values above 2^63 - I think we can live with it.

JanuszL · 2020-10-26T10:15:48Z

dali/test/python/test_pipeline.py

@@ -1707,3 +1707,12 @@ def get_output():
    out = get_output()[0].at(0)
    assert out[0] == -0.5 and out[1] == 1.25

+def test_return_constants():
+    pipe = dali.pipeline.Pipeline(1, 1, 0)


Nitpick, no GPU is used in the test anyway:

Suggested change

pipe = dali.pipeline.Pipeline(1, 1, 0)

pipe = dali.pipeline.Pipeline(1, 1, None)

JanuszL · 2020-10-26T10:16:38Z

dali/test/python/test_pipeline.py

+    assert np.array_equal(a.at(0), np.array([[1,2],[3,4]]))
+    assert b.at(0) == 10
+    assert c.at(0) == 15
+    assert c.at(0).dtype == np.uint8


Can you check other types as well?

I think so.

JanuszL · 2020-10-26T10:19:45Z

dali/python/nvidia/dali/types.py

 def _is_mxnet_array(value):
    return 'mxnet.ndarray.ndarray.NDArray' in str(type(value))

 def _is_torch_tensor(value):
    return 'torch.Tensor' in str(type(value))

 def _is_numpy_array(value):
-    return 'numpy.ndarray' in str(type(value))
+    type_name = str(type(value))
+    return  'numpy.ndarray' in type_name or \


How about, long, short, byte and other types numpy supports?

It doesn't. It's always intN, uintN or floatN. Numpy supports array of bool, but the element is an ordinary Python bool.

JanuszL · 2020-10-26T10:23:13Z

dali/python/nvidia/dali/pipeline.py

@@ -393,6 +393,8 @@ def _prepare_graph(self, define_graph = None):
            if isinstance(outputs[i], types.ScalarConstant):
                import nvidia.dali.ops
                outputs[i] = nvidia.dali.ops._instantiate_constant_node("cpu", outputs[i])
+            elif not isinstance(outputs[i], DataNode):
+                outputs[i] = types.Constant(outputs[i], device="cpu")


Can we support cupy and GPU constants as well?

Not at present.

lgtm-com · 2020-10-26T14:47:40Z

This pull request introduces 1 alert when merging a1a816bc6637c38c016612cfa136358249262723 into 8112549 - view on LGTM.com

new alerts:

1 for Unused import

mzient · 2020-10-26T14:51:26Z

dali/test/python/test_pipeline.py

@@ -1707,3 +1707,14 @@ def get_output():
    out = get_output()[0].at(0)
    assert out[0] == -0.5 and out[1] == 1.25

+def test_return_constants():
+    pipe = dali.pipeline.Pipeline(1, 1, None)
+    types = [bool, np.int8, np.uint8, np.int16, np.uint16, np.int32, np.uint32, np.float32]


NOTE: Constant operator doesn't support int64 or double at native level. We should probably fix it at some point somehow, but I don't have a good idea of how to do that at present.

mzient · 2020-10-26T15:02:58Z

!build

dali-automaton · 2020-10-26T15:05:32Z

CI MESSAGE: [1733940]: BUILD STARTED

jantonguirao · 2020-10-26T16:22:51Z

dali/python/nvidia/dali/external_source.py

@@ -259,6 +259,9 @@ def __init__(self, source = None, num_outputs = None, *, cycle = None, layout =
        self._cuda_stream = cuda_stream
        self._use_copy_kernel = use_copy_kernel

+        import nvidia.dali.ops


move this import to the top of the file?

This is a circular import, it won't work on top of the file.

jantonguirao · 2020-10-26T16:30:36Z

dali/test/python/test_operator_compose.py

+
+def test_compose():
+    batch_size = 3
+    pipe = Pipeline(batch_size,1,None)


Suggested change

pipe = Pipeline(batch_size,1,None)

pipe = Pipeline(batch_size, 1, None)

nitpick

jantonguirao · 2020-10-26T16:31:55Z

dali/test/python/test_operator_input_promotion.py

@@ -3,7 +3,16 @@
 import numpy as np

 def test_cat_numpy_array():
-    pipe = dali.pipeline.Pipeline(1,1,0)
+    pipe = dali.pipeline.Pipeline(1,1,None)


Suggested change

pipe = dali.pipeline.Pipeline(1,1,None)

pipe = dali.pipeline.Pipeline(1, 1, None)

nitpick

jantonguirao · 2020-10-26T16:37:38Z

dali/python/nvidia/dali/ops.py

+
+        return inputs[0] if len(inputs) == 1 else inputs
+
+def Compose(op_list):


Is this visible in our supported operations table? Can you paste a screenshot?

I think it may deserve a bigger and more complete example.

For now it's marked as experimental. I think the next step would be to send it to the proponent of the whole Compose affair to play with and. Once we get some feedback and suggestions for improvements, we can improve the functionality and provide comprehensive documentation - or, conversely, remove it if we decide it's not necessary after all.

It is not listed in Support Table.
I think you need

global _cpu_ops _cpu_ops = _cpu_ops.union({'DLTensorPythonFunction'}) global _gpu_ops _gpu_ops = _gpu_ops.union({'DLTensorPythonFunction'})

Also how it handles CPU to GPU memory transfer?

I added the handling of transfers.
As for the support table, it's a bit harder, since this is not an operator in native sense and doesn't have a schema.

Do we need a schema to make it a part of the table?

Unless I hardcode it into the table generator - then yes. When I tried just putting the operator there using set union, Sphinx failed with No schema registered for operator Compose.

dali-automaton · 2020-10-26T16:39:38Z

CI MESSAGE: [1733940]: BUILD PASSED

dali/python/nvidia/dali/ops.py

mzient · 2020-10-26T19:17:00Z

!build

dali-automaton · 2020-10-26T20:13:30Z

CI MESSAGE: [1734773]: BUILD STARTED

dali-automaton · 2020-10-26T21:57:38Z

CI MESSAGE: [1734773]: BUILD FAILED

dali-automaton · 2020-10-26T22:08:23Z

CI MESSAGE: [1734773]: BUILD PASSED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

…d from the pipeline. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Added Compose function to combine multiple operator instances. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Extended documentation and tests. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient · 2020-10-27T07:45:26Z

!build

dali-automaton · 2020-10-27T07:50:34Z

CI MESSAGE: [1736521]: BUILD STARTED

JanuszL · 2020-10-27T08:37:57Z

docs/supported_op_devices.py

-        supports_seq = '|v|' if schema.AllowsSequences() or schema.IsSequenceOperator() else ''
-        volumetric = '|v|' if schema.SupportsVolumetric() else ''
+        try:
+            schema = b.GetSchema(op)


I think we can exposer TryGetSchema in backend_impl.cc and use it here instead.
Other than that LGTM.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient · 2020-10-27T09:26:06Z

!build

dali-automaton · 2020-10-27T09:30:44Z

CI MESSAGE: [1736871]: BUILD STARTED

dali-automaton · 2020-10-27T10:05:08Z

CI MESSAGE: [1736521]: BUILD PASSED

dali-automaton · 2020-10-27T10:51:06Z

CI MESSAGE: [1736871]: BUILD PASSED

mzient requested a review from a team October 23, 2020 18:02

mzient force-pushed the ArgInputConstantPromotion branch 3 times, most recently from 1bf39ba to 2fdec26 Compare October 23, 2020 18:05

mzient changed the title ~~Promote non-scalar constants to DataNodes in named arguments.~~ Scalar and constant related improvements. Oct 24, 2020

jantonguirao reviewed Oct 26, 2020

View reviewed changes

JanuszL reviewed Oct 26, 2020

View reviewed changes

mzient changed the title ~~Scalar and constant related improvements.~~ Generalized handling of operator arguments. Oct 26, 2020

mzient force-pushed the ArgInputConstantPromotion branch from 6e5c219 to a1a816b Compare October 26, 2020 14:37

mzient force-pushed the ArgInputConstantPromotion branch from a1a816b to 1a53ddb Compare October 26, 2020 14:49

mzient commented Oct 26, 2020

View reviewed changes

jantonguirao approved these changes Oct 26, 2020

View reviewed changes

JanuszL reviewed Oct 26, 2020

View reviewed changes

dali/python/nvidia/dali/ops.py Show resolved Hide resolved

mzient force-pushed the ArgInputConstantPromotion branch from 1a53ddb to 87134ee Compare October 26, 2020 19:16

mzient force-pushed the ArgInputConstantPromotion branch from 87134ee to 926ce89 Compare October 26, 2020 19:21

mzient force-pushed the ArgInputConstantPromotion branch from 926ce89 to 765b66d Compare October 26, 2020 19:35

mzient added 5 commits October 27, 2020 08:34

Promote non-scalar constants to DataNodes in named arguments.

d29e679

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

More constant and scalar-related fixes. Allow constants to be returne…

31f9308

…d from the pipeline. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Added operator argument separation.

8c3f8ef

Added Compose function to combine multiple operator instances. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Move data from CPU to GPU in Compose.

ca2c436

Extended documentation and tests. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Another shot at adding Compose to support table.

b5116ce

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the ArgInputConstantPromotion branch from 7ea611a to b5116ce Compare October 27, 2020 07:34

JanuszL reviewed Oct 27, 2020

View reviewed changes

JanuszL approved these changes Oct 27, 2020

View reviewed changes

Expose TryGetSchema in Python bindings.

3fe0b52

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

JanuszL approved these changes Oct 27, 2020

View reviewed changes

mzient merged commit 5df1ac5 into NVIDIA:master Oct 27, 2020

mzient mentioned this pull request Nov 21, 2020

Add 'Compose()' style interface for DALI IO and transforms #1731

Closed

cristyioan2000 mentioned this pull request Dec 14, 2020

Conditional Augmentations in the graph #2550

Closed

	pipe = dali.pipeline.Pipeline(1, 1, 0)
	pipe = dali.pipeline.Pipeline(1, 1, None)

	pipe = Pipeline(batch_size,1,None)
	pipe = Pipeline(batch_size, 1, None)

	pipe = dali.pipeline.Pipeline(1,1,None)
	pipe = dali.pipeline.Pipeline(1, 1, None)


		return inputs[0] if len(inputs) == 1 else inputs

		def Compose(op_list):

Generalized handling of operator arguments. #2393

Generalized handling of operator arguments. #2393

Conversation

mzient commented Oct 23, 2020 • edited Loading

Why we need this PR?

What happened in this PR?

mzient commented Oct 23, 2020

dali-automaton commented Oct 23, 2020

dali-automaton commented Oct 23, 2020

mzient commented Oct 24, 2020

dali-automaton commented Oct 24, 2020

dali-automaton commented Oct 24, 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

lgtm-com bot commented Oct 26, 2020

Choose a reason for hiding this comment

mzient commented Oct 26, 2020

dali-automaton commented Oct 26, 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

JanuszL Oct 26, 2020 • 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

dali-automaton commented Oct 26, 2020

mzient commented Oct 26, 2020

dali-automaton commented Oct 26, 2020

dali-automaton commented Oct 26, 2020

dali-automaton commented Oct 26, 2020

mzient commented Oct 27, 2020

dali-automaton commented Oct 27, 2020

Choose a reason for hiding this comment

mzient commented Oct 27, 2020

dali-automaton commented Oct 27, 2020

dali-automaton commented Oct 27, 2020

dali-automaton commented Oct 27, 2020

mzient commented Oct 23, 2020 •

edited

Loading

JanuszL Oct 26, 2020 •

edited

Loading