New version converter tests #3344

matteosal · 2021-03-19T15:14:50Z

A subset of this PR which was split in two parts as requested: #3243

This part contains the new version converter tests
The other part containing the adapters is: #3343

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

onnx/test/new_version_converter_test.py

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

matteosal · 2021-06-14T15:10:17Z

@askhade I managed to write the adapters for the deprecated operators (Scatter and Upsample), removing the deprecated node and creating a new one as you suggested. But it required quite a bit of work, as I had to make all adapters return the current node and change how the converter loops through the nodes.

This PR should be fully ready now.

matteosal · 2021-06-15T16:48:38Z

I have encountered another issue with the version converter: #3529

It would be nice to discuss a solution for that while this PR is under review, so that a fix could be hopefully added here.

postrational · 2021-06-18T12:07:04Z

Please add instructions about creating a converter adapter to the docs.
Preferably here:
https://github.com/onnx/onnx/blob/master/docs/AddNewOp.md
Or here:
https://github.com/onnx/onnx/blob/master/docs/Versioning.md#operator-versioning

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

matteosal · 2021-06-18T13:03:54Z

Please add instructions about creating a converter adapter to the docs.
Preferably here:
https://github.com/onnx/onnx/blob/master/docs/AddNewOp.md
Or here:
https://github.com/onnx/onnx/blob/master/docs/Versioning.md#operator-versioning

Sorry, I forgot about that. Done

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

matteosal · 2021-06-22T12:05:59Z

Any news on this?

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

matteosal · 2021-06-28T15:52:49Z

@askhade @postrational @gramalingam another ping on this

matteosal · 2021-07-08T09:28:27Z

@askhade @postrational @gramalingam can we please move this forward? It's been more than 6 months since the original PR

askhade · 2021-07-12T17:32:21Z

docs/AddNewOp.md

@@ -49,10 +49,11 @@ Once the criteria of proposing new operator/function has been satisfied, you wil
    1. The testing examples will be extracted to the doc.
    2. We also generate binary data for it.
    3. Example: https://github.com/onnx/onnx/blob/master/onnx/backend/test/case/node/abs.py
-5. Update the documentation and generate the test data.
+5. Add at least one automatic upgrade test for your operator in https://github.com/onnx/onnx/blob/master/onnx/test/automatic_upgrade_test.py


Is this step required for new operator?
In case of op updates, what happens if automatic upgrade is not possible?

Can you add more clarification in the doc

I see some details are present in Versioning.md... can you add them here as well as people generally use this doc to understand all the steps involved

Is this step required for new operator?

Yes because the new tests check that all operators are tested at least once (test_ops_tested)

In case of op updates, what happens if automatic upgrade is not possible?

What do you mean by this? As long as the old op's functionality is a subset of the new one's, upgrade is always possible. Maybe the only case in which this got violated is batch_normalization_13_14.h which I added in this PR (opset 14 disabled outputs 4 and 5). Currently the automatic upgrade tests are not testing this upgrade path (they are FAR from testing all the possible upgrade paths from all possible settings for all ops anyways). In the future we can extend these new tests to also add upgrade paths which are expected to fail at some step.

Can you add more clarification in the doc
I see some details are present in Versioning.md... can you add them here as well as people generally use this doc to understand all the steps involved

Versioning.md says to add an adapter whenever a change is made (and don't add a test because the automatic tests will test the adapter), while this is about adding a test whenever a new op is introduced (and don't add an adapter because there's nothing to adapt), so I don't see much overlap. But I have added more information about the basic idea of the new tests.

thanks for the clarification.
How are we enforcing\gating PRs on these tests... Will these tests run as part of python test suite and fail if someone did not add a test for new op?

thanks for the clarification.
How are we enforcing\gating PRs on these tests... Will these tests run as part of python test suite and fail if someone did not add a test for new op?

Yes. The CI will fail in these 2 cases:

A new op is introduced but an automatic upgrade test is not added

An op is changed but an upgrade adapter is not added

askhade · 2021-07-12T17:39:42Z

onnx/version_converter/adapters/adapter.h

@@ -26,7 +26,7 @@ class Adapter {
        target_version_(target_version) {
    }

-    virtual void adapt(std::shared_ptr<Graph> /*graph*/, Node* node) const = 0;
+    virtual Node* adapt(std::shared_ptr<Graph> /*graph*/, Node* node) const = 0;


does this create a new Node and returns it? Can you add comments for this mtd

I had to make this change because in case of deprecated operators we have to destroy the current node and replace it with another one. Since the new node is referenced via its pointer even after adapt is called, adapt has to return the pointer. But 99% of the time (when the node is preserved and only its attributes/inputs/outputs are changed) the returned pointer is the same as the argument.

you have graph and mutable Node* can you not create a new node and point Node* to this new node?

Yes but why bother to create a new node, copy all the attributes/inputs/outputs of the old node to the new one and destroying the old node in all adapters when we can almost always modify the old node in place? That's how the adapters have always worked up to just before this PR. I have added the possibility of returning a different node only to deal with deprecated operators. I have added a comment to clarify this

askhade · 2021-07-12T17:40:40Z

onnx/version_converter/adapters/argmax_argmin_12_11.h

 	    adapt_argmax_argmin_12_11(node);
+      return node;


I dont understand the point of this change.... input Node* node is not const, it is mutable then why return the same node?

See the other reply. All adapters return the same node except adapters for deprecated operators (upsample_9_10.h, scatter_10_11.h).

what is the difficulty in changing say Upsample Node to Resize Node in place? I remember you mentioned it was not possible... cant remember what is the exact reason

I just couldn't find a way to do it. Which doesn't mean it can't be done, but kind_ is private in the definition of Node and it didn't seem designed to be changed to me. If that's possible I'll revert this change and modify the nodes in place for the deprecated ops.

So, I guess it is a matter of changing kind_ to be non-constant, and adding a method to SetKind in a Node?

It seems likely that they didn't anticipate a need to change the operation. But generalizing it to allow it seems reasonable, but I understand your concern whether it might cause some unanticipated problem.

Looking at the (existing) code it is not clear to me what NodeKind is exactly meant for... It is not exactly similar to op_type since it includes popular attribute names and such... and while it includes most of the ops it does not include all... will need to check the usage of Kind through out version converter and optimizer code to understand whether we should add a setter for kind_.

I am OK with the current change now. There are some fundamental changes that need to happen to version converter (updates post IRv3 .i.e allow initializers not included as inputs) soon and we can reconsider this when we take up that task.

I am fine with using the current API to play it safe.

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Added new test file

aeb798c

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

matteosal requested a review from a team as a code owner March 19, 2021 15:14

matteosal mentioned this pull request Mar 19, 2021

Add missing adapters + new version converter tests #3243

Closed

matteosal added 12 commits June 1, 2021 13:03

Merge branch 'master' into feature/VersionConverterTests

f78b707

Added missing compatible adapters

4268495

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Removed wrong compatible adapters for deprecated operators

ac401b7

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Add test for HardSwish

bfe77b7

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Added batchnorm adapter

21ac1bf

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Added optional argument to upgrade only up to a certain opset

547b558

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Enabled tests for scatter and upsample

2c1913e

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Fix variable name, add type annotation

847aa56

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Flake8 fixes

b509e89

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Remove compile warning

d5844b2

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Merge branch 'master' into feature/VersionConverterTests

b46f197

Added adapter & test for NegativeLogLikelihoodLoss

bc58cb3

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

jcwchen reviewed Jun 10, 2021

View reviewed changes

onnx/test/new_version_converter_test.py Outdated Show resolved Hide resolved

matteosal added 7 commits June 11, 2021 13:16

Perform shape inference in strict mode and fix softmax adapter

382ee1d

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Return a node pointer from all adapters

debe822

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Add adapter for Scatter

469f752

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Loop through nodes by explicitly manipulating the iterator

7d9413e

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Test for Scatter across deprecation point

5879ca3

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Added adapter for Upsample

5253f71

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Always upgrade all ops to the latest version

85ddd1e

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Merge branch 'master' into feature/VersionConverterTests

32687ae

matteosal added 3 commits June 18, 2021 14:35

Convert subgraphs incrementally

0b24238

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Renamed test file

33bc414

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Added documentation

a3540dc

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

matteosal requested a review from a team as a code owner June 18, 2021 13:03

matteosal added 2 commits June 18, 2021 15:03

Merge branch 'master' into feature/VersionConverterTests

b5b4368

Upgrade test for new Bernoulli op

428ed9e

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

matteosal mentioned this pull request Jun 21, 2021

More recurrent tests (reprise) #2923

Closed

matteosal added 2 commits June 24, 2021 13:05

Create Constant node in adapter, added Loop upgrade test

9891e7a

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Use Constant ops instead of initializers for all adapters

d25b34a

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

matteosal mentioned this pull request Jun 24, 2021

Bug with upgrading the Loop operator #3529

Closed

Merge branch 'master' into feature/VersionConverterTests

4cf036b

matteosal added 2 commits June 30, 2021 14:47

Merge branch 'master' into feature/VersionConverterTests

8f57b79

Merge branch 'master' into feature/VersionConverterTests

b30ddff

matteosal mentioned this pull request Jul 10, 2021

[Tracking] Work for 1.10 release #3490

Closed

askhade added this to the 1.10 milestone Jul 12, 2021

askhade reviewed Jul 12, 2021

View reviewed changes

matteosal added 5 commits July 12, 2021 20:59

Clarify documentation

94d89b7

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Merge branch 'master' into feature/VersionConverterTests

2b2bf1a

Added comment

1cdd8fa

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

Merge branch 'master' into feature/VersionConverterTests

7abd052

Added BatchNormalization 14->15

9bbf1ac

Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>

gramalingam approved these changes Jul 13, 2021

View reviewed changes

gramalingam merged commit 39c21b2 into onnx:master Jul 13, 2021

matteosal deleted the feature/VersionConverterTests branch April 26, 2023 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New version converter tests #3344

New version converter tests #3344

matteosal commented Mar 19, 2021

matteosal commented Jun 14, 2021

matteosal commented Jun 15, 2021

postrational commented Jun 18, 2021 •

edited

Loading

matteosal commented Jun 18, 2021

matteosal commented Jun 22, 2021

matteosal commented Jun 28, 2021

matteosal commented Jul 8, 2021

askhade Jul 12, 2021 •

edited

Loading

askhade Jul 12, 2021

matteosal Jul 12, 2021

askhade Jul 13, 2021

matteosal Jul 13, 2021

askhade Jul 12, 2021

matteosal Jul 12, 2021

askhade Jul 13, 2021

matteosal Jul 13, 2021 •

edited

Loading

askhade Jul 12, 2021

matteosal Jul 12, 2021

askhade Jul 13, 2021

matteosal Jul 13, 2021

gramalingam Jul 13, 2021

gramalingam Jul 13, 2021

askhade Jul 13, 2021

gramalingam Jul 13, 2021

New version converter tests #3344

New version converter tests #3344

Conversation

matteosal commented Mar 19, 2021

matteosal commented Jun 14, 2021

matteosal commented Jun 15, 2021

postrational commented Jun 18, 2021 • edited Loading

matteosal commented Jun 18, 2021

matteosal commented Jun 22, 2021

matteosal commented Jun 28, 2021

matteosal commented Jul 8, 2021

askhade Jul 12, 2021 • 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

matteosal Jul 13, 2021 • 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

Choose a reason for hiding this comment

postrational commented Jun 18, 2021 •

edited

Loading

askhade Jul 12, 2021 •

edited

Loading

matteosal Jul 13, 2021 •

edited

Loading