[G-API] Pipeline modeling tool - support local infer node config by TolyaTalamanov · Pull Request #22518 · opencv/opencv

TolyaTalamanov · 2022-09-15T10:52:12Z

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 Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the 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

Overview

The motivation of this PR is the following:

Some NN models might require individual config
Specifying config in .yml config file once is more convenient if it isn't supposed to be changed according user scenario.

The logic is following:

User can specify configuration parameters for every Infer node locally via .yml config file.
local config might be combined with global (in case it's specified via --load_config) until they have key conflicts.
In case "key conflict" app reports the problem and crash.

build_image:Custom=centos:7
buildworker:Custom=linux-1

TolyaTalamanov · 2022-09-15T10:52:49Z

@smirnov-alexey @dmatveev Hi folks! Could you have a look?

modules/gapi/samples/pipeline_modeling_tool.cpp

modules/gapi/samples/pipeline_modeling_tool/utils.hpp

smirnov-alexey

Looks good

dmatveev · 2022-09-15T19:26:38Z

modules/gapi/samples/pipeline_modeling_tool.cpp

                } else if (node_type == "Infer") {
                    auto infer_params = read<InferParams>(node_fn);
-                    infer_params.config = config;
+                    RETHROW_WITH_MSG_IF_FAILED(


Generally, macros are evil /=

I'll remove this macro and leave just try-catch there

dmatveev · 2022-09-15T19:27:02Z

modules/gapi/samples/pipeline_modeling_tool.cpp

-                    infer_params.config = config;
+                    RETHROW_WITH_MSG_IF_FAILED(
+                            utils::intersectMapWith(infer_params.config, gconfig),
+                            "Failed to combine global and local configs for Infer node: "


Why it could fail? How user could deal with it?

If user provides global and local configs that have intersection app fails with: Met dublicated key which is not descriptive. It would be great to see something like: Failed to merge global & local configs for model <model.xml>, but utils::intersectMapWith knows nothing about modes & configs it just merges map's

dmatveev · 2022-09-15T19:29:15Z

modules/gapi/samples/pipeline_modeling_tool/utils.hpp

+        std::stringstream ss;                      \
+        ss << msg << "\n caused by: " << e.what(); \
+        throw std::logic_error(ss.str());          \
+    }                                              \


What is the reason to do this? Maybe @sivanov-work have more to say here? :)

Answered above

Removed ugly macro

dmatveev · 2022-09-15T19:30:15Z

modules/gapi/samples/pipeline_modeling_tool/utils.hpp

+        if (it != target.end()) {
+            throw std::logic_error("Met already existing key: " + item.first);
+        }
+        target.insert(item);


In set theory, intersection is something different from this, isn't it?

Don't you just merge two maps into one with handling the "overwrites"?

Yes, good point!

dmatveev · 2022-09-15T19:30:51Z

modules/gapi/samples/pipeline_modeling_tool/utils.hpp

+    for (auto&& item : second) {
+        auto it = target.find(item.first);
+        if (it != target.end()) {
+            throw std::logic_error("Met already existing key: " + item.first);


"Error: key X already exists in Y" is better IMO.

Yes, but X & Y are nameless.

X is the value of your item.first.

Y is the map you're merging into.

This Y doesn't have a name. Done

* Rename intersectMapWith -> mergeMapWith * Remove macro * Add r-value ref

TolyaTalamanov · 2022-09-16T15:28:30Z

Is everybody OK to merge it?

TolyaTalamanov · 2022-09-26T08:25:37Z

modules/gapi/samples/pipeline_modeling_tool/utils.hpp


+template <typename K, typename V>
+void mergeMapWith(std::map<K, V>& target, const std::map<K, V>& second) {
+    for (auto&& item : second) {


/build/precommit_linux64/4.x/opencv/modules/gapi/samples/pipeline_modeling_tool.cpp:196:5: error: invalid initialization of non-const reference of type 'cv::FileNode&' from an rvalue of type 'cv::FileNode

Reverted back

TolyaTalamanov · 2022-09-26T10:15:46Z

@alalek @asmorkalov Can it be merged?

TolyaTalamanov · 2022-09-26T13:33:49Z

default job has passed

Support config as part of Infer node in yml

eb88831

TolyaTalamanov commented Sep 15, 2022

View reviewed changes

modules/gapi/samples/pipeline_modeling_tool.cpp Outdated Show resolved Hide resolved

asmorkalov requested a review from dmatveev September 15, 2022 11:54

asmorkalov added the category: g-api / gapi label Sep 15, 2022

smirnov-alexey reviewed Sep 15, 2022

View reviewed changes

modules/gapi/samples/pipeline_modeling_tool.cpp Show resolved Hide resolved

smirnov-alexey reviewed Sep 15, 2022

View reviewed changes

modules/gapi/samples/pipeline_modeling_tool/utils.hpp Outdated Show resolved Hide resolved

smirnov-alexey self-requested a review September 15, 2022 13:23

smirnov-alexey approved these changes Sep 15, 2022

View reviewed changes

dmatveev self-assigned this Sep 15, 2022

dmatveev added this to the 4.7.0 milestone Sep 15, 2022

dmatveev reviewed Sep 15, 2022

View reviewed changes

Apply comments

ec92f3f

* Rename intersectMapWith -> mergeMapWith * Remove macro * Add r-value ref

TolyaTalamanov commented Sep 26, 2022

View reviewed changes

Remove r-value ref

4521d66

TolyaTalamanov force-pushed the at/expand-modeling-tool-to-support-config-in-yml branch from 6a5f68c to 4521d66 Compare September 26, 2022 11:54

asmorkalov merged commit 3a64607 into opencv:4.x Sep 27, 2022

alalek mentioned this pull request Jan 8, 2023

(5.x) Merge 4.x #23113

Merged

Uh oh!

Conversation

TolyaTalamanov commented Sep 15, 2022 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Overview

Uh oh!

TolyaTalamanov commented Sep 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smirnov-alexey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TolyaTalamanov commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TolyaTalamanov commented Sep 26, 2022

Uh oh!

TolyaTalamanov commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TolyaTalamanov commented Sep 15, 2022 •

edited by alalek

Loading