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

De-vendor CMake dependencies and use shared libraries #753

Draft
wants to merge 25 commits into
base: branch-24.10
Choose a base branch
from

Conversation

gigony
Copy link
Contributor

@gigony gigony commented Aug 1, 2024

This patch de-vendors CMake dependencies and use/link shared libraries as much as possible.

Tasks

  • Update CMake files to use the conda package (and fall back to the downloaded source if the CONDA_PREFIX environment variable is not available).
    • cpp/cmake/deps
      • abseil.cmake
      • boost-header-only.cmake
      • catch2.cmake => Using Catch2 from Conda package has some issues with the CMake integration.
      • cli11.cmake
      • fmt.cmake
      • googlebenchmark.cmake
      • googletest.cmake
      • libcuckoo.cmake => no conda package available
      • nlohmann_json.cmake
      • nvtx3.cmake
      • taskflow.cmake
    • python/cmake/deps
      • fmt.cmake
      • nlohmann_json.cmake
      • pybind11_json.cmake
      • pybind11.cmake
    • cpp/plugins/cucim.kit.cuslide/cmake/deps
      • catch2.cmake
      • cli11.cmake
      • fmt.cmake
      • googlebenchmark.cmake
      • googletest.cmake
      • libculibos.cmake
      • libdeflate.cmake
      • libjpeg-turbo.cmake
      • libopenjpeg.cmake
      • libtiff.cmake
      • nlohmann_json.cmake
      • nvjpeg.cmake
      • openslide.cmake
      • pugixml.cmake
    • cpp/plugins/cucim.kit.cumed/cmake/deps
      • catch2.cmake
      • cli11.cmake
      • fmt.cmake
      • googlebenchmark.cmake
      • googletest.cmake
  • Make sure that required shared libraries are embedded in the xx.libs folder in the Python wheel file:
  • Switch to use rapids-cmake

Added conda packages (for build)

Test procedure

using docker

# On the repository root folder
docker run \
  --rm \
  -it \
  --gpus all \
  --pull=always \
  --network=host \
  --volume $PWD:/repo \
  --workdir /repo \
  rapidsai/ci-conda:cuda12.5.1-ubuntu22.04-py3.10


# Inside the docker

## remove build cache
find . -type d -name 'build-release' -exec rm -rf {} +
find /opt/conda/conda-bld/work/ -type d -name 'build-release' -exec rm -rf {} +

## run ci locally
./ci/build_cpp.sh

using conda locally

mamba env create -n cucim -f conda/environments/all_cuda-125_arch-x86_64.yaml
mamba activate cucim

./run build_local clean && ./run build_local clean_local && ./run build_local all release $CONDA_PREFIX

@gigony gigony added non-breaking Introduces a non-breaking change maintenance labels Aug 1, 2024
@gigony gigony self-assigned this Aug 1, 2024
@gigony gigony requested review from a team as code owners August 1, 2024 00:28
@gigony gigony requested a review from raydouglass August 1, 2024 00:28
Copy link

copy-pr-bot bot commented Aug 1, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@gigony gigony added the improvement Improves an existing functionality label Aug 1, 2024
@gigony gigony marked this pull request as draft August 1, 2024 00:32
@gigony
Copy link
Contributor Author

gigony commented Aug 1, 2024

/ok to test

@jakirkham jakirkham changed the base branch from branch-24.08 to branch-24.10 August 1, 2024 00:34
@jakirkham
Copy link
Member

Thanks Gigon! 🙏

Moved this to 24.10. Hope that is ok 🙂

@jakirkham
Copy link
Member

/ok to test

2 similar comments
@gigony
Copy link
Contributor Author

gigony commented Aug 1, 2024

/ok to test

@gigony
Copy link
Contributor Author

gigony commented Aug 1, 2024

/ok to test

@jakirkham
Copy link
Member

  • googlebenchmark.cmake

In conda-forge this is called benchmark

Though I wonder if we want to use rapids-cmake's rapids_cpm_gbench

@gigony
Copy link
Contributor Author

gigony commented Aug 1, 2024

/ok to test

1 similar comment
@gigony
Copy link
Contributor Author

gigony commented Aug 1, 2024

/ok to test

@jakirkham
Copy link
Member

libculibos.cmake

cuLIBOS is a static library that is included as part of CUDART and installed with the NVCC compiler

In terms of finding it, CMake already has FindCUDAToolkit, which includes a CUDA::culibos target that we can require and add to dependencies

So we can probably drop this file and simplify associated logic

@gigony
Copy link
Contributor Author

gigony commented Aug 1, 2024

/ok to test

@gigony
Copy link
Contributor Author

gigony commented Aug 1, 2024

@jakirkham It looks like the build container is out of space.

https://github.com/rapidsai/cucim/actions/runs/10206351504/job/28239540987?pr=753

  FAILED tests/unit/clara/test_image_cache.py::test_get_shared_memory_cache - RuntimeError: No space left on device

@jakirkham
Copy link
Member

Do we know how much space we are using?

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

If we just need Boost headers, we can make this change

conda/recipes/libcucim/meta.yaml Outdated Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

/ok to test

@jakirkham
Copy link
Member

Style check failure is due to a small typo. Fixing in PR: #759

gigony and others added 23 commits August 7, 2024 19:16
Signed-off-by: Gigon Bae <[email protected]>
Remove workarounds that address the following errors:
  Use generator expression to avoid `nvcc fatal   : Value '-std=c++17' is not defined for option 'Werror'`

Signed-off-by: Gigon Bae <[email protected]>
Do not use catch_discover_tests() since it causes a test to be run at build time
and somehow it causes a deadlock during the build.

Signed-off-by: Gigon Bae <[email protected]>
Signed-off-by: Gigon Bae <[email protected]>
Signed-off-by: Gigon Bae <[email protected]>
As we only need Boost headers in cuCIM, switch to the lighter weight
`libboost-headers` package from conda-forge. This way we only get the
headers and can skip the Boost libraries, which we don't use. Should
save some space during the build (and especially for users installing
the packages).
@jakirkham
Copy link
Member

jakirkham commented Aug 8, 2024

Rebased

Edit: Hoped that would mean GitHub would sign all the commits for us, but it seems that didn't work

@jakirkham
Copy link
Member

/ok to test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality maintenance non-breaking Introduces a non-breaking change
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants