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

Only install necessary components in conda packages. #2209

Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Mar 4, 2024

Closes #2175.

This PR cuts the binary size of libraft by about 50%. In total, this PR shrinks the binary size of the entire C++ conda channel (the sum of all conda packages produced by conda/recipes/libraft/meta.yaml) by around 20%, from 1.5 GB to 1.2 GB. The main difference is dropping the extraneous copy of libraft.a (~300 MB) that was shipped in the libraft package.

@bdice bdice added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 4, 2024
Comment on lines +4 to +5
./build.sh libraft --allgpuarch --compile-lib --build-metrics=compile_lib --incl-cache-stats --no-nvtx -n
cmake --install cpp/build --component compiled
Copy link
Contributor Author

@bdice bdice Mar 4, 2024

Choose a reason for hiding this comment

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

This now looks a lot like https://github.com/rapidsai/raft/blob/branch-24.04/conda/recipes/libraft/build_libraft_static.sh except installing the compiled component rather than compiled-static.

@msarahan
Copy link

msarahan commented Mar 4, 2024

compared with #2208, this approach avoids the top-level build. I think this PR is probably a better approach. Certainly fewer changes, and it should have the same effect. I'd be happy to close #2208, but I'll let you guys have the say on whether you want to.

@bdice bdice changed the title Only install compiled component in build_libraft.sh. Only install necessary components in conda packages. Mar 4, 2024
@bdice
Copy link
Contributor Author

bdice commented Mar 4, 2024

This is failing to build libraft-template for some reason due to hnswlib. I haven't had a chance to debug this yet. Just copying it here for awareness, for the moment.

CMake Error at /opt/conda/conda-bld/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/lib/cmake/raft/raft-targets.cmake:61 (set_target_properties):
  The link interface of target "raft::raft" contains:

    hnswlib::hnswlib

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /opt/conda/conda-bld/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/lib/cmake/raft/raft-config.cmake:84 (include)
  build/cmake/CPM_0.38.5.cmake:243 (find_package)
  build/cmake/CPM_0.38.5.cmake:303 (cpm_find_package)
  build/_deps/rapids-cmake-src/rapids-cmake/cpm/find.cmake:189 (CPMFindPackage)
  cmake/thirdparty/get_raft.cmake:36 (rapids_cpm_find)
  cmake/thirdparty/get_raft.cmake:57 (find_and_configure_raft)
  CMakeLists.txt:34 (include)

@robertmaynard
Copy link
Contributor

This is failing to build libraft-template for some reason due to hnswlib

The libraft-template builds build against an installed version of libraft. The installed version of libraft is now just installing the libraft target which means that the hnswlib headers / library aren't being installed into the libraft conda package. Which causes the consumers to fail.

The hnswlib install rules in get_hnswlib.cmake need to be expanded with an explicit COMPONENT and that component needs to get added to cmake --install invocation.

@bdice
Copy link
Contributor Author

bdice commented Mar 5, 2024

@robertmaynard It looks like I got the same error again. https://github.com/rapidsai/raft/actions/runs/8159304257/job/22303364726?pr=2209#step:7:3758

Can you look at the implementation (c101777) and offer guidance?

@bdice bdice marked this pull request as ready for review March 6, 2024 15:43
@bdice bdice requested review from a team as code owners March 6, 2024 15:43
@bdice bdice requested a review from a team as a code owner March 6, 2024 15:45
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@bdice
Copy link
Contributor Author

bdice commented Mar 6, 2024

Thanks @robertmaynard for the help in debugging!

@bdice
Copy link
Contributor Author

bdice commented Mar 6, 2024

It looks like libraft-tests is packaging libraft.a now. I suspect this is because it is no longer provided by the libraft conda package, but is still being built by the tests and thus repackaged in the libraft-tests package? Investigating now.

CI shows this, too:
https://github.com/rapidsai/raft/actions/runs/8176176830/job/22355050936?pr=2209#step:7:15308

2024-03-06T17:43:08.8826850Z WARNING: Exact overlap between lib/cmake/raft/raft-compiled-static-lib-targets-release.cmake in packages libraft-tests and libraft-static
2024-03-06T17:43:08.8827918Z WARNING: Exact overlap between lib/cmake/raft/raft-compiled-static-lib-targets.cmake in packages libraft-tests and libraft-static
2024-03-06T17:43:08.8828681Z WARNING: Exact overlap between lib/libraft.a in packages libraft-tests and libraft-static

@bdice
Copy link
Contributor Author

bdice commented Mar 6, 2024

@robertmaynard I couldn't figure out how to make the build_libraft_tests.sh not build/install the static library, so instead I added libraft-static as a host dependency of libraft-tests. Hopefully this will keep it from repackaging the static library and other files. See baed39a.

If we can figure out how to make the tests avoid building the static library, that would be a better solution.

@bdice
Copy link
Contributor Author

bdice commented Mar 7, 2024

As of baed39a, we are packaging the right things. It may be possible to cut down the build process further (by not building libraft.a for the libraft-tests package, or reusing other parts of the build for libraft-tests), but at least the resulting packages contain only the relevant pieces.

This PR cuts the binary size of libraft by about 50%. In total, this PR shrinks the binary size of the entire C++ conda channel by around 20%, from 1.5 GB to 1.2 GB. The main difference is dropping the extraneous copy of libraft.a (~300 MB) that was shipped in the libraft package.

@bdice
Copy link
Contributor Author

bdice commented Mar 7, 2024

/merge

@rapids-bot rapids-bot bot merged commit 2d714cb into rapidsai:branch-24.04 Mar 7, 2024
72 checks passed
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.

[BUG] conda libraft package includes static library
6 participants