Enable to load parameters & convert fit_a_line model #9

kuke · 2018-04-08T08:19:33Z

Resolve #4
Resolve #3

The correctness has been verified in #11.

kuke · 2018-04-08T08:22:14Z

Run

python convert.py --fluid_model extras/fit_a_line.inference.model/

The output yields:

The converted model is:
ir_version: 3
producer_name: "PaddlePaddle"
graph {
  node {
    output: "fc_0.w_0"
    op_type: "Constant"
    attribute {
      name: "value"
      t {
        dims: 13
        dims: 1
        data_type: FLOAT
        float_data: -0.235716566443
        float_data: 1.50793659687
        float_data: -1.37839913368
        float_data: 0.587660908699
        float_data: -1.62691628933
        float_data: 1.94002008438
        float_data: -1.5584435463
        float_data: 1.01809895039
        float_data: -2.47688126564
        float_data: -2.48663592339
        float_data: -2.72155380249
        float_data: 1.01887917519
        float_data: -2.73560881615
        name: "fc_0.w_0"
      }
      type: TENSOR
    }
  }
  node {
    output: "fc_0.b_0"
    op_type: "Constant"
    attribute {
      name: "value"
      t {
        dims: 1
        data_type: FLOAT
        float_data: 19.3055801392
        name: "fc_0.b_0"
      }
      type: TENSOR
    }
  }
  node {
    input: "x"
    input: "fc_0.w_0"
    output: "fc_0.tmp_0"
    op_type: "MatMul"
  }
  node {
    input: "fc_0.tmp_0"
    input: "fc_0.b_0"
    output: "fc_0.tmp_1"
    op_type: "Add"
    attribute {
      name: "broadcast"
      i: 1
      type: INT
    }
  }
  name: "fit_a_line"
  input {
    name: "x"
    type {
      tensor_type {
        elem_type: FLOAT
        shape {
          dim {
            dim_value: 0
          }
          dim {
            dim_value: 13
          }
        }
      }
    }
  }
  output {
    name: "fc_0.tmp_1"
    type {
      tensor_type {
        elem_type: FLOAT
        shape {
          dim {
            dim_value: 0
          }
          dim {
            dim_value: 1
          }
        }
      }
    }
  }
}
opset_import {
  version: 6
}

varunarora

Mostly looks good - pretty good stuff. Just a few questions here and there

varunarora · 2018-04-09T18:32:24Z

fluid_onnx/ops.py

    # Paddle op name : (ONNX op name, modifier)
    'abs': ('Abs', abs_op),
-    'elementwise_add': ('Add', add_op),
+    'elementwise_add': add_op,


So the reason I originally did this weird mapping to tuples is because we may want to reuse such a map in future for mapping the reverse way. So it will be easier to traverse that. If you think we will probably need to create an entirely way, this makes much more obvious sense. What do you think?

I thought we can implement the bidirectional conversion in one map. But after I write some code, I feel it would be better to decouple the two conversions very clearly. Because it is a bit hard to make the two sets of operators one-to-one correspondence. For example, ONNX has FC operator, but Fluid doesn't, in Fluid--> ONNX conversion we would never use FC op, and in the reverse conversion we need to implement the FC op with mul and elementwise_add op in Fluid.

Completely fair, let's go with this. I thought of the same, but yeah, I agree with your conclusion

varunarora · 2018-04-09T18:44:14Z

convert.py

+        for var_name in global_block.vars:
+            var = global_block.var(var_name)
+            if var_name not in ['feed', 'fetch'] and var.persistable:
+                param = fluid.executor.fetch_var(var_name, inference_scope)


Oh man this is great, I spent so much time writing a custom (py)binding to deserialize params manually, I'll throw that stuff for now. fetch_var is beautiful. You are the expert

No. Actually I am also not familiar with this part, and just find this method by chance :-)

So let me tell why I have been trying to find overly complex solutions to this. If you see this, it clearly seems like scopes get destroyed after their runs - which is fair, we shouldnt have these thousands, sometimes millions, of variables in memory if they are unused. The only things saved from destruction are persistable global block things (which you seem to be using here). So vars not in global blocks can't be fetched because everything other than global block vars are destroyed.

Now on second thoughts, we can go through the global block only because we don't care about inner blocks, given our use case. But we should be careful because fetch_var would not have been an option if we had cared about them.

It's also important to be careful because the suggested way to fetch anything out of a program run is to add it to the fetch_list argument - and not fetch_var. But to be able to use that, we can't use the default load_inference_model model function - we'd need to tweak it to pass in and return fetch_list we are interested in.

Oh you have a much deeper survey than me. It is not easy to realize the existence of fetch_var because common users usually don't have the necessity to use it.

varunarora · 2018-04-09T18:47:54Z

convert.py

-                        ops.PADDLE_TO_ONNX[op.type][0], op.input_arg_names,
-                        op.output_arg_names)
+                if op.type in ops.node_maker:
+                    # TODO(kuke): deal with the corner case that vars in 


Aren't unique names generated, no matter how local the scope is? You probably know more here..

Yes, seems that duplicated names are allowed for local variables. I am discussing with others working on framework development, to figure out a proper solution. So if you have any idea, please feel free to speak out.

No ideas here, you can leave the comment as is, but I don't think you should worry about a conflict here

Yes, I believe that we can figure a way to solve this conflicts. And should always be careful about the potential risk before it is solved totally.

varunarora · 2018-04-09T18:49:20Z

convert.py

+            var = global_block.var(var_name)
+            if var_name not in ['feed', 'fetch'] and var.persistable:
+                param = fluid.executor.fetch_var(var_name, inference_scope)
+                param_node = helper.make_node(


So where did you figure out that this was the way params need to be set in an ONNX model? I have been confused and asking around

I also didn't find the way to load parameters until I checked the onnx models in onnx/models, and found that they use Constant op to store parameter.

Oh interesting. I asked this question on the ONNX gitter chat and searched a lot through their issues. So (a) your solution is good based on the info you have. (b) it might be a good idea to use what the practice they are recommending. Which is use the initializers argument on the make_model function. More here: https://github.com/onnx/onnxmltools/tree/master/onnxmltools/convert/common and https://github.com/onnx/onnxmltools/blob/master/onnxmltools/convert/coreml/NeuralNetwork/fullyconnected.py#L33. So basically collect the values while traversing the variables, and then when calling make_model, pass them in.

This is an important information. I think that we can keep the current implementation and move on. In future we can try the initializers to see if it works.

I don't think so, parameter is a common concept, so we should make this clear.

varunarora · 2018-04-09T18:51:05Z

convert.py

-                    # raise NameError(op.type)
-                    pass
+                    if op.type not in ['feed', 'fetch']:
+                        raise NotImplementedError("OP[%s] is not supported in "


Much smarter error than NameError :)

Yeah, maybe a better one. You remind me to raise an error here ;-).

varunarora · 2018-04-09T20:20:48Z

Could you also update the README for the new arguments? Oh and when you do, you could probably remove the work in progress label I put on the top and update the status also.

varunarora · 2018-04-09T20:35:37Z

Do you know what the opset_import version is suppose to be? In either case I have version 2 and it will probably be best if we have the same one, so help me understand this

kuke · 2018-04-10T17:05:26Z

Shall we update README and the status after this pull request merged?

opset_import should stand for the version of published operators set. I guess it is because I used ONNX built from source, so I have higher ops version than yours.

https://github.com/onnx/onnx/blob/25747520f78c44b4f58c9ef65663c2de44470520/onnx/onnx-operators.proto3#L120

varunarora

Almost there! :)

varunarora · 2018-04-11T03:12:00Z

convert.py

+        for var_name in global_block.vars:
+            var = global_block.var(var_name)
+            if var_name not in ['feed', 'fetch'] and var.persistable:
+                param = fluid.executor.fetch_var(var_name, inference_scope)


So let me tell why I have been trying to find overly complex solutions to this. If you see this, it clearly seems like scopes get destroyed after their runs - which is fair, we shouldnt have these thousands, sometimes millions, of variables in memory if they are unused. The only things saved from destruction are persistable global block things (which you seem to be using here). So vars not in global blocks can't be fetched because everything other than global block vars are destroyed.

Now on second thoughts, we can go through the global block only because we don't care about inner blocks, given our use case. But we should be careful because fetch_var would not have been an option if we had cared about them.

It's also important to be careful because the suggested way to fetch anything out of a program run is to add it to the fetch_list argument - and not fetch_var. But to be able to use that, we can't use the default load_inference_model model function - we'd need to tweak it to pass in and return fetch_list we are interested in.

varunarora · 2018-04-11T03:29:26Z

convert.py

+            var = global_block.var(var_name)
+            if var_name not in ['feed', 'fetch'] and var.persistable:
+                param = fluid.executor.fetch_var(var_name, inference_scope)
+                param_node = helper.make_node(


Oh interesting. I asked this question on the ONNX gitter chat and searched a lot through their issues. So (a) your solution is good based on the info you have. (b) it might be a good idea to use what the practice they are recommending. Which is use the initializers argument on the make_model function. More here: https://github.com/onnx/onnxmltools/tree/master/onnxmltools/convert/common and https://github.com/onnx/onnxmltools/blob/master/onnxmltools/convert/coreml/NeuralNetwork/fullyconnected.py#L33. So basically collect the values while traversing the variables, and then when calling make_model, pass them in.

varunarora · 2018-04-11T03:30:05Z

convert.py

-                        ops.PADDLE_TO_ONNX[op.type][0], op.input_arg_names,
-                        op.output_arg_names)
+                if op.type in ops.node_maker:
+                    # TODO(kuke): deal with the corner case that vars in 


No ideas here, you can leave the comment as is, but I don't think you should worry about a conflict here

varunarora · 2018-04-11T03:31:01Z

fluid_onnx/ops.py

    # Paddle op name : (ONNX op name, modifier)
    'abs': ('Abs', abs_op),
-    'elementwise_add': ('Add', add_op),
+    'elementwise_add': add_op,


Completely fair, let's go with this. I thought of the same, but yeah, I agree with your conclusion

varunarora

Just update README and we are good to go! 👍

建立文件夹

Enable to load parameters & convert fit_a_line model

584a066

Yibing Liu added 3 commits April 8, 2018 03:46

Support onnx model saving

4a1ad2a

Fix conflicts

7e6be45

Make the fit_a_line onnx model runnable

ed3e4bc

kuke requested review from varunarora, pkuyym and sidgoyal78 April 9, 2018 17:24

varunarora reviewed Apr 9, 2018

View reviewed changes

varunarora mentioned this pull request Apr 9, 2018

Add script for accuracy validation #11

Merged

Merge with the update in travis-ci

9bb3e1b

Yibing Liu added 2 commits April 10, 2018 11:49

Rename var onnx_model_dir

cf64dcd

Add print_arguments in convertor

262372a

varunarora reviewed Apr 11, 2018

View reviewed changes

varunarora approved these changes Apr 11, 2018

View reviewed changes

kuke merged commit b61447b into PaddlePaddle:master Apr 12, 2018

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

Merge pull request PaddlePaddle#9 from Pcjmmc/develop

5b6831b

建立文件夹

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable to load parameters & convert fit_a_line model #9

Enable to load parameters & convert fit_a_line model #9

kuke commented Apr 8, 2018 •

edited

Loading

kuke commented Apr 8, 2018 •

edited

Loading

varunarora left a comment

varunarora Apr 9, 2018

kuke Apr 10, 2018

varunarora Apr 11, 2018

varunarora Apr 9, 2018

kuke Apr 10, 2018

varunarora Apr 11, 2018

kuke Apr 11, 2018 •

edited

Loading

varunarora Apr 9, 2018

kuke Apr 10, 2018

varunarora Apr 11, 2018

kuke Apr 11, 2018 •

edited

Loading

varunarora Apr 9, 2018

kuke Apr 10, 2018

varunarora Apr 11, 2018

kuke Apr 11, 2018

pkuyym Apr 11, 2018

varunarora Apr 9, 2018

kuke Apr 10, 2018

varunarora commented Apr 9, 2018 •

edited

Loading

varunarora commented Apr 9, 2018

kuke commented Apr 10, 2018

varunarora left a comment

varunarora Apr 11, 2018

varunarora Apr 11, 2018

varunarora Apr 11, 2018

varunarora Apr 11, 2018

varunarora left a comment

Enable to load parameters & convert fit_a_line model #9

Enable to load parameters & convert fit_a_line model #9

Conversation

kuke commented Apr 8, 2018 • edited Loading

kuke commented Apr 8, 2018 • edited Loading

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

kuke Apr 11, 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

kuke Apr 11, 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

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 commented Apr 9, 2018 • edited Loading

varunarora commented Apr 9, 2018

kuke commented Apr 10, 2018

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

varunarora left a comment

Choose a reason for hiding this comment

kuke commented Apr 8, 2018 •

edited

Loading

kuke commented Apr 8, 2018 •

edited

Loading

kuke Apr 11, 2018 •

edited

Loading

kuke Apr 11, 2018 •

edited

Loading

varunarora commented Apr 9, 2018 •

edited

Loading