Add python bindings for G-API onnx by xiong-jie-y · Pull Request #22017 · opencv/opencv

xiong-jie-y · 2022-05-20T12:25:09Z

Motivation

I think that the feature to run modle on onnxruntime is worth exposing to python G-API considering that

Onnxruntime supports many ML runtimes
Onnxruntime itself is very easy to install

This PR is the first step to expose onnxruntime features to python and support of execution providers will follow later.
This PR is draft, so let's have a discussion if necessary 🙂

Remaining tasks

Adding python unit test for onnx runtime
Create the same preprocess as openvino one to be able to accept the input with same shape as openvino. Currently transposing and reshaping is necessary like this in the user code.
-> I will not implement this because it seems that the implementation in the onnx seems right from this line.

Questions

How do you manage models for testing in opencv? I coudln't find the model used in infer_ssd_onnx.cpp test.

Example Code

https://github.com/xiong-jie-y/g_api_examples/blob/main/low_light_enhancement_onnx/enhance_low_light.py

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's no bug report.
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
I don't thik performance test is necessary this time, because it's just a binding.
The feature is well documented and sample code can be built with the project CMake

fengyuentau · 2022-05-21T12:38:29Z

How do you manage models for testing in opencv? I coudln't find the model used in infer_ssd_onnx.cpp test.

You can clone opencv_extra and run this script to get the models for testing.

xiong-jie-y · 2022-05-23T23:56:47Z

@fengyuentau
Thank you!
I wrote a test and it's ready for a detailed review 🙂

alalek

@xiong-jie-y Thank you for the contribution!

@dmatveev Could you please take a look on this proposal?

modules/gapi/include/opencv2/gapi/infer/bindings_onnx.hpp

modules/gapi/src/backends/onnx/bindings_onnx.cpp

xiong-jie-y · 2022-05-24T14:05:48Z

Does someone know how to fix this CI error?
In my understanding, I need to call onnx model downloader somewhere, but I couldn't find appropriate ci code 🙁
https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/100283/steps/test_python3/logs/stdio

alalek · 2022-05-24T14:32:03Z

modules/gapi/misc/python/test/test_gapi_infer_onnx.py

+    class test_gapi_infer(NewOpenCVTests):
+        def test_onnx_classification(self):
+            model_path = self.find_file(
+                    "vision/classification/squeezenet/model/squeezenet1.0-9.onnx",


DNN models are optional (available if OPENCV_DNN_TEST_DATA_PATH is specified).
Test should "skip" if file is not available.

Check this helper function: https://github.com/opencv/opencv/blob/4.5.5/modules/dnn/misc/python/test/test_dnn.py#L105-L110

There is missing onnx_models/ prefix also

model_path = self.find_file( "onnx_models/vision/classification/squeezenet/model/squeezenet1.0-9.onnx", [os.environ.get('OPENCV_DNN_TEST_DATA_PATH', os.getcwd()), os.environ['OPENCV_TEST_DATA_PATH']], required=False) if model_path is None: raise unittest.SkipTest("Missing DNN test file")

Finally, test should not fail with message G-API has been compiled without ONNX support.
Test should catch this cv.error, check its message and "skip" as unsupported configuration.

BTW, download .py file from "gapi" is not accurate. It doesn't support launching from opencv_extra. It must be launched from location pointed by OPENCV_DNN_TEST_DATA_PATH.

Thank you! Done! 🙂

@alalek, is behavior of download gapi .py incorrect? What is expected?

I’m out of context, sorry.

xiong-jie-y · 2022-06-02T10:36:15Z

@alalek @dmatveev
Sorry, I found a bug and fixed it about input_names and output_names.
Could you proceed the review if possible?

Also new features passes these tests.

./bin/opencv_test_gapi --gtest_filter="ONNX*"
./bin/opencv_test_core

xiong-jie-y · 2022-06-08T11:17:56Z

Hi @alalek,

Thank you for the previous review comment!

This PR is not very emergent, but I want to merge this PR soon because I have more PRs that depends on this PR.
Is there a way to finish this PR faster?
If there's any problem on my code, design...etc, feel free to tell me 🙂

alalek

Thank you for contribution!

alalek · 2022-06-09T18:24:00Z

modules/gapi/include/opencv2/gapi/infer/onnx.hpp


    std::vector<std::string> names_to_remap; //!< Names of output layers that will be processed in PostProc function.
+
+    bool is_generic;


Please add documentation for this flag.

Does it really needed? (I see only false initial value)

I believe it may be related to "generic infer" (with no network type defined), but let's @TolyaTalamanov to confirm

Yes, it's needed, because only generic infer can be exposed to python

xiong-jie-y · 2022-06-13T12:54:35Z

@alalek Thank you for the review!

Is it possible to assign one of the G-API developer as a second reviewer? 🙂
@dmatveev seems to unable to review now.

dmatveev · 2022-08-02T04:38:32Z

@TolyaTalamanov @mpashchenkov do you have any comments here?

dmatveev

Looks good to me. Thanks @xiong-jie-y for your work!

dmatveev · 2022-08-02T04:40:57Z

modules/gapi/include/opencv2/gapi/infer/onnx.hpp


    std::vector<std::string> names_to_remap; //!< Names of output layers that will be processed in PostProc function.
+
+    bool is_generic;


I believe it may be related to "generic infer" (with no network type defined), but let's @TolyaTalamanov to confirm

dmatveev · 2022-08-02T04:41:53Z

modules/gapi/include/opencv2/gapi/infer/onnx.hpp

+    @param model_path path to model file (.onnx file).
+    */
+    Params(const std::string& tag, const std::string& model_path)
+         : desc{model_path, 0u, 0u, {}, {}, {}, {}, {}, {}, {}, {}, {}, true}, m_tag(tag) {}


@alalek here is where is_generic is set to true (in the tail)

dmatveev · 2022-08-02T04:43:55Z

Sorry for delay @xiong-jie-y , we're all on sort-of-leave now

TolyaTalamanov

Technically implementation is correct, but I'd ask @mpashchenkov to check if generic onnx infer is handled properly.

TolyaTalamanov · 2022-08-09T11:28:19Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

+
+            GModel::Graph model(gr);
+            auto& op = model.metadata(nh).get<Op>();
+            if (pp.is_generic) {


@mpashchenkov Could review this part, please?

Looks like true (analogue of IE generic). But I think that tests are required.

TolyaTalamanov · 2022-08-09T11:31:45Z

modules/gapi/misc/python/test/test_gapi_infer_onnx.py

+
+    testdata_required = bool(os.environ.get('OPENCV_DNN_TEST_REQUIRE_TESTDATA', False))
+
+    class test_gapi_infer(NewOpenCVTests):


test_gapi_infer already exists, I'd change to test_gapi_onnx_infer

TolyaTalamanov · 2022-08-09T11:34:04Z

modules/gapi/include/opencv2/gapi/infer/onnx.hpp

+* @see struct Generic
+*/
+template<>
+class Params<cv::gapi::Generic> {


It'd be great to add test for generic onnx inference because it becomes core functionality for c++ users.

TolyaTalamanov · 2022-08-09T11:35:22Z

modules/gapi/include/opencv2/gapi/infer/onnx.hpp


    std::vector<std::string> names_to_remap; //!< Names of output layers that will be processed in PostProc function.
+
+    bool is_generic;


Yes, it's needed, because only generic infer can be exposed to python

mvskg · 2022-08-02T09:35:34Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

 }

-void ONNXCompiled::setOutput(int i, cv::Mat &m) {
+void ONNXCompiled::setOutput(int i, cv::Mat &m)


Unnecessary change.

mvskg · 2022-08-02T09:37:05Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

+                }
+                // Adding const input is necessary because the definition of input_names
+                // includes const input.
+                for (const auto& a : pp.const_inputs)


Is it necessary to add test for const input (generic case)?

ONNXbackend doesn’t have tests for generic infer.

mvskg · 2022-08-02T09:50:51Z

modules/gapi/misc/python/test/test_gapi_infer_onnx.py

+    class test_gapi_infer(NewOpenCVTests):
+        def test_onnx_classification(self):
+            model_path = self.find_file(
+                    "vision/classification/squeezenet/model/squeezenet1.0-9.onnx",


@alalek, is behavior of download gapi .py incorrect? What is expected?

I’m out of context, sorry.

mvskg · 2022-08-18T08:38:40Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

+            if (pp.is_generic) {
+                auto& info = cv::util::any_cast<cv::detail::InOutInfo>(op.params);
+
+                for (const auto& a : info.in_names)


Why do you use loops for assignment?

mvskg · 2022-08-18T08:39:50Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

+
+            GModel::Graph model(gr);
+            auto& op = model.metadata(nh).get<Op>();
+            if (pp.is_generic) {


Looks like true (analogue of IE generic). But I think that tests are required.

asmorkalov · 2022-10-03T10:47:36Z

@xiong-jie-y @mvskg Friendly reminder

mvskg · 2022-10-03T12:12:11Z

ONNX part looks like true but for "possible unexpected cases in the future" tests for generic part need to be added.

dmatveev · 2022-10-04T01:03:16Z

Is there anything else blocking this PR from merge?

TolyaTalamanov · 2022-10-04T06:24:50Z

Is there anything else blocking this PR from merge?

I guess only lack of tests. This PR introduces cv::gapi::Generic would be great to have c++ tests as well.
But on the other hand it works in python (based on tests).

dmatveev · 2022-10-04T06:34:44Z

I believe if there is python tests, that should be enough.
Anyway I doubt we have any ONNX builders now in the CI to test.

TolyaTalamanov

Thank you! 👍

fengyuentau added feature category: g-api / gapi labels May 21, 2022

alalek reviewed May 24, 2022

View reviewed changes

modules/gapi/include/opencv2/gapi/infer/bindings_onnx.hpp Outdated Show resolved Hide resolved

modules/gapi/src/backends/onnx/bindings_onnx.cpp Show resolved Hide resolved

alalek reviewed May 24, 2022

View reviewed changes

Add python bindings for G-API onnx

ea2527c

alalek reviewed Jun 9, 2022

View reviewed changes

dmatveev self-assigned this Aug 2, 2022

dmatveev approved these changes Aug 2, 2022

View reviewed changes

TolyaTalamanov reviewed Aug 9, 2022

View reviewed changes

mvskg reviewed Aug 18, 2022

View reviewed changes

TolyaTalamanov self-requested a review October 4, 2022 06:36

TolyaTalamanov approved these changes Oct 4, 2022

View reviewed changes

asmorkalov merged commit fef8d4c into opencv:4.x Oct 4, 2022

alalek mentioned this pull request Jan 8, 2023

(5.x) Merge 4.x #23113

Merged

dmatveev mentioned this pull request May 15, 2023

G-API: Integration branch for ONNX & Python-related changes #23597

Merged

6 tasks


		std::vector<std::string> names_to_remap; //!< Names of output layers that will be processed in PostProc function.

		bool is_generic;


		testdata_required = bool(os.environ.get('OPENCV_DNN_TEST_REQUIRE_TESTDATA', False))

		class test_gapi_infer(NewOpenCVTests):

Uh oh!

Conversation

xiong-jie-y commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Remaining tasks

Questions

Example Code

Pull Request Readiness Checklist

Uh oh!

fengyuentau commented May 21, 2022

Uh oh!

xiong-jie-y commented May 23, 2022

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xiong-jie-y commented May 24, 2022

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!

xiong-jie-y commented Jun 2, 2022

Uh oh!

xiong-jie-y commented Jun 8, 2022

Uh oh!

alalek 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!

xiong-jie-y commented Jun 13, 2022

Uh oh!

dmatveev commented Aug 2, 2022

Uh oh!

dmatveev 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!

dmatveev commented Aug 2, 2022

Uh oh!

TolyaTalamanov 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!

asmorkalov commented Oct 3, 2022

Uh oh!

xiong-jie-y commented May 20, 2022 •

edited

Loading