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

Update unit as inference/sec #2352

Merged
merged 3 commits into from
Oct 23, 2023
Merged

Update unit as inference/sec #2352

merged 3 commits into from
Oct 23, 2023

Conversation

pramenku
Copy link
Contributor

As a user, I am trying to enable the unit self-explanatory.

Usually, we see unit format as /. Example: X images/sec, X frames/sec or etc where images or frames helping user to understand , what the value for.

Below Rate unit, until, I read doc or check with owner, I am not sure what it means. Just for a layman, if we keep like below, they might not get it. Rate: X/sec

But, if we keep, X inference/sec, it will help clearly to know what this value for. The Rate is how many inferences of the given inputs can be processed per second so X inference/sec will help user for the same.

For advance user, it /sec might be ok but I am trying to cover all.

As a user, I am trying to enable the unit self-explanatory.  

Usually, we see unit format as  <some value>  <what the value for>/<time>.
Example: X images/sec,  X frames/sec or etc where images or frames helping user to understand , what the value for.

Below Rate unit, until, I read doc or check with owner, I am not sure what it means. 
Just for a layman, if we keep like below, they might not get it.
Rate: X/sec

But, if we keep, X inference/sec, it will help clearly to know what this value for. 
The Rate is  how many inferences of the given inputs can be processed per second so X inference/sec will help user for the same.

For advance user, it /sec might be ok but I am trying to cover all.
@pramenku pramenku requested a review from causten October 20, 2023 16:29
@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #2352 (8b4c3ef) into develop (8f9ccb9) will not change coverage.
The diff coverage is 100.00%.

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

@@           Coverage Diff            @@
##           develop    #2352   +/-   ##
========================================
  Coverage    91.36%   91.36%           
========================================
  Files          440      440           
  Lines        16530    16530           
========================================
  Hits         15101    15101           
  Misses        1429     1429           
Files Coverage Δ
src/program.cpp 69.51% <100.00%> (ø)

@causten causten requested a review from pfultz2 October 20, 2023 18:47
src/program.cpp Outdated Show resolved Hide resolved
@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Oct 20, 2023

Test Batch Rate new
8b8f79
Rate old
8f9ccb
Diff Compare
torchvision-resnet50 64 2,855.55 nan nan%
torchvision-resnet50_fp16 64 6,483.67 6,483.21 0.01%
torchvision-densenet121 32 2,098.59 2,103.12 -0.22%
torchvision-densenet121_fp16 32 3,677.62 3,678.17 -0.02%
torchvision-inceptionv3 32 1,598.87 1,596.31 0.16%
torchvision-inceptionv3_fp16 32 2,596.37 2,591.76 0.18%
cadene-inceptionv4 16 707.20 707.90 -0.10%
cadene-resnext64x4 16 698.06 697.91 0.02%
slim-mobilenet 64 8,346.32 8,342.98 0.04%
slim-nasnetalarge 64 227.05 nan nan%
slim-resnet50v2 64 2,680.55 nan nan%
bert-mrpc-onnx 8 824.67 825.52 -0.10%
bert-mrpc-tf 1 389.48 388.70 0.20%
pytorch-examples-wlang-gru 1 300.26 300.63 -0.13%
pytorch-examples-wlang-lstm 1 308.14 310.40 -0.73%
torchvision-resnet50_1 1 602.58 602.49 0.01%
torchvision-inceptionv3_1 1 337.45 340.89 -1.01%
cadene-dpn92_1 1 393.12 397.15 -1.01%
cadene-resnext101_1 1 329.98 329.38 0.18%
slim-vgg16_1 1 465.31 464.92 0.08%
slim-mobilenet_1 1 1,363.00 2,039.93 -33.18% 🔴
slim-inceptionv4_1 1 217.07 217.64 -0.26%
onnx-taau-downsample 1 305.79 306.08 -0.09%
dlrm-criteoterabyte 1 21.68 21.69 -0.08%
dlrm-criteoterabyte_fp16 1 40.76 40.77 -0.02%
agentmodel 1 5,721.74 5,790.76 -1.19%
unet_fp16 2 55.99 56.03 -0.08%
resnet50v1_fp16 1 937.95 935.03 0.31%
bert_base_cased_fp16 64 971.38 971.13 0.03%
bert_large_uncased_fp16 32 305.25 305.13 0.04%
bert_large_fp16 1 167.09 167.02 0.04%
distilgpt2_fp16 16 1,279.50 1,279.37 0.01%

This build is not recommended to 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


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

    :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

    :white_check_mark:resnet50v1: PASSED: MIGraphX meets tolerance

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


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


    :white_check_mark:bert_large: PASSED: MIGraphX meets tolerance

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

@pramenku pramenku assigned pramenku and unassigned pramenku Oct 21, 2023
@pramenku
Copy link
Contributor Author

Hi @causten @umangyadav
Please feel free to merge as I don't have the permission.

After that need to put the same in master and later 6.0 release branch

@causten causten merged commit 7604ecf into develop Oct 23, 2023
15 checks passed
@causten causten deleted the pramenku-patch-1 branch October 23, 2023 13:03
@kahmed10
Copy link
Collaborator

This formatting looks wrong. Before it was /sec, which is fine to not have a space between the number. But now we need to modify it as such:

    os << "Rate: " << rate * batch << " inferences/sec" << std::endl;

@causten
Copy link
Collaborator

causten commented Oct 25, 2023

This formatting looks wrong. Before it was /sec, which is fine to not have a space between the number. But now we need to modify it as such:

    os << "Rate: " << rate * batch << " inferences/sec" << std::endl;

I agree...

Batch size: 16
Rate: 1337.84inferences/sec
Total time: 11.9596ms
Total instructions time: 12.9021ms
Overhead time: 0.0595123ms, -0.942473ms
Overhead: 0%, -8%

Honestly it would parse a lot easier if it looked like...

Batch size: 16
Rate (inf/sec): 1337.84
Total time (ms): 11.9596
Total instructions time (ms): 12.9021
Overhead time (ms): 0.0595123, -0.942473
Overhead: 0%, -8%

@umangyadav
Copy link
Member

Honestly it would parse a lot easier if it looked like...
Batch size: 16
Rate (inf/sec): 1337.84
Total time (ms): 11.9596
Total instructions time (ms): 12.9021
Overhead time (ms): 0.0595123, -0.942473
Overhead: 0%, -8%

I like that better. also i'll need to change my script to reflect this change now. I'll do that now.

https://github.com/ROCmSoftwarePlatform/YModel-Scripts/blob/90bc6e1dc1b600bc062cd3d55c0791ec6387c334/run_ymodel.sh#L147

@pramenku
Copy link
Contributor Author

pramenku commented Oct 26, 2023

Rate (inf/sec): 1337.84
Total time (ms): 11.9596
Total instructions time (ms): 12.9021
Overhead time (ms): 0.0595123, -0.942473

This also looks fine. Can we get this merged in develop, master and release 6.0 branch.

After that we need to also modify our automation code.

cc: @umangyadav @causten

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.

6 participants