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

Warnings are errors #4075

Merged
merged 29 commits into from
Aug 10, 2021
Merged

Warnings are errors #4075

merged 29 commits into from
Aug 10, 2021

Conversation

harrism
Copy link
Member

@harrism harrism commented Jul 21, 2021 •
edited

Depends on rapidsai/raft#299
Depends on rapidsai/raft#311

Fixes #4086

This PR enables -Wall (warnings are errors) and fixes all current compiler warnings.

Compiler warnings should almost never be ignored. -Wall helps prevent hard-to-find bugs.

Tested with both Release and Debug builds since more warnings are typically reported in debug builds.

In any file I touched, I also cleaned up the includes so that they are grouped and sorted and ordered from "near" to "far" (relative to the file that is including).

Also cleaned up many instances of size_t --> std::size_t, however I will leave a global search and replace for a separate PR to make reviewing easier.

@harrism harrism requested review from a team as code owners July 21, 2021 05:30
@dantegd dantegd added this to PR-WIP in v21.10 Release via automation Jul 21, 2021
@dantegd dantegd added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 21, 2021
@dantegd dantegd requested a review from wphicks July 21, 2021 15:08
wphicks
wphicks requested changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

Beautiful! Very happy to see these changes. Just a few tweaks and a couple questions.

cpp/src/dbscan/dbscan.cuh Show resolved Hide resolved
cpp/src/holtwinters/internal/hw_eval.cuh Show resolved Hide resolved
cpp/src/holtwinters/internal/hw_eval.cuh Outdated Show resolved Hide resolved
cpp/src/holtwinters/internal/hw_optim.cuh Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cuh Outdated Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cuh Outdated Show resolved Hide resolved
cpp/src/random_projection/rproj.cuh Show resolved Hide resolved
cpp/src/svm/kernelcache.cuh Show resolved Hide resolved
cpp/src_prims/selection/knn.cuh Outdated Show resolved Hide resolved
v21.10 Release automation moved this from PR-WIP to PR-Needs review Jul 21, 2021
trivialfis
trivialfis reviewed Jul 22, 2021
View reviewed changes
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/src/glm/ridge_mg.cu Show resolved Hide resolved
cpp/src_prims/stats/minmax.cuh Outdated Show resolved Hide resolved
@dantegd dantegd mentioned this pull request Jul 22, 2021
Closed
wphicks
wphicks approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

The new changes look solid to me. I think this PR has exposed some other issues that cuML should address that are beyond the scope of the PR, but I'd advocate for merging and then doing follow-up cleanup.

In particular, having someone who's very familiar with the algorithm parameters go through and make sure we are using sensible types for sizes and indexing seems like a worthwhile follow-on. I'd be happy to have a crack at that myself if no one else is eager to.

@harrism
Copy link
Member Author

harrism commented Jul 27, 2021

In particular, having someone who's very familiar with the algorithm parameters go through and make sure we are using sensible types for sizes and indexing seems like a worthwhile follow-on. I'd be happy to have a crack at that myself if no one else is eager to.

Agree. Can you please create an issue?

@harrism
Copy link
Member Author

harrism commented Jul 27, 2021

Looks like I need to fix warnings in raft first...

@wphicks wphicks mentioned this pull request Jul 27, 2021
Open
@dantegd dantegd mentioned this pull request Jul 29, 2021
Open
11 tasks
@harrism
Copy link
Member Author

harrism commented Aug 5, 2021 •
edited

This this now depends on rapidsai/raft#311 which enables CUBLAS_CHECK_NO_THROW so we don't throw in a destructor.

@dantegd
Copy link
Member

dantegd commented Aug 6, 2021

rerun tests

dantegd
dantegd requested changes Aug 6, 2021
View reviewed changes
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

One very small correction, besides that it should be ready to merge!

cpp/cmake/modules/ConfigureCUDA.cmake Outdated Show resolved Hide resolved
harrism and others added 3 commits August 7, 2021 20:58
@harrism @dantegd
Update cpp/cmake/modules/ConfigureCUDA.cmake
71bb6d5 
Co-authored-by: Dante Gama Dessavre <dante.gamadessavre@gmail.com>
@harrism
Remove unused parameter name.
b9bfa80
@harrism
Merge branch 'fix-cuml-warnings-as-errors' of github.com:harrism/cuml…
da342bf 
… into fix-cuml-warnings-as-errors
v21.10 Release automation moved this from PR-Needs review to PR-Reviewer approved Aug 10, 2021
@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #4075   +/-   ##
===============================================
  Coverage                ?   85.96%           
===============================================
  Files                   ?      232           
  Lines                   ?    18500           
  Branches                ?        0           
===============================================
  Hits                    ?    15904           
  Misses                  ?     2596           
  Partials                ?        0
Flag Coverage Δ
dask 47.76% <0.00%> (?)
non-dask 78.56% <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 2c532c4...da342bf. Read the comment docs.

@harrism
Copy link
Member Author

harrism commented Aug 10, 2021

@gpucibot merge

1 similar comment
@dantegd
Copy link
Member

dantegd commented Aug 10, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e977f3e into rapidsai:branch-21.10 Aug 10, 2021
v21.10 Release automation moved this from PR-Reviewer approved to Done Aug 10, 2021
@zbjornson zbjornson mentioned this pull request Oct 31, 2021
Open
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@harrism
Warnings are errors (rapidsai#4075)
a1f972b 
Depends on rapidsai/raft#299
Depends on rapidsai/raft#311

Fixes rapidsai#4086

This PR enables -Wall (warnings are errors) and fixes all current compiler warnings.

Compiler warnings should almost never be ignored. -Wall helps prevent hard-to-find bugs.

Tested with both Release and Debug builds since more warnings are typically reported in debug builds.

In any file I touched, I also cleaned up the includes so that they are grouped and sorted and ordered from "near" to "far" (relative to the file that is including).

Also cleaned up many instances of `size_t` --> `std::size_t`, however I will leave a global search and replace for a separate PR to make reviewing easier.

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - William Hicks (https://github.com/wphicks)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4075
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertmaynard robertmaynard robertmaynard approved these changes

@cjnolet cjnolet cjnolet left review comments

@wphicks wphicks wphicks approved these changes

@trivialfis trivialfis trivialfis left review comments

@dantegd dantegd dantegd approved these changes

Assignees
No one assigned
Labels
CMake CUDA/C++ improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
v21.10 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Enable warnings as error during libcuml compilation
7 participants
@harrism @dantegd @codecov-commenter @robertmaynard @cjnolet @wphicks @trivialfis