Enhance batch_norm_op & rename output args sharing names with inputs #30

kuke · 2018-04-24T08:07:47Z

Resolve #31, see the validation results in #32
Resolve #34, please also review #33

kuke · 2018-04-24T12:20:09Z

convert.py

@@ -84,7 +84,7 @@ def convert(args):
                    # TODO(kuke): deal with the corner case that vars in 
                    #     different blocks have the same name
                    node_proto = ops.node_maker[op.type](operator=op,
-                                                         scope=inference_scope)
+                                                         block=block)


In batch_norm_op, we need to know the shape of input data, which is non-persistable, so here block is needed.

kuke · 2018-04-24T12:31:17Z

fluid/utils.py


-    return inputs, attrs, outputs
+class UniqOpIOs():
+    """Return input/output information for an operator, and resolve potential 


In Fluid Program, inputs and outputs may share the same name to optimize memory utility. Changes here are going to solve this problem. E.g., relu_op in ResNet50

ops { inputs { parameter: "X" arguments: "batch_norm_33.tmp_2" } outputs { parameter: "Out" arguments: "batch_norm_33.tmp_2" } type: "relu" attrs { name: "use_mkldnn" type: BOOLEAN b: false } is_target: false }

Nice find, I am glad you looked into this because this conflict would be pretty annoying

Yes. Also, we may need to rename the names in fetch_list in case that these memory-shared variables are fetched.

The new update has considered the rename of graph outputs.

varunarora

Great stuff @kuke!

varunarora · 2018-04-25T00:55:06Z

tests/op_test.py

-        # is unnecessary.
+        # and not provided in the list of inputs, we move on to the next
+        # expected input. If not, we make sure it the expected input is 
+        # provided, or that it is unnecessary.


Thanks for fixing my horrible English haha I realized this after I had pushed it.

Aha. Your comments are great, just have minor typos :-)

varunarora · 2018-04-25T00:55:47Z

tests/op_test.py

@@ -113,6 +114,27 @@ def create_var(block, name, np_list, var_proto):
    return var_dict


+def create_tensor(np_value, place):


Great function! Great documentation.

varunarora · 2018-04-25T01:07:35Z

fluid/utils.py

+        return self.inputs, self.attrs, self.outputs
+
+
+get_op_io_info = UniqOpIOs()


So is this a object initialized for the entire program? Slightly uncommon pattern to do an initializer this way. Did you do it because you didn't want to change the get_op_io_info calls everywhere? I think it might be a good idea to initialize a class object to make these calls in place of this pattern

Oh. Initialize get_op_io_info in this way mainly because we have a variable self._all_renamed_outputs here to store all the renamed outputs in the program, and the class UniqOpIOs can only be initialized once. Of course, we can make the variable static:

class UniqOpIOs(): _all_renamed_outputs = {} def __init__(): ... def __call__(): ...

And get_op_io_info will be initialized like that:

get_op_io_info = UniqOpIOs() inputs, attrs, outputs = get_op_io_info(op)

One more line code will be inserted whenever get_op_io_info is used. What do you think?

varunarora · 2018-04-25T01:08:00Z

fluid/utils.py

+        self._all_renamed_outputs = {}
+        self._renamed_cnt = 0
+
+    def get_new_name(self, arg):


Nicely done!

varunarora · 2018-04-25T01:08:23Z

fluid_onnx/ops.py

+    x_shape = block.vars[inputs['X'][0]].shape
+    reshape_node = None
+    if len(x_shape) == 2:
+        reshaped_x = [inputs['X'][0] + '@reshape_0']


varunarora · 2018-04-25T01:09:05Z

fluid/utils.py


-    return inputs, attrs, outputs
+class UniqOpIOs():
+    """Return input/output information for an operator, and resolve potential 


Nice find, I am glad you looked into this because this conflict would be pretty annoying

kuke

Thanks!

kuke · 2018-04-25T03:42:42Z

fluid/utils.py

+        return self.inputs, self.attrs, self.outputs
+
+
+get_op_io_info = UniqOpIOs()


Oh. Initialize get_op_io_info in this way mainly because we have a variable self._all_renamed_outputs here to store all the renamed outputs in the program, and the class UniqOpIOs can only be initialized once. Of course, we can make the variable static:

class UniqOpIOs(): _all_renamed_outputs = {} def __init__(): ... def __call__(): ...

And get_op_io_info will be initialized like that:

get_op_io_info = UniqOpIOs() inputs, attrs, outputs = get_op_io_info(op)

One more line code will be inserted whenever get_op_io_info is used. What do you think?

kuke · 2018-04-25T03:51:55Z

fluid/utils.py


-    return inputs, attrs, outputs
+class UniqOpIOs():
+    """Return input/output information for an operator, and resolve potential 


Yes. Also, we may need to rename the names in fetch_list in case that these memory-shared variables are fetched.

kuke · 2018-04-25T03:55:10Z

tests/op_test.py

-        # is unnecessary.
+        # and not provided in the list of inputs, we move on to the next
+        # expected input. If not, we make sure it the expected input is 
+        # provided, or that it is unnecessary.


Aha. Your comments are great, just have minor typos :-)

kuke · 2018-04-25T15:29:44Z

fluid_onnx/ops.py

@@ -577,8 +623,9 @@ def xor_op():
    # Need to continue the mapping below.
    '': 'ConvTranspose',
    '': 'DepthToSpace',
+    'depthwise_conv2d': conv2d_op,


sidgoyal78

I think this looks pretty good. Thanks for the PR
Do you think there will be a case where we will actually be needing scope (in addition to block)?

kuke · 2018-04-26T23:55:15Z

@sidgoyal78 Thanks for the review. Yes, when loading parameters, we need scope to fetch the value of each tensor. But in the conversion of operators, we just need to know the static information of vars, inputs and outputs, so block is enough.

update run all cases script

Yibing Liu added 4 commits April 22, 2018 05:38

New function create_tensor & format comments in op_test

5ec51d6

Add dropout operation

4684c3f

Enable batch_norm_op to support 2-D input data

567866e

Rename output args sharing the same name with inputs

222e1c6

kuke mentioned this pull request Apr 24, 2018

Some important feedforward models to be supported at first stage #24

Closed

7 tasks

kuke force-pushed the refine_unittest branch 2 times, most recently from dda12a2 to 9f59f9d Compare April 24, 2018 11:58

Change node_maker's param scope to block in op_test

2fdd6d2

kuke force-pushed the refine_unittest branch from 9f59f9d to 2fdd6d2 Compare April 24, 2018 12:18

kuke commented Apr 24, 2018

View reviewed changes

kuke requested review from varunarora, pkuyym and sidgoyal78 April 24, 2018 12:36

varunarora reviewed Apr 25, 2018

View reviewed changes

kuke commented Apr 25, 2018

View reviewed changes

Yibing Liu added 3 commits April 25, 2018 00:35

Rename onnx graph outputs if necessary

a7d43a2

Add elementwise_mul_op & sigmoid_op to support re_resnext

6f808bc

Add depthwise_conv2d_op to support mobilenet

3fb6b00

kuke commented Apr 25, 2018

View reviewed changes

Yibing Liu added 2 commits April 25, 2018 17:58

Merge branch 'develop' of upstream into refine_unittest

38a5b9a

Make model print configurable and onnx model path required

02d9ed9

sidgoyal78 approved these changes Apr 26, 2018

View reviewed changes

kuke merged commit 486b871 into PaddlePaddle:develop Apr 26, 2018

Zeref996 pushed a commit to Zeref996/Paddle2ONNX that referenced this pull request Aug 24, 2021

Update run.sh (PaddlePaddle#30)

7245299

update run all cases script

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance batch_norm_op & rename output args sharing names with inputs #30

Enhance batch_norm_op & rename output args sharing names with inputs #30

kuke commented Apr 24, 2018 •

edited

Loading

kuke Apr 24, 2018

kuke Apr 24, 2018 •

edited

Loading

varunarora Apr 25, 2018

kuke Apr 25, 2018

kuke Apr 25, 2018

varunarora left a comment

varunarora Apr 25, 2018

kuke Apr 25, 2018

varunarora Apr 25, 2018

varunarora Apr 25, 2018

kuke Apr 25, 2018

varunarora Apr 25, 2018

varunarora Apr 25, 2018

varunarora Apr 25, 2018

kuke left a comment •

edited

Loading

kuke Apr 25, 2018

kuke Apr 25, 2018

kuke Apr 25, 2018

kuke Apr 25, 2018

sidgoyal78 left a comment •

edited

Loading

kuke commented Apr 26, 2018

		@@ -113,6 +114,27 @@ def create_var(block, name, np_list, var_proto):
		return var_dict


		def create_tensor(np_value, place):

		return self.inputs, self.attrs, self.outputs


		get_op_io_info = UniqOpIOs()

Enhance batch_norm_op & rename output args sharing names with inputs #30

Enhance batch_norm_op & rename output args sharing names with inputs #30

Conversation

kuke commented Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

kuke Apr 24, 2018 • 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

varunarora left a 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

kuke left a comment • 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

sidgoyal78 left a comment • edited Loading

Choose a reason for hiding this comment

kuke commented Apr 26, 2018

kuke commented Apr 24, 2018 •

edited

Loading

kuke Apr 24, 2018 •

edited

Loading

kuke left a comment •

edited

Loading

sidgoyal78 left a comment •

edited

Loading