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

Correct install path for include folder to avoid double nesting #3901

Merged
merged 2 commits into from
May 27, 2021

Conversation

dantegd
Copy link
Member

@dantegd dantegd commented May 26, 2021

Small oversight with the CMake changes, there already is a cuml folder in our cpp/include, so we were double nesting it. After the fix we're back to the old (expected) behavior, and consistent with RMM and cuDF.

cc @hcho3 @wphicks

@dantegd dantegd added bug Something isn't working non-breaking Non-breaking change labels May 26, 2021
@dantegd dantegd requested a review from a team as a code owner May 26, 2021 15:20
@dantegd dantegd added this to PR-WIP in v21.06 Release via automation May 26, 2021
v21.06 Release automation moved this from PR-WIP to PR-Reviewer approved May 26, 2021
@wphicks
Copy link
Contributor

wphicks commented May 26, 2021

rerun tests

v21.06 Release automation moved this from PR-Reviewer approved to PR-Needs review May 26, 2021
@github-actions github-actions bot added the gpuCI gpuCI issue label May 26, 2021
v21.06 Release automation moved this from PR-Needs review to PR-Reviewer approved May 26, 2021
@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #3901   +/-   ##
===============================================
  Coverage                ?   85.42%           
===============================================
  Files                   ?      226           
  Lines                   ?    17271           
  Branches                ?        0           
===============================================
  Hits                    ?    14754           
  Misses                  ?     2517           
  Partials                ?        0           
Flag Coverage Δ
dask 48.99% <0.00%> (?)
non-dask 77.39% <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 b6153c9...55e6ae8. Read the comment docs.

wphicks added a commit to triton-inference-server/fil_backend that referenced this pull request May 26, 2021
Include cuML header directory that is currently nested one layer too
deep (to be fixed by rapidsai/cuml#3901)

Ignore unused variable warnings due to unused variable in fil.h
introduced by rapidsai/cuml#3763
@dantegd
Copy link
Member Author

dantegd commented May 26, 2021

rerun tests

@JohnZed
Copy link
Contributor

JohnZed commented May 27, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 85445f6 into rapidsai:branch-21.06 May 27, 2021
v21.06 Release automation moved this from PR-Reviewer approved to Done May 27, 2021
wphicks added a commit to triton-inference-server/fil_backend that referenced this pull request May 28, 2021
* Temporary fix for upstream cuML issues

Include cuML header directory that is currently nested one layer too
deep (to be fixed by rapidsai/cuml#3901)

Ignore unused variable warnings due to unused variable in fil.h
introduced by rapidsai/cuml#3763

* Remove extra include path following upstream fix

* Update to CalVer for cuML
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Jun 1, 2021
Similar to rapidsai/cuml#3901. 

After #1491 and #rapids-cmake and #1585, now at install time, the cugraph headers are being nested into `path/to/env/include/cugraph/cugraph` instead of just `path/to/env/include/cugraph/`. This, as far as I'm aware, is unintentional and unlike the rest of RAPIDS projects (cuDF, RMM and cuML). 

cc @trxcllnt @robertmaynard

Authors:
  - Dante Gama Dessavre (https://github.com/dantegd)

Approvers:
  - Chuck Hastings (https://github.com/ChuckHastings)
  - Robert Maynard (https://github.com/robertmaynard)
  - Seunghwa Kang (https://github.com/seunghwak)
  - Paul Taylor (https://github.com/trxcllnt)
  - Rick Ratzel (https://github.com/rlratzel)

URL: #1630
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
…dsai#3901)

Small oversight with the CMake changes, there already is a `cuml` folder in our `cpp/include`, so we were double nesting it. After the fix we're back to the old (expected) behavior, and consistent with RMM and cuDF. 

cc @hcho3 @wphicks

Authors:
  - Dante Gama Dessavre (https://github.com/dantegd)

Approvers:
  - William Hicks (https://github.com/wphicks)
  - John Zedlewski (https://github.com/JohnZed)
  - Ray Douglass (https://github.com/raydouglass)

URL: rapidsai#3901
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CMake CUDA/C++ gpuCI gpuCI issue non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants