[ONNX] Extend ONNX Frontend with `com.microsoft.Pad` #22000

siddhant-0707 · 2024-01-06T11:08:22Z

Details:

created implementation for com.microsoft.Pad
creating test

Tickets:

Closes Extend ONNX Frontend with com.microsoft.Pad operator #17576

github-actions · 2024-01-29T00:17:47Z

This PR will be closed in a week because of 2 weeks of no activity.

github-actions · 2024-02-16T00:17:20Z

This PR will be closed in a week because of 2 weeks of no activity.

mitruska

Hello @siddhant-0707, thank you for your contribution!

MS ONNX Pad and ONNX standard Pad-11 seem to be compatible. The only difference I can see is that MS Pad allows pads input to be 2D, then the first dim is required to be 1, so it could be simply resolved by Squeeze. My suggestion is to reuse set_11::pad code within a common function to avoid duplication of the main logic. Squeeze can be applied to the MS pads input if needed, before the common part.

Also this PR doesn't have any tests for the provided changes, please add them including the mentioned 2D pads scenario.
Registration of the correct pad function should be ensured.

mitruska · 2024-02-16T13:33:22Z

src/frontends/onnx/frontend/src/ops_bridge.cpp

@@ -585,6 +585,7 @@ OperatorsBridge::OperatorsBridge() {
    REGISTER_OPERATOR_WITH_DOMAIN(MICROSOFT_DOMAIN, "FusedConv", 1, fused_conv);
    REGISTER_OPERATOR_WITH_DOMAIN(MICROSOFT_DOMAIN, "FusedGemm", 1, fusedgemm);
    REGISTER_OPERATOR_WITH_DOMAIN(MICROSOFT_DOMAIN, "EmbedLayerNormalization", 1, embed_layer_normalization);
+    REGISTER_OPERATOR_WITH_DOMAIN(MICROSOFT_DOMAIN, "Pad", 1, pad);


The new file src/frontends/onnx/frontend/src/op/com.microsoft/pad.hpp is not included here, so the created function is not visible.
It means that set_1::pad from /onnx/frontend/src/op/pad.hpp has been registered instead.

openvino/src/frontends/onnx/frontend/src/op/pad.cpp

Lines 39 to 40 in ea3601e

namespace set_1 {

ov::OutputVector pad(const ov::frontend::onnx::Node& node) {

To distinguish the pad function dedicated for conversion of MS Op, my suggestion is to put it in a different namespace or change name of the function, and register like:

Suggested change

REGISTER_OPERATOR_WITH_DOMAIN(MICROSOFT_DOMAIN, "Pad", 1, pad);

register_operator_in_custom_domain("Pad", VersionRange::single_version_for_all_opsets(), op::custom::set_1::pad, MICROSOFT_DOMAIN);

mitruska · 2024-02-16T15:19:56Z

src/frontends/onnx/frontend/src/op/com.microsoft/pad.cpp

+        throw ngraph_error("Unsupported pad_mode in ONNX com.microsoft.Pad operator");
+    }
+
+    auto pads_shape = pads.get_shape();


The .get_shape() will throw for dynamic shape, it should be used only if static shape is ensured,
.get_partial_shape() can be used instead.
For partial shape .is_static() or .is_dynamic() can be used and .size() is safe only for shapes with static rank.
To check whether the rank is static: partial_shape.rank().is_static().

github-actions · 2024-03-04T00:17:52Z

This PR will be closed in a week because of 2 weeks of no activity.

github-actions · 2024-03-11T00:18:18Z

This PR was closed because it has been stalled for 2 week with no activity.

mlukasze · 2024-03-19T08:37:15Z

hey @siddhant-0707 :)
will you continue a work on this PR? Or should we move a task back to the open pool?

siddhant-0707 · 2024-03-29T17:10:52Z

Apologies for the delay @mlukasze, I was a little occupied with other tasks. Please reopen the PR.
@mitruska I have reused the implementation of set_11::pad (with modification in beginning to handle case with 2d pads input). I've done this because we have to assign the squeezed value back to node.get_ng_inputs()[1] (which we can't do) Is there a way to create a new node and set its inputs and pass it to set_11::pad?.
Another way this problem can be solved is creating an overload in onnx::pad that takes in pads separately and then passes it to set_11::pad. Something like ov::OutputVector pad(const Node& node, const ov::Output<ov::Node>& pads) {}. However, this would misalign code with docs I think.

siddhant-0707 · 2024-03-29T17:11:02Z

Also added 2d and 1d tests

mitruska · 2024-03-29T22:32:15Z

Hello @siddhant-0707, great to see some updates! Reopened the PR as you asked.
To continue review, please push commits with the changes you have mentioned and sync branch with the latest master.

gkrivor · 2024-04-03T11:09:48Z

build_jenkins

mitruska

My previous concern (#22000 (comment)) about get_shape() call has been not applied.
Please add suggested change to avoid further issues.

About creating the common pad function, it could be a helper taking ov::OutputVector as argument, and called from the onnx standard pad and custom pad (after get_ov_inputs()).
But such code unification can be considered as follow up improvement within a separate PR.

Let's focus on the last necessary change:)

src/frontends/onnx/frontend/src/op/com.microsoft/pad.cpp

Co-authored-by: Katarzyna Mitrus <katarzyna.mitrus@intel.com>

mitruska · 2024-04-03T13:34:39Z

build_jenkins

mitruska · 2024-04-04T13:05:29Z

build_jenkins

…#22000) ### Details: - created implementation for `com.microsoft.Pad` - creating test ### Tickets: - Closes openvinotoolkit#17576 --------- Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com> Co-authored-by: Katarzyna Mitrus <katarzyna.mitrus@intel.com>

enabled ONNX com.microsoft.Pad

25ff7ba

siddhant-0707 requested a review from a team as a code owner January 6, 2024 11:08

github-actions bot added the category: ONNX FE OpenVINO ONNX FrontEnd label Jan 6, 2024

siddhant-0707 mentioned this pull request Jan 6, 2024

Extend ONNX Frontend with com.microsoft.Pad operator #17576

Closed

ilya-lavrenov added ExternalPR External contributor pr: needs tests PR needs tests updating labels Jan 8, 2024

github-actions bot added the Stale label Jan 29, 2024

Merge branch 'openvinotoolkit:master' into issue17576

c63e240

github-actions bot removed the Stale label Jan 30, 2024

ilya-lavrenov assigned gkrivor Feb 1, 2024

siddhant-0707 mentioned this pull request Feb 1, 2024

ONNX microsoft range operator #21653

Closed

github-actions bot added the Stale label Feb 16, 2024

mitruska requested changes Feb 16, 2024

View reviewed changes

mitruska self-assigned this Feb 16, 2024

github-actions bot removed the Stale label Feb 17, 2024

github-actions bot added the Stale label Mar 4, 2024

github-actions bot closed this Mar 11, 2024

siddhant-0707 added 5 commits March 29, 2024 12:52

add tests

6cee0c1

edit

20a2bd1

edit

c097058

Update pad.cpp

6c8c132

add code from onnx::pad

602618c

mitruska reopened this Mar 29, 2024

github-actions bot removed the Stale label Mar 30, 2024

siddhant-0707 added 2 commits March 30, 2024 13:22

Merge branch 'master' into issue17576

1f40874

Update onnx_import_com_microsoft.in.cpp

803baa1

siddhant-0707 requested a review from mitruska March 30, 2024 07:58

gkrivor added 9 commits April 2, 2024 13:20

Merge branch 'master' into issue17576

cccbdc5

Update pad.hpp: fix after refactoring

9215878

Update pad.cpp: refactoring artefact

4935715

Update pad.cpp: split was moved

2f00dd3

Update pad.cpp: namespace

ba3673b

Update pad.hpp: namespace

39c19c1

Update pad.cpp: refactoring

3f0abb6

Update pad.hpp: refactoring...

4eb78f9

Update pad.hpp: code style

1762b6d

gkrivor approved these changes Apr 3, 2024

View reviewed changes

mitruska reviewed Apr 3, 2024

View reviewed changes

src/frontends/onnx/frontend/src/op/com.microsoft/pad.cpp Outdated Show resolved Hide resolved

use .get_partial_shape()

2c81646

Co-authored-by: Katarzyna Mitrus <katarzyna.mitrus@intel.com>

mitruska approved these changes Apr 3, 2024

View reviewed changes

Merge branch 'master' into issue17576

33f7404

mitruska removed the pr: needs tests PR needs tests updating label Apr 4, 2024

gkrivor added this pull request to the merge queue Apr 5, 2024

Merged via the queue into openvinotoolkit:master with commit 10c0d5b Apr 5, 2024
108 checks passed

mlukasze added this to the 2024.2 milestone Apr 5, 2024

p-wysocki mentioned this pull request May 23, 2024

[Good First Issue]: Extend ONNX Frontend with DeformConv operator #24649

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ONNX] Extend ONNX Frontend with `com.microsoft.Pad` #22000

[ONNX] Extend ONNX Frontend with `com.microsoft.Pad` #22000

siddhant-0707 commented Jan 6, 2024 •

edited by ilya-lavrenov

Loading

github-actions bot commented Jan 29, 2024

github-actions bot commented Feb 16, 2024

mitruska left a comment

mitruska Feb 16, 2024

mitruska Feb 16, 2024

github-actions bot commented Mar 4, 2024

github-actions bot commented Mar 11, 2024

mlukasze commented Mar 19, 2024

siddhant-0707 commented Mar 29, 2024

siddhant-0707 commented Mar 29, 2024

mitruska commented Mar 29, 2024

gkrivor commented Apr 3, 2024

mitruska left a comment

mitruska commented Apr 3, 2024

mitruska commented Apr 4, 2024

	namespace set_1 {
	ov::OutputVector pad(const ov::frontend::onnx::Node& node) {

	REGISTER_OPERATOR_WITH_DOMAIN(MICROSOFT_DOMAIN, "Pad", 1, pad);
	register_operator_in_custom_domain("Pad", VersionRange::single_version_for_all_opsets(), op::custom::set_1::pad, MICROSOFT_DOMAIN);

[ONNX] Extend ONNX Frontend with com.microsoft.Pad #22000

[ONNX] Extend ONNX Frontend with com.microsoft.Pad #22000

Conversation

siddhant-0707 commented Jan 6, 2024 • edited by ilya-lavrenov Loading

Details:

Tickets:

github-actions bot commented Jan 29, 2024

github-actions bot commented Feb 16, 2024

mitruska left a comment

Choose a reason for hiding this comment

mitruska Feb 16, 2024

Choose a reason for hiding this comment

mitruska Feb 16, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 4, 2024

github-actions bot commented Mar 11, 2024

mlukasze commented Mar 19, 2024

siddhant-0707 commented Mar 29, 2024

siddhant-0707 commented Mar 29, 2024

mitruska commented Mar 29, 2024

gkrivor commented Apr 3, 2024

mitruska left a comment

Choose a reason for hiding this comment

mitruska commented Apr 3, 2024

mitruska commented Apr 4, 2024

[ONNX] Extend ONNX Frontend with `com.microsoft.Pad` #22000

[ONNX] Extend ONNX Frontend with `com.microsoft.Pad` #22000

siddhant-0707 commented Jan 6, 2024 •

edited by ilya-lavrenov

Loading