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

[FEA] Add option to build faiss and treelite shared libs, inherit common dependencies from raft #4256

Merged
merged 96 commits into from
Jan 23, 2022

Conversation

trxcllnt
Copy link
Collaborator

@trxcllnt trxcllnt commented Sep 30, 2021

Depends on rapidsai/raft#345, and CI will be blocked until raft is removed from libcumlprims install export set.

@trxcllnt trxcllnt requested a review from a team as a code owner September 30, 2021 20:49
@dantegd dantegd added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Sep 30, 2021
@trxcllnt trxcllnt changed the base branch from branch-21.10 to branch-21.12 October 4, 2021 17:26
v22.02 Release automation moved this from PR-Reviewer approved to PR-Needs review Jan 20, 2022
@github-actions github-actions bot added conda conda issue Cython / Python Cython or Python issue labels Jan 20, 2022
@trxcllnt trxcllnt requested review from a team as code owners January 20, 2022 23:39
@trxcllnt
Copy link
Collaborator Author

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Jan 21, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Jan 21, 2022

Darn. It looks like even though we are using the cucollections patch that depends on thrust 1.15, CI in cuml is still pulling in thrust 1.12 from somewhere. I’m wondering if it’s something within cuML’s dependencies which is doing this (such as cumlprims_mg maybe?) since the RAFT side seems to be properly using thrust 1.15 (as evidenced in this CI log)

@cjnolet
Copy link
Member

cjnolet commented Jan 21, 2022

Looks like a signature change issue with ML::HDBSCAN::detail::Utils::cub_segmented_reduce

@trxcllnt and I finally figured out that there are conflicting versions of thrust being pulled in, which are causing the issues w/ the cub::DeviceSegmentedReduce signature.

@robertmaynard
Copy link
Contributor

Darn. It looks like even though we are using the cucollections patch that depends on thrust 1.15, CI in cuml is still pulling in thrust 1.12 from somewhere. I’m wondering if it’s something within cuML’s dependencies which is doing this (such as cumlprims_mg maybe?) since the RAFT side seems to be properly using thrust 1.15 (as evidenced in this CI log)

I am reading the CI logs to be that Thrust 1.12 exists in the conda env, and therefore causing the mismatch issue for cuml.

-- Thrust: TargetInfo: Thrust::Thrust: (1.12.0.0)
-- Thrust: TargetInfo: Thrust::Thrust > ALIASED_TARGET: _Thrust_Thrust
-- Thrust: TargetInfo: Thrust::Thrust > INTERFACE_INCLUDE_DIRECTORIES: $PREFIX/include/rapids/thrust
-- Thrust: Assembling target rmm::Thrust. Options: FROM_OPTIONS
-- Thrust: Creating system cache options: (advanced=FALSE)

I believe this as INTERFACE_INCLUDE_DIRECTORIES is pointing to $PREFIX/include/rapids/thrust which isn't where a source build places Thrust, but where we install it after a build. To me this reads like an issue with the base container holding an older version of Thrust. When was the arm container last updated?

@robertmaynard
Copy link
Contributor

rerun tests

2 similar comments
@trxcllnt
Copy link
Collaborator Author

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Jan 22, 2022

rerun tests

v22.02 Release automation moved this from PR-Needs review to PR-Reviewer approved Jan 22, 2022
@github-actions github-actions bot removed Cython / Python Cython or Python issue conda conda issue labels Jan 22, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.02@5df7010). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-22.02    #4256   +/-   ##
===============================================
  Coverage                ?   85.69%           
===============================================
  Files                   ?      236           
  Lines                   ?    19326           
  Branches                ?        0           
===============================================
  Hits                    ?    16562           
  Misses                  ?     2764           
  Partials                ?        0           
Flag Coverage Δ
dask 46.52% <0.00%> (?)
non-dask 78.59% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5df7010...9f39243. Read the comment docs.

@cjnolet
Copy link
Member

cjnolet commented Jan 23, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4653293 into rapidsai:branch-22.02 Jan 23, 2022
v22.02 Release automation moved this from PR-Reviewer approved to Done Jan 23, 2022
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
…pendencies from raft (rapidsai#4256)

Depends on rapidsai/raft#345, and CI will be blocked until `raft` is removed from `libcumlprims` install export set.

Authors:
  - Paul Taylor (https://github.com/trxcllnt)
  - Divye Gala (https://github.com/divyegala)
  - Ashwin Srinath (https://github.com/shwina)
  - Corey J. Nolet (https://github.com/cjnolet)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: rapidsai#4256
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CMake CUDA/C++ improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants