[GitHub] [incubator-mxnet] vandanavk commented on a change in pull request #19017: [v1.x] Update onnx support to work with onnx 1.7.0 with most CV models

[GitBox <gi...@apache.org>]

vandanavk commented on a change in pull request #19017:
URL: https://github.com/apache/incubator-mxnet/pull/19017#discussion_r482703828



##########
File path: python/mxnet/contrib/onnx/mx2onnx/_op_translations.py
##########
@@ -462,6 +462,7 @@ def convert_pad(node, **kwargs):
     """Map MXNet's pad operator attributes to onnx's Pad operator
     and return the created node.
     """
+    opset_version = kwargs["opset_version"]

Review comment:
       Is this info always available? We did not get this in the earlier code because there were times when the opset_version was missing. I don't have an example ready in hand, but in case this happens, can we fallback to a default opset_version?

##########
File path: python/mxnet/contrib/onnx/mx2onnx/_op_translations.py
##########
@@ -470,28 +471,74 @@ def convert_pad(node, **kwargs):
     pad_mode = attrs.get("mode")
 
     if pad_mode == "constant":
-        pad_value = float(attrs.get("constant_value")) \
-            if "constant_value" in attrs else 0.0
-        node = onnx.helper.make_node(
-            'Pad',
-            inputs=input_nodes,
-            outputs=[name],
-            mode='constant',
-            value=pad_value,
-            pads=onnx_pad_width,
-            name=name
-        )
+        pad_value = np.float32(attrs.get("constant_value", 0.0))
+        if opset_version >= 11:
+            # starting with opset 11, pads and constant_value are inputs instead of attributes
+            from onnx.helper import make_tensor, make_tensor_value_info
+            initializer = kwargs["initializer"]
+            pads_input_name = name + "_pads"
+            pads_input_type = onnx.TensorProto.INT64
+            pads_input_shape = np.shape(np.array(onnx_pad_width))
+            pads_value_node = make_tensor_value_info(pads_input_name, pads_input_type, pads_input_shape)
+            pads_tensor_node = make_tensor(pads_input_name, pads_input_type, pads_input_shape, onnx_pad_width)
+            initializer.append(pads_tensor_node)
+            input_nodes.append(pads_input_name)

Review comment:
       L476-L485 is repeated below in L515-523. move these lines to a function?

##########
File path: python/mxnet/contrib/onnx/mx2onnx/_op_translations.py
##########
@@ -945,17 +1006,35 @@ def convert_dropout(node, **kwargs):
     and return the created node.
     """
     name, input_nodes, attrs = get_inputs(node, kwargs)
+    opset_version = kwargs["opset_version"]

Review comment:
       can we get this once in get_inputs and return to the caller?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org