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

Make 'librmm' a 'host' dependency for conda packages #2284

Merged
merged 14 commits into from
May 2, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Apr 30, 2024

Contributes to rapidsai/build-planning#54.

The libraft-headers and libraft-headers-only conda packages are bundling rmm's headers. I believe that's because the librmm conda package isn't available in the libraft* conda build environment, and as a result it's getting rmm via CPM (thanks to rapids-cmake).

As a result, this project and any that depend on it are seeing warnings like the following in conda builds where conda's path_conflict setting is set to warn or prevent (like #2245):

This transaction has incompatible packages due to a shared path.
  packages: rapidsai-nightly/linux-64::librmm-24.04.00a38-cuda11_240326_ga98931b9_38, rapidsai-nightly/linux-64::libraft-headers-only-24.04.00a93-cuda11_240326_g9637b3c2_93
  path: 'include/rmm/mr/device/arena_memory_resource.hpp'

To fix that, this proposes the following changes:

  • make librmm a host dependency of the following conda packages: libraft-headers-only, libraft-headers

Benefits of this change

  • slightly smaller libraft-headers and libraft-headers-only conda packages
  • reduced risk of runtime and installation issues caused by file clobbering

Notes for reviewers

History of changes to the librmm dependency for libraft-headers:

In particular, #2102 referred to the host dependency on librmm as "extraneous" but from a packaging perspective, I don't think it is. librmm being in host means it'll be present in the build environment, which means its headers will be found instead of downloaded, and therefore not packaging into the libraft* conda packages.

How I tested this

Built all the raft conda packages locally from branch-24.06 and confirmed that they contain rmm headers. Then again from this branch and confirmed they were gone.

docker run \
    --rm \
    --env-file "${PWD}/aws-creds.env" \
    -v $(pwd):/opt/work \
    -w /opt/work \
    -it rapidsai/ci-conda:cuda12.2.2-ubuntu22.04-py3.10-amd64 \
    bash

CI="true" \
  ci/build_cpp.sh

# On 'branch-24.06', this showed the rmm headers being packaged.
# On this branch, they're omitted.
tar --bzip2 -tf \
  /tmp/conda-bld-output/linux-64/libraft-headers-only-24.06.00a50-cuda12_240430_g1e0e2283_50.tar.bz2 \
| grep 'include/rmm' \
| wc -l

Also checked the CI logs from conda-cpp-build jobs here. On other recent PRs, I see CPM downloading rmm ...

-- CPM: Adding package rmm@24.06 (branch-24.06)

... and all the rmm headers getting installed as part of the libraft-headers package

-- Installing: /opt/conda/conda-bld/_h_env_placehold_placehold_..._placeho/include/rmm/cuda_stream.hpp

(build link)

Here, I see librmm coming through via the conda package requirements ...

The following NEW packages will be INSTALLED:
    ...
    librmm:                      24.06.00a17-cuda12_240430_g26fa9ecb_17 rapidsai-nightly

... and being used instead of downloads via CPM ...

-- CPM: Using local package rmm@24.06.0

... and no installation of the rmm headers as part of building any libraft packages.

(build link)

@jameslamb jameslamb added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change 2 - In Progress Currenty a work in progress labels Apr 30, 2024
@jameslamb jameslamb changed the title WIP: Make 'librmm' a 'host' dependency for conda packages Make 'librmm' a 'host' dependency for conda packages May 1, 2024
@jameslamb jameslamb removed the 2 - In Progress Currenty a work in progress label May 1, 2024
@jameslamb jameslamb marked this pull request as ready for review May 1, 2024 17:43
@jameslamb jameslamb requested a review from a team as a code owner May 1, 2024 17:43
@jameslamb
Copy link
Member Author

Alright there are still two arm64 test jobs queued, but I see enough other stuff passing that I think this is ready for review.

@bdice @ajschmidt8 since you'd done previous work related to this dependency, could you take a look?

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Yup. That'd explain it. I made a mistake in #2102 in my recollection of how conda uses host dependencies. This should be a proper fix.

@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 3406569 into rapidsai:branch-24.06 May 2, 2024
69 checks passed
@jameslamb jameslamb deleted the test-rmm-host branch May 2, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants