Skip to content

Remove NN Builder API#16906

Closed
l-bat wants to merge 29 commits intoopencv:3.4from
l-bat:remove_builder_api
Closed

Remove NN Builder API#16906
l-bat wants to merge 29 commits intoopencv:3.4from
l-bat:remove_builder_api

Conversation

@l-bat
Copy link
Copy Markdown
Contributor

@l-bat l-bat commented Mar 25, 2020 •
edited
Loading

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under OpenCV (BSD) License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake
force_builders=Custom,Custom Win,Custom Mac
build_image:Custom=ubuntu-openvino-2020.1.0:16.04
build_image:Custom Win=openvino-2020.1.0
build_image:Custom Mac=openvino-2020.1.0

Xtest_modules:Custom=dnn,python2,python3,java
test_modules:Custom Win=dnn,python2,python3,java
test_modules:Custom Mac=dnn,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

dkurt
dkurt reviewed Mar 25, 2020
View reviewed changes
dkurt
dkurt reviewed Mar 25, 2020
View reviewed changes
modules/dnn/test/test_backends.cpp
{
if (outputLayer == "detection_out")
{
if (backend == DNN_BACKEND_INFERENCE_ENGINE_NN_BUILDER_2019)
Copy link
Copy Markdown
Member

@dkurt dkurt Mar 25, 2020

Choose a reason for hiding this comment

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

Can you please check why nGraph path works correctly this way? I thought outputs from both APIs should be the same.

Copy link
Copy Markdown
Contributor Author

@l-bat l-bat Mar 30, 2020

Choose a reason for hiding this comment

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

It strange, but nGraph works in both cases

dkurt
dkurt reviewed Mar 25, 2020
View reviewed changes
modules/dnn/include/opencv2/dnn/dnn.hpp Outdated
virtual Ptr<BackendNode> initInfEngine(const std::vector<Ptr<BackendWrapper> > &inputs);

virtual Ptr<BackendNode> initNgraph(const std::vector<Ptr<BackendWrapper> > &inputs, const std::vector<Ptr<BackendNode> >& nodes);
virtual Ptr<BackendNode> initNgraph(const std::vector<Ptr<BackendNode> >& nodes);
Copy link
Copy Markdown
Member

@dkurt dkurt Mar 25, 2020

Choose a reason for hiding this comment

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

What do you think, it's better to rename it to initial initInfEngine? But let's discuss before change :)

@l-bat l-bat added this to the 3.4.11 milestone Mar 26, 2020
dkurt
dkurt reviewed Mar 27, 2020
View reviewed changes
modules/dnn/src/dnn.cpp Outdated
{
return backendId == DNN_BACKEND_OPENCV ||
(backendId == DNN_BACKEND_INFERENCE_ENGINE_NN_BUILDER_2019 && inputsData.size() == 1);
return backendId == DNN_BACKEND_OPENCV;
Copy link
Copy Markdown
Member

@dkurt dkurt Mar 27, 2020

Choose a reason for hiding this comment

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

We need to support this layer with nGraph as for me

Copy link
Copy Markdown
Contributor Author

@l-bat l-bat Mar 30, 2020

Choose a reason for hiding this comment

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

Ddo we need inputData.size () == 1condition?

Copy link
Copy Markdown
Member

@dkurt dkurt Mar 31, 2020

Choose a reason for hiding this comment

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

At least match DNN Builder implementation

dkurt
dkurt reviewed Mar 27, 2020
View reviewed changes
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 9, 2020

@l-bat @dkurt Do you plan to work on the patch in mean time? There are merge conflicts.

@asmorkalov asmorkalov mentioned this pull request Apr 9, 2020
Closed
2 tasks
@alalek
Copy link
Copy Markdown
Member

alalek commented Apr 9, 2020

Lets hold this activity for now.

@l-bat l-bat force-pushed the remove_builder_api branch from 0b5860f to d506147 Compare April 10, 2020 06:53
@l-bat l-bat force-pushed the remove_builder_api branch from d506147 to 9ac98f2 Compare April 10, 2020 08:40
@alalek alalek mentioned this pull request Jun 21, 2020
Closed
@JulienMaille
Copy link
Copy Markdown
Contributor

JulienMaille commented Jul 15, 2020

@alalek is there a way to compile current master with legacy IE disabled (and avoid linking with inference_engine_legacy.dll)?
Thanks

@alalek alalek mentioned this pull request Feb 9, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dkurt dkurt dkurt left review comments

Assignees

No one assigned

Labels

category: dnn RFC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@l-bat @asmorkalov @alalek @JulienMaille @dkurt