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

document changes in library-loading, update build-UCX-from-source docs #1099

Merged
merged 5 commits into from
Dec 16, 2024

Conversation

jameslamb
Copy link
Member

Contributes to rapidsai/build-planning#118

Caused by rapidsai/ucx-wheels#13

I originally came here to document the implications of rapidsai/ucx-wheels#13 in the docs, namely:

  • if you have a libucx-cu{11,12} wheel installed, then by default ucx-py will use UCX libraries from that wheel
  • environment variable RAPIDS_LIBUCX_PREFER_SYTEM_LIBRARY=true can be set to opt out of this and use a system installation instead

While doing that, I noticed some other opportunities for improvement in the installation docs:

Notes for Reviewers

How I tested this

Followed these instructions in a Docker container running on an x86_64 machine with 8 V100s.

docker run \
    --rm \
    --gpus 0 \
    -v $(pwd):/opt/work \
    -w /opt/work \
    -it rapidsai/ci-conda:latest \
    bash

Used conda to set up the build environment:

conda create -n ucx -c conda-forge \
    automake make libtool pkg-config \
    "python=3.12" "setuptools>=64.0" "cython>=3.0.0" \
    cuda-nvcc \
    cuda-cudart-dev \
    cuda-nvml-dev \
    cuda-nvtx-dev \
    cuda-version=12.5

Ran variations of this code snippet to test my install:

python -c "import ucp; print(ucp.get_ucx_version())"

Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

Thanks @jameslamb I've left a few comments, overall looks good.

Comment on lines 100 to 101
See https://docs.rapids.ai/install/ for the list of supported Python versions
for recent RAPIDS releases.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not the most user-friendly way to phrase this here, unfortunately we do not have UCX-Py explicitly documented in https://docs.rapids.ai/install/ which has a different versioning scheme than the rest of RAPIDS, and thus it would be difficult for someone to understand what Python version is supported for UCX-Py based solely on the RAPIDS version. I agree the way it was documented isn't optimal and I can't think of a better way to do this differently than what it was either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that makes sense. My main motivation here was to avoid a hard-coded list of versions, since duplicating that kind of information in multiple places makes it more likely that it'll become out of date.

That said.... we do have a rapids-reviser template set up to replace this line when we roll out add-a-new-Python-version updates to all of RAPIDS.

I'll revert this change based on your comments here.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that would be ideal, unfortunately I think it's not helpful for the users given the different branching.

How does the rapids-reviser work? Can we easily make it replace arbitrary code/docs lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, no prob! I've reverted that change here.

Can we easily make it replace arbitrary code/docs lines?

Yes totally! You give it a shell script and say "run this on these repos". This type of case is easy to automatically update there with sed, not a problem.

docs/source/install.rst Show resolved Hide resolved
docs/source/install.rst Outdated Show resolved Hide resolved
Co-authored-by: Peter Andreas Entschev <[email protected]>
docs/source/install.rst Outdated Show resolved Hide resolved
@jameslamb jameslamb requested a review from pentschev December 16, 2024 15:02
Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jameslamb .

@jameslamb
Copy link
Member Author

Thanks @pentschev for continuing to work with me and @vyasr on these patterns for how these libraries find UCX 😁

@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit c7848de into rapidsai:branch-0.42 Dec 16, 2024
39 checks passed
@jameslamb jameslamb deleted the docs/library-loading branch December 16, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants