Skip to content
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

DNN/CUDA: Solve the bug of same shape broadcast with CUDA #23560

Merged
merged 1 commit into from May 16, 2023
Merged

DNN/CUDA: Solve the bug of same shape broadcast with CUDA #23560

merged 1 commit into from May 16, 2023

Conversation

WanliZhong
Copy link
Member

@WanliZhong WanliZhong commented Apr 28, 2023 •
edited

Merged after #23557

Fix yolo regression error mentioned in e3e1f70#commitcomment-110475963

After fix this bug, the brute force test mentioned in #23556 will all passed with CUDA. (different shape broadcast will fallback to cpu)

Test is added in PR #23557

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

@WanliZhong WanliZhong added bug category: gpu/cuda (contrib) OpenCV 4.0+: moved to opencv_contrib category: dnn labels Apr 28, 2023
@WanliZhong WanliZhong added this to the 4.8.0 milestone Apr 28, 2023
@WanliZhong
Copy link
Member Author

WanliZhong commented Apr 28, 2023 •
edited

@peters Hi, could you help to test if this pr will run yolov7-tiny correctly mentioned in e3e1f70#commitcomment-110475963? Thanks!

WanliZhong referenced this pull request Apr 28, 2023
@asmorkalov
Merge pull request #23528 from WanliZhong:issue23278
e3e1f70 
DNN/CUDA: make 'abcd op 1b11' broadcast eltwise operator support cuda
@peters
Copy link
Contributor

peters commented Apr 28, 2023

@WanliZhong This PR fixes the yolov7-tiny inference problem. All tests are now green on our side! Thank you :)

@WanliZhong
Copy link
Member Author

@peters That's good! Thanks!

@zihaomu zihaomu added the pr: needs test New functionality requires minimal tests set label May 5, 2023
zihaomu
zihaomu approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@zihaomu zihaomu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@zihaomu zihaomu added test and removed pr: needs test New functionality requires minimal tests set labels May 11, 2023
@WanliZhong
Copy link
Member Author

Test results after merged PR #23557 and this PR
image

@opencv-alalek
Copy link
Contributor

Lets re-enable disabled tests. They are fine on CPU target and pass on CUDA target (but they are slow in the current implementation).

On CUDA target they could be skipped (if performance is critical) in this way:

    if (backendId == DNN_BACKEND_CUDA && ...optional conditionals to select skipped tests...)
        applyTestTag(CV_TEST_TAG_DNN_SKIP_CUDA);

or

    if (backendId == DNN_BACKEND_CUDA && ...optional conditionals...)
        applyTestTag(CV_TEST_TAG_VERYLONG);

To manually run "skipped" tests by tags add this option: --test_tag_enable=dnn_skip_cuda / --test_tag_force=dnn_skip_cuda (see #21088 discussions for details and #12783 for implementation)

@WanliZhong
Copy link
Member Author

Thanks @opencv-alalek ! I will enable it.

@asmorkalov
Copy link
Contributor

@opencv-alalek All tests are green. Should I merge with enabled or disabled brute-force tets?

opencv-alalek
opencv-alalek reviewed May 16, 2023
View reviewed changes
modules/dnn/test/test_layers.cpp
@@ -1949,6 +1949,9 @@ struct Layer_Test_Eltwise_bcast : testing::TestWithParam<tuple<string, int, tupl
int backend = get<0>(backend_target);
int target = get<1>(backend_target);

if (backend == DNN_BACKEND_CUDA && dim > 4)
applyTestTag(CV_TEST_TAG_LONG);
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, Looks like no tests are skipped on CI.

CV_TEST_TAG_VERYLONG tests are skipped by default.
CV_TEST_TAG_LONG tests should be skipped on embedded platforms or coverage/valgrind build configurations (though extra parameter on CI).

Copy link
Member Author

Choose a reason for hiding this comment

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

Longer than 5s but less than 20s, so it should use LONG tag

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, lets run all CUDA tests.

@asmorkalov asmorkalov merged commit 59ca444 into opencv:4.x May 16, 2023
19 checks passed
@WanliZhong WanliZhong deleted the eltwise_cuda_bug branch May 16, 2023 12:33
@asmorkalov asmorkalov mentioned this pull request May 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@opencv-alalek opencv-alalek opencv-alalek approved these changes

@zihaomu zihaomu zihaomu approved these changes

@rogday rogday Awaiting requested review from rogday

Assignees
No one assigned
Labels
bug category: dnn category: gpu/cuda (contrib) OpenCV 4.0+: moved to opencv_contrib test
Projects
None yet
Milestone
4.8.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@WanliZhong @peters @opencv-alalek @asmorkalov @zihaomu