-
Notifications
You must be signed in to change notification settings - Fork 1
[Feature] Bump hipCollections version #16
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
base: develop
Are you sure you want to change the base?
Changes from all commits
2789bd9
5984306
c8c5c16
7da392a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| build/ | ||
| graphbolt/build/ | ||
| dgl_sparse/build/ | ||
| tensoradapter/pytorch/build/ | ||
| python/build/ | ||
| python/dist/ | ||
| python/*.egg-info/ | ||
| python/libdgl.so |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,66 +1,41 @@ | ||||||
| #!/usr/bin/env bash | ||||||
| export CC=/opt/rocm/llvm/bin/clang | ||||||
| export CXX=/opt/rocm/llvm/bin/clang++ | ||||||
|
|
||||||
| # set the install prefix to the cwd/install | ||||||
| # INSTALL_PREFIX=$(pwd)/install | ||||||
| INSTALL_PREFIX=/opt/rocm | ||||||
| FILE_SOURCE_DIR=$(dirname $(realpath $0)) | ||||||
| DEPS_DIR=$(pwd) | ||||||
| ROCM_ROOT=/opt/rocm | ||||||
|
|
||||||
| # Not installed by default | ||||||
| git clone https://github.com/ROCm/libhipcxx.git | ||||||
| cd libhipcxx | ||||||
| git checkout v2.2.0 | ||||||
| cmake -B build \ | ||||||
| -DCMAKE_INSTALL_PREFIX=${INSTALL_PREFIX} | ||||||
| cmake --build build --target install | ||||||
| cd ${DEPS_DIR} | ||||||
| export CC=${ROCM_ROOT}/llvm/bin/clang | ||||||
| export CXX=${ROCM_ROOT}/llvm/bin/clang++ | ||||||
|
|
||||||
| # Need to patch for https://github.com/ROCm/rocm-libraries/issues/101. | ||||||
| # Should be fixed in | ||||||
| # https://github.com/ROCm/rocm-libraries/commit/e403601a2abe4a305cafd6526af2dc9bc69823e2#diff-7579081ee4dda43a07274a2397b8277bfa022af6d485ba086efc66a124ee8f5b | ||||||
| git clone https://github.com/tpopp/rocThrust.git | ||||||
| cd rocThrust | ||||||
| git checkout 613db9a025709fb18f2a676543a17850bd231b04 | ||||||
| cmake -B build \ | ||||||
| -DCMAKE_INSTALL_PREFIX=${INSTALL_PREFIX} | ||||||
| cmake --build build --target install | ||||||
| cd ${DEPS_DIR} | ||||||
| set -x | ||||||
| INSTALL_PREFIX=${ROCM_ROOT} | ||||||
| FILE_SOURCE_DIR=$(dirname $(realpath $0)) | ||||||
| DEPS_DIR=$(pwd) | ||||||
| export CMAKE_PREFIX_PATH="/opt/rocm/hip/lib/cmake;/opt/rocm/lib/cmake" | ||||||
|
|
||||||
| # Need to patch for https://github.com/ROCm/hipCollections/issues/7, https://github.com/ROCm/hipCollections/issues/8, https://github.com/ROCm/hipCollections/issues/9 | ||||||
| git clone https://github.com/tpopp/hipCollections.git | ||||||
| git clone https://github.com/ROCm/hipCollections.git -b release/rocmds-25.10 | ||||||
| export RAPIDS_CMAKE_SCRIPT_BRANCH=release/rocmds-25.10 | ||||||
| cd hipCollections | ||||||
| git checkout 6e31da8fd309f229d28adde8583a30bb4efaf1b7 | ||||||
| cmake -B build \ | ||||||
| -DCMAKE_INSTALL_PREFIX=${INSTALL_PREFIX} -DINSTALL_CUCO=ON -DBUILD_TESTS=OFF -DBUILD_BENCHMARKS=OFF -DBUILD_EXAMPLES=OFF | ||||||
| cmake --build build --target install | ||||||
| cd ${DEPS_DIR} | ||||||
|
|
||||||
|
|
||||||
| # if ROCM < 7.0 we also need to install rocThrust | ||||||
| ROCM_VERSION=$(/opt/rocm/bin/hipconfig --version) | ||||||
| #strip the major version from the ROCM_VERSION (before the dot) | ||||||
| ROCM_VERSION=${ROCM_VERSION%%.*} | ||||||
| echo "Working with ROCm Major Version: $ROCM_VERSION" | ||||||
| if [ "$ROCM_VERSION" -lt "7" ]; then | ||||||
|
|
||||||
| # Need to patch for https://github.com/ROCm/rocm-libraries/issues/94. Fixed in https://github.com/ROCm/rocm-libraries/commit/2539bb2e1cd17d287f532a65125b662bf0b658dc | ||||||
| git clone https://github.com/tpopp/hipCUB.git | ||||||
| cd hipCUB | ||||||
| git checkout f342111197dd020f1c4210b16aa550b08992e97b | ||||||
| cmake -B build \ | ||||||
| -DCMAKE_INSTALL_PREFIX=${INSTALL_PREFIX} | ||||||
| cmake --build build --target install | ||||||
| cd ${DEPS_DIR} | ||||||
| else | ||||||
| echo "ROCm Major Version is 7.0 or higher, skipping hipCUB installation" | ||||||
| # TODO remove this once the patches are merged | ||||||
| # Right now we need to patch the rocPRIM headers to fix the build because these | ||||||
| # config headers are missing gfx942 (I've added them manually) | ||||||
| cp ${FILE_SOURCE_DIR}/*.hpp ${INSTALL_PREFIX}/include/rocprim/device/detail/config/. | ||||||
|
|
||||||
| fi | ||||||
|
|
||||||
|
|
||||||
|
|
||||||
| # TODO this is an unacceptable way to do this, | ||||||
| # see https://github.com/ROCm/libhipcxx/issues/10 for more details | ||||||
| # This was implicitly not allowed in previous releases we were using, | ||||||
| # but with v2.7.0 they are explicitly not allowed. | ||||||
|
|
||||||
| # We only use semaphores for a counter of IO operations in graphbolt, | ||||||
| # that only runs on the host (not on the device) so we should be "safe" | ||||||
| # to use this for now. | ||||||
| sed -i '/#error semaphore is not supported on AMD hardware and should not be included/d' ${INSTALL_PREFIX}/include/rapids/libhipcxx/cuda/semaphore | ||||||
| sed -i '/#error semaphore is not supported on AMD hardware and should not be included/d' ${INSTALL_PREFIX}/include/rapids/libhipcxx/hip/semaphore | ||||||
| sed -i '/#error semaphore is not supported on AMD hardware and should not be included/d' ${INSTALL_PREFIX}/include/rapids/libhipcxx/cuda/std/semaphore | ||||||
| sed -i '/#error semaphore is not supported on AMD hardware and should not be included/d' ${INSTALL_PREFIX}/include/rapids/libhipcxx/hip/std/semaphore | ||||||
|
|
||||||
| # TODO remove this once the patches are merged | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recommend tagging all TODOs with a corresponding GitHub issue, e.g.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea this is a miss by myself when I added this todo a couple of weeks ago. It's tracked with an internal ticket and I'm trying to find the public reference. I think it's already been merged into the rocm-libraries, but those changes weren't included in several important releases of rocm (e.g. 7.0.0 and 7.1.0). I'll track down the links and add them here
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it was merged into the develop branch on rocm-libraries here: ROCm/rocm-libraries#1883. Not sure when it will get included in the rocm release though. I've added a link to this PR and more comments here to better document this.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I was suggesting something like making an issue in this repo to fix this and linking this TODO to that issue. Same with the TODO above. So it would be
Suggested change
and then issue dmlc#1234 can have all the necessary detail. I think it would also be more self-explanatory if rather than copying all the lose header files in this directory, this applied this upstream commit as a patch, but anyways, this is all kind of unrelated to your change here. |
||||||
| # the patches for this were merged in https://github.com/ROCm/rocm-libraries/pull/1883 | ||||||
| # but may take more time to be released. | ||||||
|
|
||||||
| # Right now we need to patch the rocPRIM headers to fix the build because these | ||||||
| # config headers are missing gfx942 (I've added them manually) | ||||||
| cp ${FILE_SOURCE_DIR}/*.hpp ${INSTALL_PREFIX}/include/rocprim/device/detail/config/. | ||||||
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.
Is this just a reformatting? Maybe revert, as it seems unrelated?
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.
This is just reformatting, but it seems like it was incorrect before. When I made changes to the file, it then tripped the CI checks here so even though it's separate from the important changes here, I think we should keep it.
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.
Ah, I think such linters should really only be looking at modified lines, but I see it comes from upstream, so no need to fight it.