Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only support upsample with ONNX OPSET version <=7 #2792

Closed
wants to merge 1 commit into from
Closed

Only support upsample with ONNX OPSET version <=7 #2792

wants to merge 1 commit into from

Conversation

gemfield
Copy link
Contributor

"scales" now is an input instead of an attribute in ONNX OPSET 8, so if we compile a OPSET 8 onnx model, below error may occur if there is upsample op in the model:

assert len(scales) == 4 and scales[0] == 1.0 and scales[1] == 1.0 and scales[2] == scales[3]
TypeError: object of type 'NoneType' has no len()

This commit will report a meaningful error message in this situation.

@@ -449,6 +449,8 @@ class Upsample(OnnxOpConverter):
@classmethod
def _impl_v7(cls, inputs, attr, params):
scales = attr.get('scales')
if not scales:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputs[1] may be mostly available in params.

Can you attempt taking it from params and proceeding to make it compatible for opset-8 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how to get the scale value from input[1], and how to create the new input? Is following code snippet resonable? Thanks.

    def _impl_v7(cls, inputs, attr, params):
        scales = attr.get('scales')
        if not scales:
            #Here we are going to higher OPSET version.
            assert len(inputs) == 2, "Upsample op take 2 inputs, {} given".format(len(inputs))
            scales = params[inputs[1].name_hint].asnumpy()
            inputs = inputs[:1]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemfield yes, above code look good. Add it and let the CI pass for it.
Also please try adding a test case too for it.

@gemfield
Copy link
Contributor Author

Closed, see #2840

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants