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

[BUG] Clean Up #include Dependencies #3376

Closed
mdemoret-nv opened this issue Jan 14, 2021 · 1 comment · Fixed by #3402
Closed

[BUG] Clean Up #include Dependencies #3376

mdemoret-nv opened this issue Jan 14, 2021 · 1 comment · Fixed by #3402
Labels
bug Something isn't working Build or Dep Issues related to building the code or dependencies CUDA / C++ CUDA issue Tech Debt Issues related to debt

Comments

@mdemoret-nv
Copy link
Contributor

Discovered in PR #3367, there are dependencies to cpp/src from the include folder, C-API, prims, tests, benchmark, etc. which causes issues when determining cmake PUBLIC interface includes for libcuml++. Due to this dependency, cpp/src needs to be listed as PUBLIC, implying it would be installed into the include directory on a system. Since this does not happen, any dependent cmake package or third party library will run into issues.

A quick search shows that many of the issues are related to #include <common/cumlHandle.hpp> which resides in cpp/src. This file is included from:

  • cpp/bench/sg/dataset.cuh
    • Benchmark target
  • cpp/include/cuml/neighbors/knn.hpp
    • This is particularly problematic since include should not depend on src
  • cpp/src/dbscan/dbscan_api.cpp
    • C-API target
  • cpp/test/prims/gram.cu
    • Test target
  • ... and 73 others

We should be able to remove this dependency with the addition of raft::handle_t.

Additionally, it would be worthwhile to do the following optional but beneficial improvements:

  1. Audit all includes and remove unused includes
  2. Convert any possible pointer objects to forward definitions
  3. Move the C-API files into a separate folder to better separate their headers and source files from the C++ library

This issue is related to (and partially blocks) issue #3375
@mdemoret-nv mdemoret-nv added bug Something isn't working ? - Needs Triage Need team to review and classify CUDA / C++ CUDA issue Build or Dep Issues related to building the code or dependencies Tech Debt Issues related to debt labels Jan 14, 2021
@mdemoret-nv mdemoret-nv mentioned this issue Jan 14, 2021
Closed
18 tasks
@wphicks wphicks removed the ? - Needs Triage Need team to review and classify label Jan 15, 2021
@mdemoret-nv
Copy link
Contributor Author

Quick analysis using our C++ Doxygen output shows some of the dependencies:
image

Seems like the following number of files need to be cleaned up:

  1. Files that #include "src/..." from include: 13
  2. Files that #include "src_prims/..." from include: 1
  3. Files that #include "src/..." from src_prims: 1

@mdemoret-nv mdemoret-nv mentioned this issue Jan 23, 2021
Merged
@rapids-bot rapids-bot bot closed this as completed in #3402 Feb 5, 2021
rapids-bot bot pushed a commit that referenced this issue Feb 5, 2021
@mdemoret-nv
Clean Up #include Dependencies (#3402)
d49dd1a 
Closes #3376

This PR moves a few header files around to fix bad dependencies from `include` -> `src`. Moving forward, the only allowed `#include` direction will be:

- `src` -> `include`
- `src` -> `src_prims`
- `src_prims` -> `include`

To facilitate this, a few changes needed to be made:
1. Functions for the C-API have been separated into their own `*_api.cpp` file (some were combined with C++ files)
2. `host_buffer`, `device_buffer`, `hostAllocator` and `deviceAllocator` were moved from `src_prims` to `include`
3. `#include <common/cumlHandle.hpp>` has been removed from all C++ files.

This PR includes some additional improvements:
- Updated the `include_checker.py` script:
   - Added functionality to check for badly placed `#pragma once`
   - Added functionality to fix some of the existing warnings
   - General refactor to support more checks
- Fixed all incorrect uses of `#pragma once`
- Fixed incorrect uses of `using namespace ...` in header files outside of a namespace
- Removed some unnecessary `#include`

While this touches a lot of files, the actual number of changes is relatively small. Below is a before/after comparison of the include graphs:

**`src/include`:**
Before:
![image](https://user-images.githubusercontent.com/42954918/105561661-ab32fe00-5cd4-11eb-8850-bfeaffaba60f.png)
After:
![image](https://user-images.githubusercontent.com/42954918/105561673-b4bc6600-5cd4-11eb-8bb7-04be91f902b6.png)

**`src/src_prims`:**
Before:
![image](https://user-images.githubusercontent.com/42954918/105561616-79ba3280-5cd4-11eb-8a49-1d0c030df7c1.png)
After:
![image](https://user-images.githubusercontent.com/42954918/105561633-89397b80-5cd4-11eb-9702-27b2a809834b.png)

Authors:
  - Michael Demoret (@mdemoret-nv)

Approvers:
  - Dante Gama Dessavre (@dantegd)
  - John Zedlewski (@JohnZed)
  - Thejaswi. N. S (@teju85)

URL: #3402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working Build or Dep Issues related to building the code or dependencies CUDA / C++ CUDA issue Tech Debt Issues related to debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[REVIEW] Clean Up #include Dependencies mdemoret-nv/cuml
2 participants
@wphicks @mdemoret-nv