Fix conversion from split 11 to split 18. #6020

ZelboK · 2024-03-13T14:50:18Z

Addresses #5222

There does not exist a Split 12 from what I can see. I believe whomever made the conversion for Split 12 to Split 13 made a mistake and intended to do so for Split 11 to Split 13?

IIUC, the conversions should chain. Running this with my built ONNX changes seems to work fine

>>> import onnx
rt onnxruntime
model = onnx.load('simple_model_split.onnx')
model = onnx.version_converter.convert_version(model, 18)
onnx.checker.check_model(model)  # success
sess = onnxruntime.InferenceSession(model.SerializeToString())  # fail>>> import onnxruntime
>>> model = onnx.load('simple_model_split.onnx')
>>> model = onnx.version_converter.convert_version(model, 18)
>>> onnx.checker.check_model(model)  # success
>>> sess = onnxruntime.InferenceSession(model.SerializeToString())  # fail
>>>
>>> sess
<onnxruntime.capi.onnxruntime_inference_collection.InferenceSession object at 0x7f82dcce0a30>
>>>

>>> import onnxruntime as rt
>>> import numpy as np
>>> model_serialized = model.SerializeToString()
>>> sess = rt.InferenceSession(model_serialized)
>>> dummy_input = np.random.randn(1, 1, 28, 28).astype(np.float32)
 Perfo>>>
>>> outputs = sess.run(output_names=['output1', 'output2'],
...                    input_feed={'input': dummy_input})

>>> print(outputs)
[array([[[[ 4.02100086e-01,  1.91692543e+00,  5.60453236e-01, ...,
           1.10894358e+00, -9.10616279e-01,  2.63174176e-01],
         [ 9.16809678e-01,  8.39324117e-01,  1.30101681e+00, ...,
          -6.86384559e-01,  8.50693703e-01, -2.23367453e-01],
         [ 9.90843654e-01,  1.80581224e+00, -6.30877018e-01, ...,
          -4.17214930e-02,  9.42695260e-01, -9.54421163e-01],

Inferencing seems to work. I should remove the old split 12-> 13 converters but I would like confirmation from a contributor before I move forward.

codecov · 2024-03-13T14:59:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.82%. Comparing base (6eb45c0) to head (97f7873).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6020   +/-   ##
=======================================
  Coverage   56.81%   56.82%           
=======================================
  Files         506      506           
  Lines       30370    30377    +7     
  Branches     4592     4592           
=======================================
+ Hits        17256    17263    +7     
  Misses      12285    12285           
  Partials      829      829

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

justinchuby · 2024-03-13T15:57:30Z

There is a catch to this thing (which can be confusing): Split 11 implies (11, 12 ...) until a new version (13) is defined. In theory, we have 12->13, 17->18 already, which should be enough. From the issue it seems like the model is converted but turns out to be invalid.

Looking at the issue and code https://github.com/ZelboK/onnx/blob/3160a050150cc5bc5232c96be3a1bbed5187a264/onnx/version_converter/adapters/split_17_18.h#L25 this line should have handled the situation when split is not provided 🤔

ZelboK · 2024-03-13T16:41:13Z

There is a catch to this thing (which can be confusing): Split 11 implies (11, 12 ...) until a new version (13) is defined. In theory, we have 12->13, 17->18 already, which should be enough. From the issue it seems like the model is converted but turns out to be invalid.

Looking at the issue and code https://github.com/ZelboK/onnx/blob/3160a050150cc5bc5232c96be3a1bbed5187a264/onnx/version_converter/adapters/split_17_18.h#L25 this line should have handled the situation when split is not provided 🤔

Ah I see, I figured there was context I lacked. My bad 😅 . Would reproducing this in a test be trivial? Looking at hte issue again I don't see the model anywhere. I need to familiarize myself with the code flow and see where it is going wrong.

gramalingam · 2024-03-14T19:58:53Z

Justin is right. Version conversion is chained in this fashion.

this line should have handled the situation when split is not provided

It is not supposed to be SetAttribute(...) ! It should be node->_i(...) ... I am surprised this wasn't caught. Any unit-test should have caught this.

ZelboK · 2024-03-15T01:43:30Z

@gramalingam @justinchuby

<
   ir_version: 6,
   opset_import: ["" : 11]
>
model (float[12,42] input) => (float[6,42] a, float[6,42] b) {
   a, b = Split <axis: int = 1> (input)
}
Model 11 is a valid ONNX model.
<
   ir_version: 6,
   opset_import: ["" : 18]
>
model (float[12,42] input) => (float[6,42] a, float[6,42] b) {
   a, b = Split <axis: int = 1, num_outputs: int = 2> (input)
}
Model 18 is a valid ONNX model.

From runing your script, this is what I get now @justinchuby.
Would it make sense to rename SetAttribute? From the name at an immediate glance I picture it actually performing an effect rather than declaring one. It is a bit misleading as is imo from an outsiders POV.

justinchuby · 2024-03-15T06:36:29Z

Would it make sense to rename SetAttribute?

That would be good, but we have to maintain stable public apis.

justinchuby

Thanks a lot! 🙌

justinchuby · 2024-03-15T18:21:11Z

Could you check the DCO ci and sign off commits following its instructions?

ZelboK · 2024-03-15T19:01:21Z

Woops! Forgot to do so. Will do now.

Signed-off-by: Danial Javady <danialjavady96@gmail.com>

…than returning a function that doesn't do it Signed-off-by: Danial Javady <danialjavady96@gmail.com>

Signed-off-by: Danial Javady <danialjavady96@gmail.com>

Addresses onnx#5222 There does not exist a Split 12 from what I can see. I believe whomever made the conversion for Split 12 to Split 13 made a mistake and intended to do so for Split 11 to Split 13? IIUC, the conversions should chain. Running this with my built ONNX changes seems to work fine ``` >>> import onnx rt onnxruntime model = onnx.load('simple_model_split.onnx') model = onnx.version_converter.convert_version(model, 18) onnx.checker.check_model(model) # success sess = onnxruntime.InferenceSession(model.SerializeToString()) # fail>>> import onnxruntime >>> model = onnx.load('simple_model_split.onnx') >>> model = onnx.version_converter.convert_version(model, 18) >>> onnx.checker.check_model(model) # success >>> sess = onnxruntime.InferenceSession(model.SerializeToString()) # fail >>> >>> sess <onnxruntime.capi.onnxruntime_inference_collection.InferenceSession object at 0x7f82dcce0a30> >>> >>> import onnxruntime as rt >>> import numpy as np >>> model_serialized = model.SerializeToString() >>> sess = rt.InferenceSession(model_serialized) >>> dummy_input = np.random.randn(1, 1, 28, 28).astype(np.float32) Perfo>>> >>> outputs = sess.run(output_names=['output1', 'output2'], ... input_feed={'input': dummy_input}) >>> print(outputs) [array([[[[ 4.02100086e-01, 1.91692543e+00, 5.60453236e-01, ..., 1.10894358e+00, -9.10616279e-01, 2.63174176e-01], [ 9.16809678e-01, 8.39324117e-01, 1.30101681e+00, ..., -6.86384559e-01, 8.50693703e-01, -2.23367453e-01], [ 9.90843654e-01, 1.80581224e+00, -6.30877018e-01, ..., -4.17214930e-02, 9.42695260e-01, -9.54421163e-01], ``` Inferencing seems to work. I should remove the old split 12-> 13 converters but I would like confirmation from a contributor before I move forward. --------- Signed-off-by: Danial Javady <danialjavady96@gmail.com> Signed-off-by: isdanni <leedanni@gmail.com>

Addresses onnx#5222 There does not exist a Split 12 from what I can see. I believe whomever made the conversion for Split 12 to Split 13 made a mistake and intended to do so for Split 11 to Split 13? IIUC, the conversions should chain. Running this with my built ONNX changes seems to work fine ``` >>> import onnx rt onnxruntime model = onnx.load('simple_model_split.onnx') model = onnx.version_converter.convert_version(model, 18) onnx.checker.check_model(model) # success sess = onnxruntime.InferenceSession(model.SerializeToString()) # fail>>> import onnxruntime >>> model = onnx.load('simple_model_split.onnx') >>> model = onnx.version_converter.convert_version(model, 18) >>> onnx.checker.check_model(model) # success >>> sess = onnxruntime.InferenceSession(model.SerializeToString()) # fail >>> >>> sess <onnxruntime.capi.onnxruntime_inference_collection.InferenceSession object at 0x7f82dcce0a30> >>> >>> import onnxruntime as rt >>> import numpy as np >>> model_serialized = model.SerializeToString() >>> sess = rt.InferenceSession(model_serialized) >>> dummy_input = np.random.randn(1, 1, 28, 28).astype(np.float32) Perfo>>> >>> outputs = sess.run(output_names=['output1', 'output2'], ... input_feed={'input': dummy_input}) >>> print(outputs) [array([[[[ 4.02100086e-01, 1.91692543e+00, 5.60453236e-01, ..., 1.10894358e+00, -9.10616279e-01, 2.63174176e-01], [ 9.16809678e-01, 8.39324117e-01, 1.30101681e+00, ..., -6.86384559e-01, 8.50693703e-01, -2.23367453e-01], [ 9.90843654e-01, 1.80581224e+00, -6.30877018e-01, ..., -4.17214930e-02, 9.42695260e-01, -9.54421163e-01], ``` Inferencing seems to work. I should remove the old split 12-> 13 converters but I would like confirmation from a contributor before I move forward. --------- Signed-off-by: Danial Javady <danialjavady96@gmail.com> Signed-off-by: Linsho Kaku <linsho@preferred.jp>

ZelboK requested a review from a team as a code owner March 13, 2024 14:50

justinchuby approved these changes Mar 15, 2024

View reviewed changes

ZelboK added 4 commits March 15, 2024 15:06

add 11 split conv to 13

795fc91

Signed-off-by: Danial Javady <danialjavady96@gmail.com>

add conversion from split 11 to 13 with test

cc035d5

Signed-off-by: Danial Javady <danialjavady96@gmail.com>

add new test and fix converter to actually transform the node rather …

1580813

…than returning a function that doesn't do it Signed-off-by: Danial Javady <danialjavady96@gmail.com>

format

97f7873

Signed-off-by: Danial Javady <danialjavady96@gmail.com>

ZelboK force-pushed the fix-split-11-18 branch from d1c6d78 to 97f7873 Compare March 15, 2024 19:06

justinchuby enabled auto-merge March 15, 2024 19:34

justinchuby added this pull request to the merge queue Mar 15, 2024

Merged via the queue into onnx:main with commit 9cc907f Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix conversion from split 11 to split 18. #6020

Fix conversion from split 11 to split 18. #6020

Uh oh!

ZelboK commented Mar 13, 2024

Uh oh!

codecov bot commented Mar 13, 2024 •

edited

Loading

Uh oh!

justinchuby commented Mar 13, 2024

Uh oh!

ZelboK commented Mar 13, 2024

Uh oh!

gramalingam commented Mar 14, 2024

Uh oh!

ZelboK commented Mar 15, 2024 •

edited

Loading

Uh oh!

justinchuby commented Mar 15, 2024 •

edited

Loading

Uh oh!

justinchuby left a comment

Uh oh!

justinchuby commented Mar 15, 2024

Uh oh!

ZelboK commented Mar 15, 2024

Uh oh!

Uh oh!

Fix conversion from split 11 to split 18. #6020

Fix conversion from split 11 to split 18. #6020

Uh oh!

Conversation

ZelboK commented Mar 13, 2024

Uh oh!

codecov bot commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

justinchuby commented Mar 13, 2024

Uh oh!

ZelboK commented Mar 13, 2024

Uh oh!

gramalingam commented Mar 14, 2024

Uh oh!

ZelboK commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinchuby commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinchuby left a comment

Choose a reason for hiding this comment

Uh oh!

justinchuby commented Mar 15, 2024

Uh oh!

ZelboK commented Mar 15, 2024

Uh oh!

Uh oh!

codecov bot commented Mar 13, 2024 •

edited

Loading

ZelboK commented Mar 15, 2024 •

edited

Loading

justinchuby commented Mar 15, 2024 •

edited

Loading