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.

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

Move ref tests under a separate directory and split them based on the type of operation #2106

Merged
merged 13 commits into from
Sep 6, 2023

Conversation

ahsan-ca
Copy link
Contributor

@ahsan-ca ahsan-ca commented Aug 22, 2023

This PR re-organizes ref tests. The following ref tests under the
test directory are moved to a separate ref directory under test:

ref_dot_op_test.cpp
ref_loop_test.cpp
ref_ops_nonstd_shape_test.cpp
ref_ops_test.cpp
ref_rnn_ops_test.cpp

Following changes are made to ref tests:

  • Tests have been stripped from ref_ops_test.cpp and
    ref_ops_nonstd_shape_test.cpp, and moved to separate
    files based on the type of operation.
    Other ref test files were renamed.

  • TEST_CASE blocks having multiple tests have been broken down to have a single test for each TEST_CASE block.

  • All the tests under ref are compiled to a single executable.

@ahsan-ca ahsan-ca self-assigned this Aug 22, 2023
@ahsan-ca ahsan-ca force-pushed the split-ref_ops_test branch from 42a4413 to 8b74613 Compare August 22, 2023 18:47
@ahsan-ca ahsan-ca linked an issue Aug 22, 2023 that may be closed by this pull request
@ahsan-ca ahsan-ca requested a review from umangyadav August 22, 2023 18:49
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #2106 (d0ee111) into develop (53c406b) will not change coverage.
Report is 1 commits behind head on develop.
The diff coverage is n/a.

❗ Current head d0ee111 differs from pull request most recent head b758626. Consider uploading reports for the commit b758626 to get more accurate results

@@           Coverage Diff            @@
##           develop    #2106   +/-   ##
========================================
  Coverage    91.43%   91.43%           
========================================
  Files          422      422           
  Lines        15771    15771           
========================================
  Hits         14420    14420           
  Misses        1351     1351           

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Aug 22, 2023

Test Batch Rate new
b75862
Rate old
53c406
Diff Compare
torchvision-resnet50 64 2,281.08 2,282.45 -0.06%
torchvision-resnet50_fp16 64 5,350.86 5,335.11 0.30%
torchvision-densenet121 32 1,836.72 1,837.49 -0.04%
torchvision-densenet121_fp16 32 3,395.63 3,384.41 0.33%
torchvision-inceptionv3 32 1,332.13 1,340.57 -0.63%
torchvision-inceptionv3_fp16 32 2,585.40 2,583.12 0.09%
cadene-inceptionv4 16 679.52 679.18 0.05%
cadene-resnext64x4 16 590.63 590.26 0.06%
slim-mobilenet 64 7,216.28 7,205.34 0.15%
slim-nasnetalarge 64 236.72 236.66 0.02%
slim-resnet50v2 64 2,526.32 2,525.02 0.05%
bert-mrpc-onnx 8 720.60 720.82 -0.03%
bert-mrpc-tf 1 390.79 390.57 0.05%
pytorch-examples-wlang-gru 1 303.35 303.94 -0.19%
pytorch-examples-wlang-lstm 1 306.20 314.25 -2.56%
torchvision-resnet50_1 1 556.26 560.67 -0.79%
torchvision-inceptionv3_1 1 306.71 306.12 0.19%
cadene-dpn92_1 1 342.15 348.83 -1.92%
cadene-resnext101_1 1 219.92 220.77 -0.39%
slim-vgg16_1 1 224.25 224.64 -0.18%
slim-mobilenet_1 1 1,473.27 1,458.82 0.99%
slim-inceptionv4_1 1 214.92 224.37 -4.21% 🔴
onnx-taau-downsample 1 247.86 247.60 0.11%
dlrm-criteoterabyte 1 21.66 21.66 -0.00%
dlrm-criteoterabyte_fp16 1 40.59 40.63 -0.08%
agentmodel 1 5,838.50 5,827.23 0.19%
unet_fp16 2 55.08 55.00 0.15%

This build is not recommended to merge 🔴

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Aug 22, 2023


    :white_check_mark:bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

    :white_check_mark:bert-mrpc-tf: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

    :white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

🔴torchvision-inceptionv3_1: FAILED: MIGraphX is not within tolerance - check verbose output


🔴cadene-dpn92_1: FAILED: MIGraphX is not within tolerance - check verbose output


    :white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

🔴slim-inceptionv4_1: FAILED: MIGraphX is not within tolerance - check verbose output


    :white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

    :white_check_mark:agentmodel: PASSED: MIGraphX meets tolerance

    :white_check_mark:unet: PASSED: MIGraphX meets tolerance

test/ref/CMakeLists.txt Outdated Show resolved Hide resolved
test/ref/main.cpp Outdated Show resolved Hide resolved
@ahsan-ca ahsan-ca requested a review from umangyadav August 24, 2023 16:39
test/ref/concat.cpp Outdated Show resolved Hide resolved
test/ref/gather.cpp Outdated Show resolved Hide resolved
test/ref/gathernd.cpp Outdated Show resolved Hide resolved
@ahsan-ca ahsan-ca requested a review from umangyadav August 29, 2023 15:58
@ahsan-ca ahsan-ca force-pushed the split-ref_ops_test branch from 645e4b3 to 0761a09 Compare August 29, 2023 19:54
@ahsan-ca ahsan-ca marked this pull request as ready for review August 29, 2023 19:55
@ahsan-ca ahsan-ca force-pushed the split-ref_ops_test branch from c4fe9d2 to 323d9be Compare August 29, 2023 21:23
Copy link
Member

@umangyadav umangyadav left a comment

Choose a reason for hiding this comment

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

Fix CI checks. Otherwise LGTM

@umangyadav umangyadav requested a review from pfultz2 August 30, 2023 12:02
@ahsan-ca ahsan-ca changed the title Split tests based on operator for ref_ops_test.cpp Move ref tests under a separate directory and split them based on the type of operation Aug 30, 2023

file(GLOB VERIFY_REF CONFIGURE_DEPENDS *.cpp)

add_executable(test_ref ${VERIFY_REF})
Copy link
Collaborator

@pfultz2 pfultz2 Aug 31, 2023

Choose a reason for hiding this comment

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

This should use add_test_executable, otherwise the tests wont be run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. I have made the change to use add_test_executable.

add_dependencies(tests test_ref)
add_dependencies(check test_ref)
target_link_libraries(test_ref migraphx migraphx_onnx migraphx_ref)
target_include_directories(test_ref PUBLIC ../include)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rest of these lines should already be handles by add_test_executable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched to use add_test_executable but without target_include_directories(test_ref PUBLIC ../include), it complains that it could not find test.hpp. So I have kept target_include_directories(test_ref PUBLIC ../include).

@ahsan-ca ahsan-ca requested a review from pfultz2 September 5, 2023 18:51
@causten causten merged commit a0894c2 into develop Sep 6, 2023
12 checks passed
@causten causten deleted the split-ref_ops_test branch September 6, 2023 21:37
@bpickrel bpickrel restored the split-ref_ops_test branch September 11, 2023 21:11
@bpickrel bpickrel deleted the split-ref_ops_test branch September 11, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split ref_ops_test into multiple files
5 participants