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 lib target soversion #2267

Merged
merged 3 commits into from
Oct 20, 2023
Merged

Update lib target soversion #2267

merged 3 commits into from
Oct 20, 2023

Conversation

arvindcheru
Copy link
Contributor

Summary of proposed changes:

  • Update lib target so version format to recommended format - (ex: libmigraphx.so.2.7.0 updated to libmigraphx.so.2.7)

@pfultz2
Copy link
Collaborator

pfultz2 commented Sep 30, 2023

Why are we dropping the patch version?

@arvindcheru
Copy link
Contributor Author

Why are we dropping the patch version?
Looking latest versions majority of modules are having soversion with major,minor based link names. there are 3 other modules including migraphx were only showing difference by having patch version also. these 4 modules are requested to update using these changes proposed to the recommended soversion format.

@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

Merging #2267 (70e72b1) into develop (e748657) will increase coverage by 0.09%.
The diff coverage is n/a.

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

@@             Coverage Diff             @@
##           develop    #2267      +/-   ##
===========================================
+ Coverage    91.37%   91.46%   +0.09%     
===========================================
  Files          439      433       -6     
  Lines        16493    16174     -319     
===========================================
- Hits         15069    14792     -277     
+ Misses        1424     1382      -42     

see 13 files with indirect coverage changes

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Sep 30, 2023

Test Batch Rate new
d51632
Rate old
883732
Diff Compare
torchvision-resnet50 64 2,325.71 2,323.99 0.07%
torchvision-resnet50_fp16 64 5,350.90 5,358.70 -0.15%
torchvision-densenet121 32 1,849.08 1,847.03 0.11%
torchvision-densenet121_fp16 32 3,417.04 3,410.48 0.19%
torchvision-inceptionv3 32 1,295.60 1,295.72 -0.01%
torchvision-inceptionv3_fp16 32 2,537.61 2,536.60 0.04%
cadene-inceptionv4 16 620.49 620.02 0.08%
cadene-resnext64x4 16 589.13 588.80 0.06%
slim-mobilenet 64 7,219.21 7,219.83 -0.01%
slim-nasnetalarge 64 236.43 236.41 0.01%
slim-resnet50v2 64 2,557.41 2,559.45 -0.08%
bert-mrpc-onnx 8 824.88 824.99 -0.01%
bert-mrpc-tf 1 389.80 387.19 0.67%
pytorch-examples-wlang-gru 1 299.90 299.70 0.07%
pytorch-examples-wlang-lstm 1 311.71 316.36 -1.47%
torchvision-resnet50_1 1 548.38 543.92 0.82%
torchvision-inceptionv3_1 1 307.60 305.91 0.55%
cadene-dpn92_1 1 351.64 353.11 -0.42%
cadene-resnext101_1 1 219.68 219.09 0.27%
slim-vgg16_1 1 224.20 224.68 -0.21%
slim-mobilenet_1 1 1,532.56 1,495.14 2.50%
slim-inceptionv4_1 1 218.26 216.37 0.87%
onnx-taau-downsample 1 304.50 306.35 -0.60%
dlrm-criteoterabyte 1 21.71 21.68 0.13%
dlrm-criteoterabyte_fp16 1 40.75 40.75 0.02%
agentmodel 1 5,861.80 5,869.23 -0.13%
unet_fp16 2 55.19 55.16 0.04%
resnet50v1_fp16 1 771.56 751.35 2.69%
bert_base_cased_fp16 64 971.31 970.93 0.04%
bert_large_uncased_fp16 32 305.07 305.32 -0.08%
bert_large_fp16 1 166.89 167.18 -0.17%
distilgpt2_fp16 16 1,278.83 1,278.19 0.05%

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

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

    :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

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

    :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

@pfultz2
Copy link
Collaborator

pfultz2 commented Oct 2, 2023

The major, minor and patch should be part of the SO major version:

math(EXPR MIGRAPHX_SO_MAJOR_VERSION "(${PROJECT_VERSION_MAJOR} * 1000 * 1000) + (${PROJECT_VERSION_MINOR} * 1000) + ${PROJECT_VERSION_PATCH}")
set(MIGRAPHX_SO_VERSION ${MIGRAPHX_SO_MAJOR_VERSION}.0)

Or we if we can drop the minor version it could just be:

math(EXPR MIGRAPHX_SO_VERSION "(${PROJECT_VERSION_MAJOR} * 1000 * 1000) + (${PROJECT_VERSION_MINOR} * 1000) + ${PROJECT_VERSION_PATCH}")

@umangyadav
Copy link
Member

umangyadav commented Oct 3, 2023

The major, minor and patch should be part of the SO major version:

math(EXPR MIGRAPHX_SO_MAJOR_VERSION "(${PROJECT_VERSION_MAJOR} * 1000 * 1000) + (${PROJECT_VERSION_MINOR} * 1000) + ${PROJECT_VERSION_PATCH}")
set(MIGRAPHX_SO_VERSION ${MIGRAPHX_SO_MAJOR_VERSION}.0)

Or we if we can drop the minor version it could just be:

math(EXPR MIGRAPHX_SO_VERSION "(${PROJECT_VERSION_MAJOR} * 1000 * 1000) + (${PROJECT_VERSION_MINOR} * 1000) + ${PROJECT_VERSION_PATCH}")

I don't see any difference between the two EXPRs that you mentioned.

@pfultz2
Copy link
Collaborator

pfultz2 commented Oct 6, 2023

I don't see any difference between the two EXPRs that you mentioned.

One has the version as ${VERSION}.0 and the other is just ${VERSION}.

@nunnikri
Copy link

nunnikri commented Oct 13, 2023

Are we good to approve this change? Or need further changes here? All other modules are using same format for lib version

@causten
Copy link
Collaborator

causten commented Oct 13, 2023

Merge conflict and you have not addressed Pauls comments

@frepaul
Copy link

frepaul commented Oct 16, 2023

I don't see any difference between the two EXPRs that you mentioned.

One has the version as ${VERSION}.0 and the other is just ${VERSION}.

The convention for actual library files that we have in ROCm across all the component is to have major.minor.{encoded ROCm version}.
rocm_set_soversion need to pass only major.minor. {encoded ROCm version} is handled by the function and attached as the patch number and ROCm version is passed by the build CI. Hope this clarify why we do not need the patch number passed here.

@causten causten merged commit c8f1cd9 into develop Oct 20, 2023
14 of 15 checks passed
@causten causten deleted the lib-soversion-update branch October 20, 2023 03:56
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.

7 participants