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

use rapids-build-backend #234

Merged
merged 12 commits into from
Jun 10, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Jun 3, 2024

Contributes to rapidsai/build-planning#31
Contributes to rapidsai/build-planning#69
Contributes to rapidsai/dependency-file-generator#89

Proposes:

  • introducing rapids-build-backend as this project's build backend, to reduce the complexity of various CI/build scripts
  • using pip install ./dist/*.whl instead of pip install --find-links ./dist in CI, to reduce the risk of the types of bugs described in Remove uses of 'pip --find-links' in CI build-planning#69

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jun 3, 2024
@jameslamb jameslamb changed the title WIP: Rapids build backend use rapids-build-backend Jun 4, 2024
@jameslamb jameslamb marked this pull request as ready for review June 4, 2024 18:57
@jameslamb jameslamb requested review from a team as code owners June 4, 2024 18:57
@jameslamb jameslamb requested a review from bdice June 4, 2024 18:58
Comment on lines 38 to 40
sed_runner "/- librmm =/ s/=.*/=${NEXT_RAPIDS_VERSION},>=0.0.0a0/g" conda/recipes/ucxx/meta.yaml
sed_runner "/- rmm =/ s/=.*/=${NEXT_RAPIDS_VERSION},>=0.0.0a0/g" conda/recipes/ucxx/meta.yaml
sed_runner "/- rapids-dask-dependency =/ s/=.*/=${NEXT_RAPIDS_VERSION},>=0.0.0a0/g" conda/recipes/ucxx/meta.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

We use a slightly different approach to handling the RAPIDS-like / UCX-like version schemes elsewhere. In raft-dask, for instance, we have a conda_build_config.yaml variable named ucxx_version and we update that in one place in update-version.sh.

Could we do something similar for ucxx? Rather than replacing the version for each RAPIDS library, let's add a single variable like rapids_version in conda_build_config.yaml and use that everywhere in meta.yaml.

This could be done in a follow-up PR, but I'd like to see it fixed in this release. (Same for ucx-py, probably, though I haven't checked.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Love that idea, thank you! That change seemed small enough that I pushed it here. Will look at doing something similar on rapidsai/ucx-py#1044.

@jameslamb jameslamb removed the request for review from a team June 10, 2024 14:40
@jameslamb jameslamb requested review from a team as code owners June 10, 2024 14:41
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 , left a few minor comments but other than that looks good.

@@ -226,12 +226,12 @@ if buildAll || hasArg ucxx; then

cd ${REPODIR}/python/
SKBUILD_CMAKE_ARGS="-DCMAKE_PREFIX_PATH=${INSTALL_PREFIX};-DCMAKE_BUILD_TYPE=${BUILD_TYPE};${SKBUILD_EXTRA_CMAKE_ARGS}" \
python -m pip install --no-build-isolation --no-deps .
python -m pip install --no-build-isolation --no-deps --config-settings rapidsai.disable-cuda=true .
Copy link
Member

Choose a reason for hiding this comment

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

What does rapidsai.disable-cuda=true do exactly? Note that the libucxx Cython SO does link to CUDA currently (at least in the conda package):

ldd ./lib/python3.10/site-packages/ucxx/_lib/libucxx.cpython-310-x86_64-linux-gnu.so
        linux-vdso.so.1 (0x00007ffc2d996000)
        libucxx_python.so => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../../libucxx_python.so (0x00007f1c8b42a000)
        libpython3.10.so.1.0 => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../../libpython3.10.so.1.0 (0x00007f1c8b08d000)
        libucxx.so => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../../libucxx.so (0x00007f1c8affa000)
        libcudart.so.11.0 => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../../libcudart.so.11.0 (0x00007f1c8ad53000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f1c8ad38000)
        libucp.so.0 => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../../libucp.so.0 (0x00007f1c8ac68000)
        libstdc++.so.6 => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../../libstdc++.so.6 (0x00007f1c8aa85000)
        libgcc_s.so.1 => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../../libgcc_s.so.1 (0x00007f1c8aa6a000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f1c8a878000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f1c8a855000)
        libutil.so.1 => /lib/x86_64-linux-gnu/libutil.so.1 (0x00007f1c8a84e000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f1c8a6ff000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f1c8a6f5000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f1c8b4e4000)
        libuct.so.0 => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../.././libuct.so.0 (0x00007f1c8a6b8000)
        libucs.so.0 => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../.././libucs.so.0 (0x00007f1c8a65a000)
        libucm.so.0 => /datasets/pentschev/miniconda3/envs/rn-240610/./lib/python3.10/site-packages/ucxx/_lib/../../../../././libucm.so.0 (0x00007f1c8a63e000)

It probably doesn't need to as I'd assume libucxx.so/libucxx_python.so would do any necessary CUDA linkage though, but admittedly I get confused as to what is (not) necessary to satisfy all the dependency tree.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry if the name is confusing. This does NOT disable linking against anything from the CUDA Toolkit.

This option tells rapids-build-backend to avoid doing the following:

  • add a suffix like -cu11 or -cu12 to the name of the package it builds
  • update the list of dependencies by reading the output of running rapids-dependency-file-generator with something like cuda="12.2"

It's passed here in build.sh for conda builds because the conda packages we publish don't have -cu{version} type of suffixes.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks for the details. In that case we're good here! 😄

ci/release/update-version.sh Outdated Show resolved Hide resolved
Co-authored-by: Peter Andreas Entschev <[email protected]>
@pentschev
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 1e6d80c into rapidsai:branch-0.39 Jun 10, 2024
55 checks passed
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.

4 participants