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

Fixed stringutils test #2154

Merged
merged 2 commits into from
Sep 11, 2023
Merged

Fixed stringutils test #2154

merged 2 commits into from
Sep 11, 2023

Conversation

tvukovic-amd
Copy link
Collaborator

Fixed stringutils test with unbalanced brackets

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Sep 6, 2023

Test Batch Rate new
6e5f95
Rate old
e7f533
Diff Compare
torchvision-resnet50 64 2,285.46 2,283.19 0.10%
torchvision-resnet50_fp16 64 5,378.06 5,360.94 0.32%
torchvision-densenet121 32 1,834.37 1,815.17 1.06%
torchvision-densenet121_fp16 32 3,387.28 3,395.37 -0.24%
torchvision-inceptionv3 32 1,341.98 1,337.73 0.32%
torchvision-inceptionv3_fp16 32 2,570.92 2,588.31 -0.67%
cadene-inceptionv4 16 682.56 682.02 0.08%
cadene-resnext64x4 16 589.90 589.99 -0.01%
slim-mobilenet 64 7,216.70 7,223.31 -0.09%
slim-nasnetalarge 64 236.98 237.08 -0.04%
slim-resnet50v2 64 2,528.74 2,530.58 -0.07%
bert-mrpc-onnx 8 721.36 721.35 0.00%
bert-mrpc-tf 1 391.02 389.11 0.49%
pytorch-examples-wlang-gru 1 302.85 300.75 0.70%
pytorch-examples-wlang-lstm 1 309.49 311.33 -0.59%
torchvision-resnet50_1 1 559.26 557.00 0.41%
torchvision-inceptionv3_1 1 306.64 305.84 0.26%
cadene-dpn92_1 1 353.69 350.58 0.89%
cadene-resnext101_1 1 220.38 220.17 0.10%
slim-vgg16_1 1 224.44 224.43 0.00%
slim-mobilenet_1 1 1,495.70 1,494.20 0.10%
slim-inceptionv4_1 1 223.75 221.91 0.83%
onnx-taau-downsample 1 248.05 247.91 0.06%
dlrm-criteoterabyte 1 21.68 21.68 0.00%
dlrm-criteoterabyte_fp16 1 40.62 40.63 -0.04%
agentmodel 1 5,868.30 5,725.74 2.49%
unet_fp16 2 55.01 55.08 -0.14%

This build is OK for merge ✅

@migraphx-bot
Copy link
Collaborator


    :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

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Merging #2154 (fe8ffe5) into develop (37787e0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff            @@
##           develop    #2154   +/-   ##
========================================
  Coverage    91.48%   91.48%           
========================================
  Files          424      424           
  Lines        15871    15873    +2     
========================================
+ Hits         14519    14521    +2     
  Misses        1352     1352           
Files Changed Coverage Δ
src/include/migraphx/stringutils.hpp 98.52% <100.00%> (+0.04%) ⬆️

@causten causten requested a review from pfultz2 September 6, 2023 14:07
@pfultz2
Copy link
Collaborator

pfultz2 commented Sep 6, 2023

Can you add a unit test? The unit tests for this function are here: https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/develop/test/stringutils.cpp

@apwojcik apwojcik added the Windows Related changes for Windows Environments label Sep 6, 2023
@tvukovic-amd
Copy link
Collaborator Author

This fix is for fixing test case issue where brackets are unbalanced by changing src/include/migraphx/stringutils.hpp. Unit test for case where inputs are correct (which checks if output is correct) and test which throws error when input has unbalanced brackets already exists.
The test where inputs are correct is here and test where input has unbalanced brackets and throws an error is here.

@pfultz2
Copy link
Collaborator

pfultz2 commented Sep 7, 2023

Unit test for case where inputs are correct (which checks if output is correct) and test which throws error when input has unbalanced brackets already exists.

If we already have a test, then why didnt we catch that now when we run the tests?

@tvukovic-amd
Copy link
Collaborator Author

I built MIGraphX on Ubuntu20.04 and used develop branch and this test failed. When I rebased my changes on top of develop branch, test_stringutils also failed, but with my changes test on Windows is working. It is not related to OS, this fix is related to algorithm issue:

The test fails on interpolate_string_unbalanced case throwing exception with message cannot seek string iterator after end, after the string iterator exceeds a valid range.

Part of the code needed to be rewritten, since in its current form it uses undefined behaviour as a condition for a while loop
while(it != input.end()), so I added case to throw error if we didn't find closed brackets before input end.

@causten causten merged commit dfd95c5 into develop Sep 11, 2023
12 checks passed
@causten causten deleted the fix/test_stringutils branch September 11, 2023 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows Related changes for Windows Environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants