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

[REVIEW] add logger in cuML C++ #1867

Merged
merged 138 commits into from
Apr 10, 2020
Merged

Conversation

teju85
Copy link
Member

@teju85 teju85 commented Mar 13, 2020

This is the main PR for all logger-related issues in cuML:

  1. Contains a proposal to add spdlog-based logging capability in cuML (issue [TASK] [DEBT] Use GLOG in the cuML's back-end #100).
  2. It also tries to use the added logging capability in all algos currently just printing out to stdout.
  3. Adds *_NO_THROW macros (issue [FEA] [DEBT] support for CUDA_CHECK_NO_THROW macro #229) using these logging methods. In this process, it also fixes the exit issue with the cublas/cusolver macros (issue [FEA] [DEBT] Improve CUBLAS_CHECK and CUSOLVER_CHECK macros #240) as well as issue [DEBT] there are 2 cusparse_wrappers.h header files inside ml-prims #1691, as a by-product.
  4. Updates all dtor's to use these *_NO_THROW macros. This also means it fixes issue [FEA] [DEBT] Move CUBLAS_CHECK and CUSOLVER_CHECK to a headers that can be used in cpp files #239 .

Tagging @cjnolet, @dantegd and @JohnZed for discussions. (Guys, needless to say, this can certainly wait until 0.13 release!) Please note that even though the number of files touched is large, not the number of changes aren't, as this only updates the portions of these sources needing logging infra.

PS: Currently, I'm keeping the target branch as 0.13, but will update it to 0.14 once it becomes default.

@teju85 teju85 requested review from a team as code owners March 13, 2020 16:01
@teju85 teju85 requested a review from a team as a code owner April 10, 2020 04:01
@@ -165,6 +165,14 @@ class Base:
"""
self.handle = cuml.common.handle.Handle() if handle is None else handle
self.verbose = verbose
# NOTE:
Copy link
Member Author

Choose a reason for hiding this comment

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

Issue #2052 has been filed for this.

@cjnolet
Copy link
Member

cjnolet commented Apr 10, 2020

Dbscan looks good to me.

@teju85
Copy link
Member Author

teju85 commented Apr 10, 2020

Great. Since you're ok with this approach, I'll resolve the remaining 2 conversations on this logging thingy and will soon be updating all other algos currently using 'verbose' flag too.

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM

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 CUDA / C++ CUDA issue Tech Debt Issues related to debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants