jameslamb
commented
1 month ago If you removed the RMM_EXPORT from a header, would the rmm Python builds or tests fail?
Good question!
I just tried removing the RMM_EXPORT from include/rmm/cuda_stream.hpp and repeated the testing from the PR description... building succeeded, ci/run_pytests.sh succeeded.
Then tried removing ALL of the RMM_EXPORT macros. Same thing... building succeeded, ci/run_pytests.sh succeeded.
Maybe this is because the Python package only calls into the DSOs through public symbols generated by Cython, and those are not affected by this CMake change?
I'm going to try pushing a commit with those removals here, let's see what other CI jobs say.
bdice
harrism
robertmaynard
vyasr
Description
Fixes #1645
Contributes to https://github.com/rapidsai/build-planning/issues/33
Similar to https://github.com/rapidsai/cudf/pull/15982
Proposes more tightly controlling the visibility of symbols in the shared libraries produces for the
rmmPython library, via the following:-fvisibility=hiddenby defaultrmm(everything in thermm::namespace) with__attribute__((visibility("default")))Benefits of this change
Reduces the risk of symbol conflicts when
rmmis used alongside other libraries. For example, see this case incudfwhere thespdlog::symbols inrmmare conflicting with thespdlog::symbols innvcomp: https://github.com/rapidsai/cudf/pull/15483#discussion_r1670892743Reduces library size by a bit (around 0.3 MB uncompressed), by reducing the size of symbol tables in DSOs.
Notes for Reviewers
This is at the very edge of my C++ knowledge, apologies in advance if I've missed something obvious 😬
Checklist
How I tested this
Re-used the wheels produced from this PR over in
cudfand sawcudf's tests succeed where previously the had failed because of symbol conflicts: https://github.com/rapidsai/cudf/pull/15483#discussion_r1711964794Also tested this locally, read on...
On an x86_64 machine with CUDA 12.2, checked out the latest
24.10ofrmmand the branch ofcudffrom https://github.com/rapidsai/cudf/pull/15483 (which is up to date with the latest24.10ofcudf).Built
librmm,rmm,libcudf, andcudfwheels from source, in the same CUDA 12, Python 3.11 image used for wheel building CI.how I did that (click me)
Created a script, `build-all.sh`, with the following. ```shell #!/bin/bash set -e -u -o pipefail mkdir -p "${WHEEL_DIR}" rm -r ${WHEEL_DIR}/*.whl || true echo "" > "${PIP_CONSTRAINT}" rm -rf /usr/local/src/rmm/python/*/{build,dist,final_dist} rm -rf /usr/local/src/cudf/python/*/{build,dist,final_dist} # move sources to /tmp, so file permissions don't get messed up cp -R /usr/local/src/cudf /tmp/cudf cp -R /usr/local/src/rmm /tmp/rmm # install wheel python -m pip install wheel # build rmm C++ wheel echo "" echo "--- building rmm C++ wheel ---" echo "" cd /tmp/rmm/python/librmm python -m pip wheel . -w ./dist -vvv --no-deps --disable-pip-version-check python -m wheel tags --platform any ./dist/librmm*.whl --remove mv ./dist/librmm*.whl ${WHEEL_DIR}/ echo "" echo "--- done done building rmm C++ wheel ---" echo "" # build rmm Python wheel # ensure 'rmm' wheel builds always use the 'librmm' just built in the same CI run # # using env variable PIP_CONSTRAINT is necessary to ensure the constraints # are used when created the isolated build environment echo "librmm-cu12 @ file://$(echo ${WHEEL_DIR}/librmm_cu12*.whl)" >> "${PIP_CONSTRAINT}" echo "" echo "--- building rmm Python wheel ---" echo "" cd /tmp/rmm/python/rmm python -m pip wheel . -w ./dist -vvv --no-deps --disable-pip-version-check mkdir -p ./final_dist python -m auditwheel repair -w ./final_dist ./dist/rmm_*.whl mv ./final_dist/rmm_*.whl ${WHEEL_DIR}/ echo "rmm-cu12 @ file://$(echo ${WHEEL_DIR}/rmm_cu12*.whl)" >> "${PIP_CONSTRAINT}" echo "" echo "--- done building rmm Python wheel ---" echo "" # build cudf C++ wheel echo "" echo "--- building cudf C++ wheel ---" echo "" cd /tmp/cudf/python/libcudf mkdir -p ./final_dist python -m pip wheel . -w ./dist -vvv --no-deps --disable-pip-version-check python -m auditwheel repair \ --exclude libarrow.so.1601 \ -w ./final_dist \ ./dist/libcudf_*.whl mv ./final_dist/libcudf_*.whl ${WHEEL_DIR}/ echo "libcudf-cu12 @ file://$(echo ${WHEEL_DIR}/libcudf_cu12*.whl)" >> "${PIP_CONSTRAINT}" echo "" echo "--- done building cudf C++ wheel ---" echo "" # build cudf Python wheel echo "" echo "--- building cudf Python wheel ---" echo "" cd /tmp/cudf/python/cudf export SKBUILD_CMAKE_ARGS="-DUSE_LIBARROW_FROM_PYARROW=ON" mkdir -p ./final_dist python -m pip wheel . -w ./dist -vvv --no-deps --disable-pip-version-check python -m auditwheel repair \ --exclude libcudf.so \ --exclude libarrow.so.1601 \ -w ./final_dist \ ./dist/cudf_*.whl mv ./final_dist/cudf_*.whl ${WHEEL_DIR}/ echo "cudf-cu12 @ file://$(echo ${WHEEL_DIR}/cudf_cu12*.whl)" >> "${PIP_CONSTRAINT}" echo "" echo "--- done building cudf Python wheel ---" echo "" ``` Ran it like this: ```shell docker run \ --rm \ -v $(pwd):/usr/local/src \ -w /usr/local/src \ --env PIP_CONSTRAINT=/usr/local/src/build-constraints.txt \ --env WHEEL_DIR=/usr/local/src/dist \ -it rapidsai/ci-wheel:cuda12.5.1-rockylinux8-py3.11 \ bash # build all the wheels ./build-all.sh ```Installed the wheels produced by that script, using one of the testing images used across RAPIDS CI.
how I did that (click me)
```shell docker run \ --rm \ --gpus 1 \ --env WHEEL_DIR=/usr/local/src/dist \ -v $(pwd):/usr/local/src \ -w /usr/local/src \ -it rapidsai/ci-conda:cuda12.2.2-ubuntu22.04-py3.11 \ bash # install everything pip install ${WHEEL_DIR}/*.whl ```Saw that fail with exactly the same symbol-conflict issue discussed in https://github.com/rapidsai/cudf/pull/15483#discussion_r1670892743..
Repeated that process with the changes you see in this PR, and observed:
rmm's Python tests passingcudf😁Also inspected the symbol tables with
readelf. For example, here are the counts of symbols by visibility inmemory_resource.cpython-311-x86_64-linux-gnu.so:It also helped reduce the sizes a bit
how I did that (click me)
Unzipped the `rmm` wheel. ```shell rm -rf ./delete-me mkdir -p ./delete-me unzip -d ./delete-me ./dist/rmm_cu12*.whl ``` Checked the symbol tables with `readelf` ```shell readelf --symbols --wide \ ./delete-me/rmm/_lib/memory_resource.cpython-311-x86_64-linux-gnu.so \ | c++filt \ > syms.txt echo "GLOBAL: $(grep --count -E ' GLOBAL ' < ./syms.txt)" echo "WEAK: $(grep --count -E ' WEAK ' < ./syms.txt)" echo "LOCAL: $(grep --count -E ' LOCAL ' < ./syms.txt)" ``` Checked the sizes with `pydistcheck` ```shell # and to get the sizes pip install pydistcheck pydistcheck --inspect ./dist/rmm_cu12*.whl ```Where did
RMM_EXPORTcome from?It's been in
rmmfor a few years:https://github.com/rapidsai/rmm/blob/975c911ad59219e98b8aed1e9959120a2c877d06/include/rmm/detail/export.hpp#L19-L26
But only used in one place, from #833.
References
I found these helpful: