-
Notifications
You must be signed in to change notification settings - Fork 89
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
Set the SO version correctly #2355
Conversation
@@ -76,7 +76,8 @@ include(ROCMSetupVersion) | |||
option(BUILD_DEV "Build for development purpose only" OFF) | |||
|
|||
rocm_setup_version(VERSION 2.8.0) | |||
set(MIGRAPHX_SO_VERSION ${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR}) | |||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the rocm_set_soversion
already doing this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No its not. For 2.7 it will set the SO version to 2.7.{rocm_version}
, which the minor version should be part of the major version(and no minor version), so it should be 2007000.0.{rocm_version}
.
Thats because between 2.6 and 2.7 we want different major versions for the SO. So we want the versions to be 2007000.0.{rocm_version1}
and 2006000.0.{rocm_version2}
, not 2.7.{rocm_version1}
and 2.6.{rocm_version2}
(which incorrectly implies they are compatible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frepaul can you approve this? Would it result in any incompatibilities?
Codecov Report
@@ Coverage Diff @@
## develop #2355 +/- ##
========================================
Coverage 91.36% 91.36%
========================================
Files 440 440
Lines 16530 16530
========================================
Hits 15101 15101
Misses 1429 1429 |
This build is OK for merge ✅ |
🔴torchvision-inceptionv3_1: FAILED: MIGraphX is not within tolerance - check verbose output🔴slim-inceptionv4_1: FAILED: MIGraphX is not within tolerance - check verbose output🔴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🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving as this change doesn't affect the API library. For the API library, it would follow whatever rocm_set_soversion
scheme as defined here.
(https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/d1abf06fa61a2833c7b66feae4a02696f8684db7/src/api/CMakeLists.txt#L33)
Different migraphx versions are not binary compatible, so we need to make sure a different major version is used for every version of migraphx.