Expose arithm ops in Python #1355

klecki · 2019-10-08T19:31:00Z

Make arithmetic ops accessible from python using python special methods for operator overloading on _EdgeReference.
Supported ops: +, -, *, /, //

Why we need this PR?

So we can use arithmetic Ops in natural way through python API.

What happened in this PR?

Explain solution of the problem, new feature added.
EdgeReference (the return value type of Operator call) was extended with overloaded operators (like add, mul, ...) that place a appropriate ArithmeticGenericOp.
Additionally artihm_meta is now fully specialized every time.
A fdiv op was added to reflect python's insistence of producing float in division.
What was changed, added, removed?
EdgeReference from nvidia.dali.edge was deprecated and is replaced by _EdgeReference, that was placed in nvidia.dali.ops.
What is most important part that reviewers should focus on?
Pipeline validation of EdgeReference-like objects, loading of arithmetic ops
Was this PR tested? How?
Jupyter notebook, will add a nose test.
Were docs and examples updated, if necessary?
There is an example utilizing numpy added, in future PRs I will add some image blending but I want to have more backend features available for it.

JIRA TASK: [DALI-1054]

JanuszL · 2019-10-08T22:34:50Z

Any test or example?

klecki · 2019-10-17T14:10:29Z

!build

dali-automaton · 2019-10-17T14:15:08Z

CI MESSAGE: [949094]: BUILD STARTED

dali-automaton · 2019-10-17T15:43:05Z

CI MESSAGE: [949094]: BUILD PASSED

mzient · 2019-10-17T17:28:19Z

dali/python/nvidia/dali/check_edge.py

+# limitations under the License.
+
+def _validate_edge_reference(edge):
+    # TODO(klecki): adjust to <class 'nvidia.dali.ops._EdgeReference'>


it's <class in Python 3, but <type in Python 2, just FYI

mzient · 2019-10-17T17:31:51Z

dali/python/nvidia/dali/ops.py

+    def __rmul__(self, other):
+        return _arithm_op("mul", other, self)
+
+    def __truediv__(self, other):


What are the semantics here?
Are we promoting division results to floating point?

From now on, yes

mzient · 2019-10-17T17:35:19Z

dali/python/nvidia/dali/ops.py

+def _arithm_op(name, *inputs):
+        input_desc = ""
+        for i, input in enumerate(inputs):
+            input_desc += "&" + str(i)


This is based on master, and constants are in #1391 which is not merger yet. I will add support for constants in a follow-up after this and #1391 are merged.

jantonguirao · 2019-10-18T08:22:37Z

dali/python/nvidia/dali/check_edge.py

+          raise TypeError(("Expected outputs of type compatible with \"EdgeReference\". Received"
+                  " output type \"{}\" does not have the attribute \"{}\" that is required.")
+                  .format(type(edge).__name__, attr))
+    return True


nitpick: add new line at the end of file

jantonguirao · 2019-10-18T09:16:12Z

dali/python/nvidia/dali/edge.py

@@ -17,8 +17,17 @@
 from nvidia.dali.backend import TensorCPU
 from nvidia.dali.backend import TensorListCPU

+print("[Warning] nvidia.dali.edge is deprecated. For TensorCPU and TensorListCPU " +
+        "use nvidia.dali.tensors. EdgeReference is intended to be created only by " +


Suggested change

"use nvidia.dali.tensors. EdgeReference is intended to be created only by " +

"use nvidia.dali.tensors._EdgeReference is intended to be created only by " +

No. This is a next sentence.

jantonguirao · 2019-10-18T09:22:15Z

dali/python/nvidia/dali/ops.py

+_load_arithm_ops()
+
+def _choose_device(inputs):
+    result_device = "cpu"


suggestion:

result_device = "gpu" if any(in.device == "gpu" for in in inputs) else "cpu

I think a loop is clearer, especially than inlining python weird ternary with it's clunky list-comprehension.

I used a version of this actually, but without the ternary.

jantonguirao · 2019-10-18T10:15:06Z

docs/examples/expressions.ipynb

+    "        left = []\n",
+    "        right = []\n",
+    "        for sample in range(self.batch_size):\n",
+    "            left.append(np.array([sample + self.i], dtype = self.left_type))\n",


Can we have this kind of tests also in python tests?

I added one.

klecki · 2019-10-18T17:22:37Z

!build

dali-automaton · 2019-10-18T19:22:03Z

CI MESSAGE: [951238]: BUILD STARTED

dali-automaton · 2019-10-18T19:29:50Z

CI MESSAGE: [951238]: BUILD FAILED

klecki · 2019-10-21T08:48:46Z

!build

dali-automaton · 2019-10-21T08:50:23Z

CI MESSAGE: [953488]: BUILD STARTED

dali-automaton · 2019-10-21T08:56:38Z

CI MESSAGE: [953488]: BUILD FAILED

JanuszL · 2019-10-21T10:32:01Z

dali/python/nvidia/dali/ops.py

+    def __rfloordiv__(self, other):
+        return _arithm_op("div", other, self)
+
+    # def __neg__(self):


It is some ToDo?

Yes, as we don't have the unary ops implemented yet (they are in next PR).

klecki · 2019-10-21T10:51:20Z

!build

dali-automaton · 2019-10-21T10:55:16Z

CI MESSAGE: [953607]: BUILD STARTED

JanuszL · 2019-10-21T11:25:00Z

docs/examples/expressions.ipynb

@@ -0,0 +1,170 @@
+{


You need to add it into docs.

JanuszL · 2019-10-21T11:27:39Z

docs/examples/expressions.ipynb

+    "The next step is to define the Pipeline.\n",
+    "\n",
+    "We override `Pipeline.iter_setup`, a method called by the pipeline before every `Pipeline.run`, to call the iterator\n",
+    "and feed the result to `ExternalSource()` operator, referenced by `self.jpeg`, by using `feed_input`."


self.jpeg ?

JanuszL · 2019-10-21T11:29:24Z

docs/examples/expressions.ipynb

+    "    def define_graph(self):                                                                \n",
+    "        self.left = self.left_source()\n",
+    "        self.right = self.right_source()\n",
+    "        return self.left, self.right, self.left + self.right * self.left\n",


I would add a comment that now you can operate on tensors in define_graph and DALI under the hood would insert right operators. After all this is all about it.

jantonguirao · 2019-10-21T11:35:32Z

dali/operators/expressions/arithmetic_meta.h

@@ -204,75 +176,136 @@ inline DALIDataType TypePromotion(span<DALIDataType> types) {
 */
 template <ArithmeticOp op, typename Backend>
 struct arithm_meta {


shouldn't it be just forward declared (instead of empty)?

jantonguirao · 2019-10-21T11:37:37Z

dali/operators/expressions/expression_impl_factory_gpu.cu

@@ -24,8 +24,9 @@
 #define ALLOWED_TYPES \
  (uint8_t, uint16_t, uint32_t, uint64_t, int8_t, int16_t, int32_t, int64_t, float, double)

-#define ALLOWED_OPS \
-  (ArithmeticOp::add, ArithmeticOp::sub, ArithmeticOp::mul, ArithmeticOp::div, ArithmeticOp::mod)
+#define ALLOWED_OPS                                                                                \


how about defining ALLOWED_TYPES and ALLOWED_OPS only once?

It's done in next PR, where I merged the factories.

jantonguirao · 2019-10-21T11:37:49Z

dali/operators/expressions/expression_impl_factory_gpu.cu

@@ -24,8 +24,9 @@
 #define ALLOWED_TYPES \
  (uint8_t, uint16_t, uint32_t, uint64_t, int8_t, int16_t, int32_t, int64_t, float, double)


float16 missing

It's broken. We need to clean up the cpu/gpu types that were merged and don't work. Otherwise the type promotions get complicated.

klecki · 2019-10-21T12:51:45Z

!build

dali-automaton · 2019-10-21T13:08:55Z

CI MESSAGE: [953741]: BUILD STARTED

JanuszL · 2019-10-21T13:09:37Z

docs/examples/external_input.ipynb

@@ -220,7 +220,7 @@
   "name": "python",
   "nbconvert_exporter": "python",
   "pygments_lexer": "ipython2",
-   "version": "2.7.15+"
+   "version": "2.7.12"


I <3 jupyter.

klecki · 2019-10-21T13:37:43Z

!build

dali-automaton · 2019-10-21T13:41:14Z

CI MESSAGE: [953771]: BUILD STARTED

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com> Add a test and implement division that results in `float` Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2019-10-21T14:56:18Z

!build

dali-automaton · 2019-10-21T15:00:04Z

CI MESSAGE: [953834]: BUILD STARTED

dali-automaton · 2019-10-21T17:24:50Z

CI MESSAGE: [953834]: BUILD PASSED

klecki force-pushed the python-arithm-op-2 branch 2 times, most recently from 7acc71c to 130571a Compare October 9, 2019 16:52

klecki force-pushed the python-arithm-op-2 branch 4 times, most recently from b9d1848 to ced658d Compare October 17, 2019 14:10

klecki changed the title ~~[WIP] Arithm ops in Python~~ Arithm ops in Python Oct 17, 2019

klecki requested review from jantonguirao, banasraf, awolant, JanuszL and mzient October 17, 2019 16:08

mzient reviewed Oct 17, 2019

View reviewed changes

jantonguirao reviewed Oct 18, 2019

View reviewed changes

klecki force-pushed the python-arithm-op-2 branch 3 times, most recently from 601f91b to 7c992d7 Compare October 18, 2019 17:18

klecki force-pushed the python-arithm-op-2 branch from 7c992d7 to 9695d1b Compare October 21, 2019 08:48

JanuszL reviewed Oct 21, 2019

View reviewed changes

klecki force-pushed the python-arithm-op-2 branch from 9695d1b to 3b8ae7a Compare October 21, 2019 10:50

JanuszL reviewed Oct 21, 2019

View reviewed changes

jantonguirao reviewed Oct 21, 2019

View reviewed changes

klecki force-pushed the python-arithm-op-2 branch from 3b8ae7a to 433c363 Compare October 21, 2019 12:49

jantonguirao approved these changes Oct 21, 2019

View reviewed changes

JanuszL reviewed Oct 21, 2019

View reviewed changes

JanuszL approved these changes Oct 21, 2019

View reviewed changes

klecki force-pushed the python-arithm-op-2 branch from 433c363 to f26d280 Compare October 21, 2019 13:36

Add supprot for Arithmetic Ops in Python

2f85c47

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com> Add a test and implement division that results in `float` Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki force-pushed the python-arithm-op-2 branch from f26d280 to 2f85c47 Compare October 21, 2019 14:55

klecki changed the title ~~Arithm ops in Python~~ Expose arithm ops in Python Oct 21, 2019

klecki merged commit 78f78b0 into NVIDIA:master Oct 21, 2019

klecki deleted the python-arithm-op-2 branch October 21, 2019 17:29

	"use nvidia.dali.tensors. EdgeReference is intended to be created only by " +
	"use nvidia.dali.tensors._EdgeReference is intended to be created only by " +

		@@ -24,8 +24,9 @@
		#define ALLOWED_TYPES \
		(uint8_t, uint16_t, uint32_t, uint64_t, int8_t, int16_t, int32_t, int64_t, float, double)

Expose arithm ops in Python #1355

Expose arithm ops in Python #1355

Conversation

klecki commented Oct 8, 2019 • edited Loading

Why we need this PR?

What happened in this PR?

JanuszL commented Oct 8, 2019

klecki commented Oct 17, 2019

dali-automaton commented Oct 17, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klecki commented Oct 18, 2019

dali-automaton commented Oct 18, 2019

dali-automaton commented Oct 18, 2019

klecki commented Oct 21, 2019

dali-automaton commented Oct 21, 2019

dali-automaton commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klecki commented Oct 21, 2019

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

JanuszL Oct 21, 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

klecki Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

klecki commented Oct 21, 2019

dali-automaton commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klecki commented Oct 21, 2019

dali-automaton commented Oct 21, 2019

klecki commented Oct 21, 2019

dali-automaton commented Oct 21, 2019

dali-automaton commented Oct 21, 2019

klecki commented Oct 8, 2019 •

edited

Loading

JanuszL Oct 21, 2019 •

edited

Loading

klecki Oct 21, 2019 •

edited

Loading