some problem about node.attr in frontend

[fumihwh]

After frontend.py L149, I get such node.attr as

{'padding': s: "SAME"
, 'data_format': s: "NHWC"
, 'strides': list {
  i: 1
  i: 1
  i: 1
  i: 1
}
, 'use_cudnn_on_gpu': b: true
}

If I just pass it to make_node in helper.py, I get

{ValueError}Value "s: "NHWC"
" is not valid attribute data type.

So I try to get value before make_node: node.attr = dict(map(lambda item: (item[0], get_attribute_value(item[1])), node.attr.items())),
then I get

{ValueError}Protocol message has no non-repeated submessage field "t"

---

So there are two problems

• current frontend.py can not handle attr structure (maybe changed?) -> can be fixed with my code above
• how to deal with different behavior between tf protobuf3 and onnx protobuf2 (HasField in protobuf3 will cause error) -> create get_attribute_value in onnx-tensorflow or make onnx get_attribute_value let it can deal with both protobuf version

[tjingrant]

1. The first solution sounds good to me.
2. I'm not sure what you mean by this:

make onnx get_attribute_value let it can deal with both protobuf version

Can you explain a bit more?

[fumihwh]

UPDATED:
First, I export a simple net just 2 convolution layers with mnist.
Then, I want to convert to onnx and failed.
Because of something like googleapis/google-cloud-python#1402
If attr doesn't have field, then attr.HasField() will cause error, which is different between protobuf2 and 3.
Any ideas?

[fumihwh]

So I am here. onnx's get_attrbute_value checks fields (f, g, ints, floats, strings, tensors, graphs) that does not exist in AttrValue.
Maybe we should make get_attribute_value inside onnx-tensorflow?

def get_attribute_value(attr):
    if attr.HasField('f'):
        return attr.f
    elif attr.HasField('i'):
        return attr.i
    elif attr.HasField('s'):
        return attr.s
    elif attr.HasField('t'):
        return attr.t
    elif attr.HasField('g'):
        return attr.g
    elif len(attr.floats):
        return list(attr.floats)
    elif len(attr.ints):
        return list(attr.ints)
    elif len(attr.strings):
        return list(attr.strings)
    elif len(attr.tensors):
        return list(attr.tensors)
    elif len(attr.graphs):
        return list(attr.graphs)
    else:
        raise ValueError("Unsupported ONNX attribute: {}".format(attr))

[tjingrant]

It occurs to me that we need proper sanitization before passing it to make_node b/c ONNX does not accept the data_format attribute. Also, I don't think there's Conv op in the frontend yet (and that's part of the reason why data_format attribute wasn't filtered out..). Am I missing something? I don't see why we need to get its attribute.

[fumihwh]

Just leave data_format, attr I mentioned at first is just an example, my local works.
The point is I think we can not just use onnx's get_attribute_value because it contains unneeded if branches which will cause errors (at least in my local......).

[tjingrant]

Can you push your local (as I understand, your local is your proposed changes right?) as a work in progress PR so we can actually look at the code?

[fumihwh]

I've created a PR #67 . I think it shows how do I handle above problems.
You can get example pb here: https://www.dropbox.com/s/5qd8tlbzesyh6z6/tf_gh_mnist.pb?dl=0

There is a similar method in onnx-caffe2.
https://github.com/onnx/onnx-caffe2/blob/master/onnx_caffe2/frontend.py#L83