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

remove system-installed UCX headers in wheel build environment #230

Merged
merged 1 commit into from
May 10, 2024

Conversation

jameslamb
Copy link
Member

Contributes to rapidsai/build-planning#57.

Follow-up to #226.

Proposes the following changes for wheel builds:

  • removing system-installed UCX headers
  • making the code to remove system-installed UCX libraries a bit more specific
    • (to minimize the risk of accidentally deleting some non-UCX thing who name matches the pattern libuc*)

Notes for Reviewers

Before applying similar changes to ucx-py, I noticed it being compiled with the system-installed headers but then linking against the libraries provided by the libucx wheels: rapidsai/ucx-py#1041 (comment)

This change should reduce the risk of that happening.

How I tested this

Poked around the filesystem that build_wheel.sh runs in by pulling one of our standard wheel-building container images used in CI.

docker run \
    --rm \
    -v $(pwd):/opt/work \
    -w /opt/work \
    -it rapidsai/ci-wheel:cuda12.2.2-rockylinux8-py3.10 \
    bash

find /usr -type f -name 'libucm*'
# /usr/lib64/libucm.la
# /usr/lib64/libucm.a
# /usr/lib64/libucm.so.0.0.0
# /usr/lib64/ucx/libucm_cuda.a
# /usr/lib64/ucx/libucm_cuda.la
# /usr/lib64/ucx/libucm_cuda.so.0.0.0

find /usr -type d -name 'uct'
# /usr/include/uct

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 10, 2024
@jameslamb jameslamb requested review from vyasr and pentschev May 10, 2024 16:17
@jameslamb jameslamb requested a review from a team as a code owner May 10, 2024 16:17
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered suggesting this when I was reviewing the ucx-py PR but decided I wasn't too worried because ucxx is a CMake-driven build so all the paths will be set correctly at once (unlike ucx-py, which uses setuptools and you have to do this more manually) so I didn't want to make work for you. It doesn't hurt though, so thanks!

@vyasr
Copy link
Contributor

vyasr commented May 10, 2024

/merge

@rapids-bot rapids-bot bot merged commit faa22fb into rapidsai:branch-0.38 May 10, 2024
55 checks passed
@jameslamb jameslamb deleted the remove-ucx-headers branch May 10, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants