dnn: cleanup of halide backend for 5.x by fengyuentau · Pull Request #24231 · opencv/opencv

fengyuentau · 2023-09-06T09:06:46Z

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

modules/dnn/include/opencv2/dnn/dnn.hpp

dkurt · 2023-09-06T10:41:50Z

modules/dnn/test/test_common.hpp


 testing::internal::ParamGenerator< tuple<Backend, Target> > dnnBackendsAndTargets(
        bool withInferenceEngine = true,
-        bool withHalide = false,


Might break the tests. Can we just check for CV_Assert(!withHalide)?

I can review every location where dnnBackendsAndTargets is used, and make changes if needed. Let me try.

All dnnBackendsAndTargets called with parameters should be taken well care now.

Sounds good, thanks

I can review every location

This is not one time activity.
In case of parameter change we need to review each of "(5.x) Merge 4.x" PRs.
It is much safer to rename this parameter to bool obsolete_withHalide = false.

Again, fixing it here will not resolve merging 4.x->5.x issues (no errors would be raised in the most part of cases) which would lead to mistakes.

So how about removing the parameter from 4.x like what we want to do with modules/dnn/test/test_halide_layers.cpp? Does this address your concern?

So how do we do for this issue?

Series of boolean parameters is anti-pattern, especially in form with optional default values.
Any obvious refactoring is dangerous here, especially in case of regular merges between branches.

It is much safer to rename this parameter to bool obsolete_withHalide = false

It is much safer to rename this parameter to bool obsolete_withHalide = false

Okay, I will do this. I guess we can safely remove this legacy once our main dev branch switches to 5.x, right?

modules/dnn/test/test_common.impl.hpp

modules/dnn/test/test_halide_layers.cpp

asmorkalov · 2023-09-15T05:25:38Z

@fengyuentau There are merge conflicts after 4.x->5.x merge. Please take a look.

fengyuentau · 2023-09-15T07:50:50Z

Thank you for the notification. Conflicts are resolved.

dkurt

Thanks!

fengyuentau · 2023-09-15T08:32:48Z

Wait, I am doing the merge. Shouldn't we merge test_halide_layers.cpp into test_backends.cpp?

dkurt · 2023-09-15T08:37:19Z

@fengyuentau, that would be great, please do.

wait for tests merge

opencv-alalek

On tech meeting there were an agreement on these steps:

on 4.x branch refactor modules/dnn/test/test_halide_layers.cpp to extract "generic layers tests"
merge 4.x to 5.x
update PR with halide drop from 5.x

Such backport is necessary to avoid future 4.x->5.x merge conflicts.

/cc @asmorkalov

fengyuentau · 2023-09-18T08:33:37Z

On tech meeting there were an agreement on these steps:

on 4.x branch refactor modules/dnn/test/test_halide_layers.cpp to extract "generic layers tests"

merge 4.x to 5.x

update PR with halide drop from 5.x

Such backport is necessary to avoid future 4.x->5.x merge conflicts.

/cc @asmorkalov

Just submitted a pr for this #24283

dnn: merge tests from test_halide_layers to test_backends #24283 Context: #24231 (review) ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake

fengyuentau · 2023-09-28T16:35:54Z

Waiting for a 4.x -> 5.x merge containing #24283.

asmorkalov · 2023-10-05T06:43:04Z

@fengyuentau I merged 4x->5x and Halide related part has been merged.

…Test_layers_backends

dnn: merge tests from test_halide_layers to test_backends opencv#24283 Context: opencv#24231 (review) ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake

asmorkalov · 2023-10-12T06:47:58Z

@dkurt @opencv-alalek Friendly reminder.

dkurt · 2023-10-12T11:01:07Z

modules/gapi/doc/slides/gapi_overview.org

-  platform details.
- More than Halide:
-  - Kernels can be written in unconstrained platform-native code;
-  - Halide can serve as a backend (one of many).


Please keep this reference. Not related to Halide in dnn.

BTW, @dmatveev, does G-API even support Halide? If not, only some statements are actual:

- Similar to Halide -- separates logic from platform details. - More than Halide: - Kernels can be written in unconstrained platform-native code;

I basically revert every gapi doc change about halide.

asmorkalov · 2023-10-13T09:38:31Z

@opencv-alalek Please take a look again.

opencv-alalek

LGTM 👍

dnn: merge tests from test_halide_layers to test_backends opencv#24283 Context: opencv#24231 (review) ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake

fengyuentau added category: dnn cleanup Code cleanup (e.g, drop legacy C-API, legacy unmaintained code) labels Sep 6, 2023

fengyuentau added this to the 5.0 milestone Sep 6, 2023

fengyuentau assigned asmorkalov Sep 6, 2023

fengyuentau mentioned this pull request Sep 6, 2023

dnn: cleanup of Halide backend #24123

Closed

7 tasks

dkurt reviewed Sep 6, 2023

View reviewed changes

modules/dnn/include/opencv2/dnn/dnn.hpp Show resolved Hide resolved

dkurt reviewed Sep 6, 2023

View reviewed changes

modules/dnn/test/test_common.impl.hpp Show resolved Hide resolved

dkurt reviewed Sep 6, 2023

View reviewed changes

modules/dnn/test/test_halide_layers.cpp Outdated Show resolved Hide resolved

asmorkalov assigned dkurt and unassigned asmorkalov Sep 7, 2023

dkurt reviewed Sep 14, 2023

View reviewed changes

modules/dnn/test/test_halide_layers.cpp Outdated Show resolved Hide resolved

dkurt reviewed Sep 14, 2023

View reviewed changes

modules/dnn/test/test_halide_layers.cpp Outdated Show resolved Hide resolved

fengyuentau force-pushed the halide_cleanup_5.x branch from 4797e80 to 51660b1 Compare September 15, 2023 07:47

dkurt previously approved these changes Sep 15, 2023

View reviewed changes

opencv-alalek reviewed Sep 15, 2023

View reviewed changes

fengyuentau mentioned this pull request Sep 18, 2023

dnn: merge tests from test_halide_layers to test_backends #24283

Merged

6 tasks

fengyuentau added 4 commits October 5, 2023 16:33

rebased first commit

44d8328

enum compatibility

ad9d3cd

take care of dnnBackendsAndTargets defaults

129c1b1

restore test_halide_layers.cpp with compatible changes

8b86ed6

fengyuentau added 3 commits October 5, 2023 16:33

remove dnnBackendsAndTargetsWithHalide; rename Test_Halide_layers to …

61ca1f6

…Test_layers_backends

correct conflict

dbca49c

merge test_halide_layers.cpp into test_backends.cpp

f64a11c

fengyuentau force-pushed the halide_cleanup_5.x branch from 4a97f64 to f64a11c Compare October 5, 2023 08:37

dkurt reviewed Oct 12, 2023

View reviewed changes

restore gapi docs about halide

162a399

dkurt approved these changes Oct 13, 2023

View reviewed changes

keep obsolete_withHalide

521b0ab

opencv-alalek approved these changes Oct 13, 2023

View reviewed changes

asmorkalov merged commit d789cb4 into opencv:5.x Oct 13, 2023

fengyuentau deleted the halide_cleanup_5.x branch June 28, 2024 09:57

Uh oh!

Conversation

fengyuentau commented Sep 6, 2023

Pull Request Readiness Checklist

Uh oh!

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!

fengyuentau Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asmorkalov commented Sep 15, 2023

Uh oh!

fengyuentau commented Sep 15, 2023

Uh oh!

dkurt left a comment

Choose a reason for hiding this comment

Uh oh!

fengyuentau commented Sep 15, 2023

Uh oh!

dkurt commented Sep 15, 2023

Uh oh!

opencv-alalek left a comment

Choose a reason for hiding this comment

Uh oh!

fengyuentau commented Sep 18, 2023 • edited by dkurt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fengyuentau commented Sep 28, 2023

Uh oh!

asmorkalov commented Oct 5, 2023

Uh oh!

asmorkalov commented Oct 12, 2023

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 13, 2023

Uh oh!

opencv-alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fengyuentau Sep 18, 2023 •

edited

Loading

fengyuentau commented Sep 18, 2023 •

edited by dkurt

Loading