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.

Sign up for GitHub

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

Jump to bottom

Fix various build errors #1152

Merged
merged 6 commits into from
Jan 19, 2023
Merged

Fix various build errors #1152

merged 6 commits into from
Jan 19, 2023

Conversation

hcho3
Copy link
Contributor

@hcho3 hcho3 commented Jan 18, 2023

Fix various build errors I encountered as I tried to build RAFT locally on my workstation. (Command used: ./build.sh -g raft-dask pylibraft libraft tests bench --compile-libs)

  • Add gtest as a link dependency of the C++ benchmark suite, to fix the error
[266/332] Building CUDA object CMakeFiles/NEIGHBORS_BENCH.dir/bench/neighbors/refine.cu.o
FAILED: CMakeFiles/NEIGHBORS_BENCH.dir/bench/neighbors/refine.cu.o
In file included from /home/phcho/Desktop/raft/cpp/bench/neighbors/../../test/neighbors/../test_utils.cuh:19,
from /home/phcho/Desktop/raft/cpp/bench/neighbors/../../test/neighbors/ann_utils.cuh:28,
from /home/phcho/Desktop/raft/cpp/bench/neighbors/../../test/neighbors/refine_helper.cuh:18,
from /home/phcho/Desktop/raft/cpp/bench/neighbors/refine.cu:39:
/home/phcho/Desktop/raft/cpp/bench/neighbors/../../test/neighbors/../test_utils.h:22:10: fatal error:
gtest/gtest.h: No such file or directory
22 | #include <gtest/gtest.h>
   |          ^~~~~~~~~~~~~~~
compilation terminated.
  • Explicitly specify the namespace for alignTo.
  • Cast pointers into an integral type prior to passing it to alignTo.
  • When calling areSameAlignOffsets(), pass the underlying pointers of the mdspan objects. Passing an mdspan to areSameAlignOffsets() is an error.

@hcho3 hcho3 requested review from a team as code owners January 18, 2023 02:04
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2023 •
edited
Loading

Codecov Report

Base: 87.99% // Head: 87.99% // No change to project coverage 👍

Coverage data is based on head (f27bf94) compared to base (efd42c9).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@              Coverage Diff              @@
##           branch-23.02    #1152   +/-   ##
=============================================
  Coverage         87.99%   87.99%           
=============================================
  Files                21       21           
  Lines               483      483           
=============================================
  Hits                425      425           
  Misses               58       58

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 18, 2023
lowener
lowener approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@lowener lowener left a comment

Choose a reason for hiding this comment

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

Nice fix, I have been bothered by some of these errors recently as well!

@achirkin
Copy link
Contributor

achirkin commented Jan 18, 2023 •
edited
Loading

The assert error only happens in Debug builds, because in the Release build the asserts are disabled. @cjnolet is there a chance we could add one Debug build to our CI matrix, so that bugs like these do not slip through?

achirkin
achirkin requested changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@achirkin achirkin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

cpp/bench/CMakeLists.txt
@@ -32,6 +32,7 @@ function(ConfigureBench)
PRIVATE raft::raft
$<$<BOOL:${ConfigureBench_DIST}>:raft::distance>
$<$<BOOL:${ConfigureBench_NN}>:raft::nn>
GTest::gtest
Copy link
Contributor

Choose a reason for hiding this comment

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

The benchmark shouldn't depend on gtest. I think, we should try to break the offending dependency instead (which is not so trivial).

Copy link
Contributor

Choose a reason for hiding this comment

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

...Or maybe we could live with the gtest dependency in benchmarks while we solve the problem with the header dependencies? Any thoughts, @cjnolet ?

Copy link
Member

Choose a reason for hiding this comment

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

100% agreed w/ @achirkin here. I was going to type up a similar comment but he beat me to it!

Copy link
Member

Choose a reason for hiding this comment

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

The benchmark shouldn't depend on gtest. I think, we should try to break the offending dependency instead (which is not so trivial).

My previous comment was in response to this, btw. I don't think we should be crossing the gtest and benchmark dependencies. We should try and pull out the shared bits into a separate header (and maybe try to find a good place in the include/raft/?) for the shared bits but keep gtest-specific things in test and benchmark-specific things in bench.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also okay opening a github issue about this and keeping the gtest dependency in here for now but adding a comment here that references the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #1153

cpp/include/raft/matrix/detail/linewise_op.cuh Outdated Show resolved Hide resolved
robertmaynard
robertmaynard approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Given that @cjnolet says we are good with opening an issue about refactoring the GTest dependency required by the benchmarks I am approving the CMake changes.

@hcho3 hcho3 mentioned this pull request Jan 18, 2023
Closed
@cjnolet
Copy link
Member

cjnolet commented Jan 19, 2023

/merge

@rapids-bot rapids-bot bot merged commit a7399cb into rapidsai:branch-23.02 Jan 19, 2023
@hcho3 hcho3 deleted the fix_build_error branch January 19, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lowener lowener lowener approved these changes

@achirkin achirkin achirkin approved these changes

@cjnolet cjnolet cjnolet approved these changes

@robertmaynard robertmaynard robertmaynard approved these changes

Assignees

@hcho3 hcho3

Labels
CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hcho3 @codecov-commenter @achirkin @cjnolet @robertmaynard @lowener