Skip to content

Remove add #1156

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove add #1156

wants to merge 1 commit into from

Conversation

ilyachur
Copy link
Contributor

This PR should be merged first: #1144

@ilyachur ilyachur force-pushed the feature/ichuraev/remove_add_v0 branch 3 times, most recently from 3ea6a6b to a7a56de Compare June 30, 2020 07:13
@ilyachur ilyachur changed the title Remove add v0 Remove add, all, allreduce v0 Jun 30, 2020
@ilyachur ilyachur force-pushed the feature/ichuraev/remove_add_v0 branch 7 times, most recently from 763c6a3 to dc345f2 Compare July 2, 2020 11:13
@ilyachur ilyachur changed the title Remove add, all, allreduce v0 Remove add Jul 2, 2020
@ilyachur ilyachur marked this pull request as ready for review July 2, 2020 11:15
@ilyachur ilyachur requested review from a team July 2, 2020 11:15
@ilyachur ilyachur force-pushed the feature/ichuraev/remove_add_v0 branch from dc345f2 to 0c2d8f4 Compare July 3, 2020 03:34
@ilyachur ilyachur requested review from tomdol and postrational July 3, 2020 03:35
@ilyachur ilyachur force-pushed the feature/ichuraev/remove_add_v0 branch from d1ef240 to b021d72 Compare July 3, 2020 06:46
tomdol
tomdol suggested changes Jul 3, 2020
View reviewed changes
ngraph/test/backend_api.cpp
Shape shape{2, 2};
auto A = make_shared<op::Parameter>(element::f32, shape);
auto B = make_shared<op::Parameter>(element::f32, shape);
auto f = make_shared<Function>(make_shared<op::Add>(A, B), ParameterVector{A, B});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should rather change the op in this test to a different op rather than removing it. It's not the addition operation itself itested here, it checks the save/load interface of a backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will change it. Thank you!

ngraph/test/provenance.cpp Outdated
@@ -601,89 +601,3 @@ TEST(provenance, opset0_downgrade_pass_topk)
};
traverse_nodes({topk_v0}, tag_check, as_node_vector(topk_v1->input_values()));
}

TEST(provenance, opset1_upgrade_pass_graph)
Copy link
Contributor

Choose a reason for hiding this comment

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

The upgade pass is still used in the tests. It's supposed to be removed when we verify and confirm that the tests don't use opset0 any more. I suggest to revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will recheck. Thank you!

ngraph/test/runtime/interpreter/int_executable.cpp
@@ -29,6 +29,7 @@
#include "ngraph/pass/manager.hpp"
#include "ngraph/serializer.hpp"
#include "ngraph/util.hpp"
#include "op/add.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this include is needed here.

ngraph/test/serialize.cpp
@@ -144,21 +144,6 @@ TEST(serialize, friendly_name)
}
#endif

TEST(serialize, existing_models)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test has nothing to do with the Add operation. Why is it removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Models from this test cannot be serialised, because model contains v0::Add op.

ngraph/test/serialize.cpp Outdated
@@ -197,26 +182,6 @@ TEST(serialize, constant)
EXPECT_TRUE(found);
}

TEST(benchmark, serialize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - why are you removing this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Model contains Add v0 operation

@ilyachur ilyachur force-pushed the feature/ichuraev/remove_add_v0 branch 3 times, most recently from c46e7d6 to bad0f70 Compare July 8, 2020 05:03
@ilya-lavrenov ilya-lavrenov added this to the 2021.1 milestone Jul 8, 2020
@ilya-lavrenov ilya-lavrenov added the category: Core OpenVINO Core (aka ngraph) label Jul 8, 2020
@ilyachur ilyachur force-pushed the feature/ichuraev/remove_add_v0 branch from bad0f70 to 85a70d2 Compare July 8, 2020 13:00
@ilyachur ilyachur marked this pull request as draft July 27, 2020 08:26
@openvino-pushbot openvino-pushbot added the category: IE Tests OpenVINO Test: plugins and common label Aug 11, 2020
@ilyachur ilyachur closed this Aug 20, 2020
@ilyachur ilyachur deleted the feature/ichuraev/remove_add_v0 branch August 20, 2020 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomdol tomdol tomdol requested changes

@postrational postrational Awaiting requested review from postrational

Assignees

@GlebKazantaev GlebKazantaev

Labels
category: Core OpenVINO Core (aka ngraph) category: IE Tests OpenVINO Test: plugins and common
Projects
None yet
Milestone
2021.1
Development

Successfully merging this pull request may close these issues.
5 participants
@ilyachur @tomdol @ilya-lavrenov @GlebKazantaev @openvino-pushbot