Please verify ONNX v1.10.0 Release Candidate

[rajeevsrao]

Hello ONNX partner,
We have published (to TestPyPI) a Release Candidate package for the upcoming ONNX v1.10.0 release. Please help validate it and let us know if you encounter any issues. Note that the tentative release date for ONNX v1.10.0 is July 31, 2021. Thank you for your assistance!

pip install numpy protobuf typing-extensions>=3.6.2.1
pip install -i https://test.pypi.org/simple/ onnx==1.9.101

• 07/26/2021: RC2 with make_sequence_value_info alias - onnx 1.9.101
• 07/21/2021: RC1 for validation - onnx 1.9.100

cc @guschmue @TomWildenhain-Microsoft

[TomWildenhain-Microsoft]

@rajeevsrao some tests are failing due to the error:
AttributeError: module 'onnx.helper' has no attribute 'make_sequence_value_info'
Did you remove onnx.helper.make_sequence_value_info?

[rajeevsrao]

@TomWildenhain-Microsoft for TensorProto, make_sequence_value_info is now renamed to make_tensor_sequence_value_info so please update your code accordingly. For the newly introduced SparseTensorProto we now have make_sparse_tensor_value_info. FYI: https://github.com/onnx/onnx/pull/3407/files

[TomWildenhain-Microsoft]

I see. To avoid breaking existing code, would it be possible to make make_sequence_value_info call make_tensor_sequence_value_info (and possibly display a deprecation message)? I see that there are some redundant names already:

https://github.com/onnx/onnx/blob/a57bc99daa6ddeef2ad535f8f78d1847f57216f0/onnx/__init__.py#L220

[askhade]

The intention was to clean up the existing apis and make them more intuitive... existing apis were error prone. Since sequence ops are not yet very popular we decided to deprecate this api in this release.

How difficult is it to make this change in tensorflow-onnx converter? Is it a lot of effort?

[TomWildenhain-Microsoft]

No it's not a big deal for the converter, though we'll need to case on the version and still use the old API for previous versions. I have another script that will need to be updated. Also not a big deal but the casing logic is ugly. Just thought it might be cleaner to make an alias to it.

[guschmue]

+1 for Tom. One does not change existing api's because you'd force all consumers to now to look at the onnx version and depending on that version you need to call a different method. Considering that onnx is the onnx python module is called by a lot of applications we badly would want to avoid this!
Its not a big deal for tf2onnx - the big deal is that it breaks every consumer of onnx.

[rajeevsrao]

@guschmue agree that removing the API on short-notice is not advisable. We will post a change with the alias for maintaining backwards compatibility in this release. However this will be on the deprecation schedule (pending clarification from ONNX working groups) and likely going to be removed in a future release. Will post an update to the packages shortly. Thanks.

[guschmue]

Cool. For longer term - I think its never good to change api's and force everybody to update.
But the sig/community should decide.

[TomWildenhain-Microsoft]

Technically if you do change the API I think you need to increase the major version number (2.0.0 not 1.10.0) so PyPI versioning works properly.

[rajeevsrao]

@TomWildenhain-Microsoft @guschmue RC2 with make_sequence_value_info alias is now posted - please help validate v1.9.101

[TomWildenhain-Microsoft]

All conversions are working now but I'm getting some test failures with the onnx checker in opset 8. It seems like for old opsets, initializers for constants had to have a corresponding input entry, which the checker does not like for loop subgraphs.

On a converted model in opset 8 I get errors like:
onnx.onnx_cpp2py_export.shape_inference.InferenceError: [ShapeInferenceError] Shape inference error(s): (op_type:Loop, node name: while_loop): [ShapeInferenceError] Graph has 7 inputs but 3 were provided

Line of tf2onnx adding the inputs:

tensorflow-onnx/tf2onnx/graph.py

Line 1148 in 93fe844

input_ids += [op.output[0] for op in const_ops]

I guess the real issue here is optional inputs in subgraphs of Loop nodes should not cause an error if they do not have a corresponding input in the node.

Alternatively, if this actually is considered an error, tf2onnx shouldn't do this and technically should be updated (though ORT still accepts the model and there will be a lot of models in the wild with this structure).

[TomWildenhain-Microsoft]

Example model:

from onnx import helper, numpy_helper, TensorProto
from onnx.helper import make_node, make_graph

import onnx
import numpy as np

model = helper.make_model(
    opset_imports=[helper.make_operatorsetid('', 8)],
    ir_version=4,
    producer_name='tf2onnx',
    producer_version='1.8.0',
    graph=make_graph(
        name='tf2onnx',
        inputs=[
            helper.make_tensor_value_info('input:0', TensorProto.INT32, shape=[1]),
            helper.make_tensor_value_info('while/maximum_iterations:0', TensorProto.INT64, shape=[]),
            helper.make_tensor_value_info('const_fold_opt__18', TensorProto.INT64, shape=[1]),
            helper.make_tensor_value_info('const_fold_opt__17', TensorProto.FLOAT, shape=[]),
            helper.make_tensor_value_info('Const:0', TensorProto.INT32, shape=[1]),
        ],
        outputs=[helper.make_tensor_value_info('output:0', TensorProto.INT32, shape=[1])],
        initializer=[
            numpy_helper.from_array(np.array(9223372036854775807, dtype=np.int64), name='while/maximum_iterations:0'),
            numpy_helper.from_array(np.array([-1], dtype=np.int64), name='const_fold_opt__18'),
            numpy_helper.from_array(np.array(10.0, dtype=np.float32), name='const_fold_opt__17'),
            numpy_helper.from_array(np.array([1], dtype=np.int32), name='Const:0'),
        ],
        doc_string='test',
        nodes=[
            make_node('Cast', inputs=['input:0'], outputs=['while_cond_158_while/Less__13:0'], name='while_cond_158_while/Less__13', domain='', to=TensorProto.FLOAT),
            make_node('Less', inputs=['while_cond_158_while/Less__13:0', 'const_fold_opt__17'], outputs=['while_cond_158_while/Less:0'], name='while_cond_158_while/Less', domain=''),
            make_node('Squeeze', inputs=['while_cond_158_while/Less:0'], outputs=['while_cond_158_while/Squeeze:0'], name='while_cond_158_while/Squeeze', domain=''),
            make_node(
                'Loop',
                inputs=['while/maximum_iterations:0', 'while_cond_158_while/Squeeze:0', 'input:0', 'Const:0'],
                outputs=['while_loop:0', 'while_loop:1'],
                name='while_loop',
                domain='',
                body=make_graph(
                    name='while_body_159',
                    inputs=[
                        helper.make_tensor_value_info('while_while_loop_counter:0', TensorProto.INT64, shape=[]),
                        helper.make_tensor_value_info('cond__15:0', TensorProto.BOOL, shape=[]),
                        helper.make_tensor_value_info('while_placeholder:0', TensorProto.INT32, shape=[1]),
                        helper.make_tensor_value_info('while_add_const_0:0', TensorProto.INT32, shape=[1]),
                        helper.make_tensor_value_info('const_fold_opt__19', TensorProto.FLOAT, shape=[]),
                    ],
                    outputs=[
                        helper.make_tensor_value_info('cond___while/Identity_graph_outputs_Identity__3:0', TensorProto.BOOL, shape=[]),
                        helper.make_tensor_value_info('while/Identity_2:0', TensorProto.INT32, shape=[1]),
                        helper.make_tensor_value_info('while_add_const_0:0', TensorProto.INT32, shape=[1]),
                    ],
                    initializer=[numpy_helper.from_array(np.array(10.0, dtype=np.float32), name='const_fold_opt__19')],
                    doc_string='graph for while_loop body',
                    nodes=[
                        make_node('Add', inputs=['while_placeholder:0', 'while_add_const_0:0'], outputs=['while/Identity_2:0'], name='while/Add'),
                        make_node('Cast', inputs=['while/Identity_2:0'], outputs=['cond___while/Less__13:0'], name='cond___while/Less__13', domain='', to=TensorProto.FLOAT),
                        make_node('Less', inputs=['cond___while/Less__13:0', 'const_fold_opt__19'], outputs=['cond___while/Less:0'], name='cond___while/Less', domain=''),
                        make_node('Squeeze', inputs=['cond___while/Less:0'], outputs=['cond___while/Identity_graph_outputs_Identity__3:0'], name='cond___while/Squeeze', domain=''),
                    ],
                ),
            ),
            make_node('Unsqueeze', inputs=['while_loop:0'], outputs=['Reshape/tensor:0'], name='Reshape/tensor', axes=[0]),
            make_node('Reshape', inputs=['Reshape/tensor:0', 'const_fold_opt__18'], outputs=['output:0'], name='Reshape'),
        ],
    ),
)

onnx.checker.check_model(model, full_check=True)

[gramalingam]

Initial thoughts: I think this is an error and tf2onnx shouldn't do this. Do these models work with ORT? Specifically, the specification of the Loop op indicates that the subgraph inputs should be loop carried dependences. Constants/initializers are not expected to be in the subgraph inputs. (It is true that the requirements on initializers and inputs changed around the transition from IR version 3 to IR version 4.) From what I understand, these error messages were present in previous ONNX releases too, but it is only the strict-check option that has changed. But if these models work with ORT, may be there is a discrepancy between ORT and ONNX.

[TomWildenhain-Microsoft]

Yes, the models work with ORT.

Note that the constant initializer inputs are always at the end of the input list so everything still lines up if you zip them with the loop carried dependencies. For loops it is like if you had a python function that runs a loop node:

def loop_runner(body, m, cond, *v_initial):
    """Runs a loop.  Body must take 2 + len(v_initial) inputs"""
    vs = v_initial
    i = 0
    while cond and i < m:
        cond, *vs = body(i, cond, *vs)
        i += 1
    return vs

And then gave it:

def sum_n_body(i, cond, running_total, step=1):  # Has 3 or 4 inputs?  Only 3 expected.
    return True, running_total + i * step
result = loop_runner(sum_n_body, 100, True, 0)

It works but sum_n_body arguably has more inputs than expected...

I get the same error for If nodes. Here I think the tf2onnx output is pretty clearly compliant, since the spec for the If op does not state that the graph should have no inputs.

onnx.onnx_cpp2py_export.shape_inference.InferenceError: [ShapeInferenceError] Shape inference error(s): (op_type:If, node name: test_cond): [ShapeInferenceError] Graph has 1 inputs but 0 were provided

I can easily disable the onnx checker for these tests in tf2onnx and this shouldn't block the onnx release, but it's a discrepancy to be aware of. We already skip running the checker for a few tests anyway.

[guschmue]

I think the problem might be that this is shared code that we use for the main onnx graph as well as the subgraph. For the main graph we have to add those constants to the input list, for the subgraph we should not I assume.
The code has been there since opset-9.