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.12
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 Show resolved Hide resolved
dependencies.yaml 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

@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

We discussed this PR in the cuCIM meeting and decided this wasn't going to make 24.10. So have moved to 24.12

@gigony
Copy link
Contributor Author

gigony commented Oct 1, 2024

#785 (comment) for devendoring libtiff

Signed-off-by: Gigon Bae <[email protected]>
Signed-off-by: Gigon Bae <[email protected]>
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]>
Signed-off-by: Gigon Bae <[email protected]>
Signed-off-by: Gigon Bae <[email protected]>
It looks like the RAPIDS CI/CD is launching the docker container with a reduced shared memory size. This is causing the shared memory cache to be failed with
'RuntimeError: No space left on device'.

Signed-off-by: Gigon Bae <[email protected]>
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