[BUG] We shouldn't expose our dependent library symbols across libcuml++.so

[teju85]

Describe the bug
While discussing RAFT integration, @dantegd described the reason for (re)cloning of dependencies of cuml here. Ideally, all symbols from these libraries that we depend upon should not have been exposed across libcuml++.so interface!

Expected behavior
If a user only has libcuml++.so and all headers inside cpp/include installed already, then they shouldn't need anything else (perhaps excepting cuda libraries and rmm), if they want to build python layer or just use the C++ library directly.

Advantages

1. Simplifies the linker process, as the users don't have to worry too much about maintaining other dependencies.
2. Simplifies a whole lot of cuML's python setup.py

Disadvantages

1. This means we might have to link those dependencies statically, thereby further increasing our .so file size

[hcho3]

@teju85 dmlc/xgboost#5590 may be relevant to this discussion, where XGBoost devs hid all C++ symbols from libxgboost.so and only exposed C API symbols. As a result, none of the C++ symbols from Rabit and dmlc-core dependencies leaked into libxgboost.so.

[teju85]

@hcho3 unfortunately, we have to expose C++ symbols since our cython layer depends on those!

[hcho3]

@teju85 Got it. It should be however possible to selectively hide some symbols by using compiler attributes. In addition, we can hide C++ symbols that come from dependencies.

[teju85]

Agreed. That's essentially the hope behind filing this issue.

[hcho3]

dmlc/xgboost#5590 used a white-list approach: hide all symbols, except a few symbols that were marked with a special macro.
Alternatively, we can consider a black-list approach: expose all symbols except those we elect to hide.

Either way, we are essentially mimicking what Windows does to all shared libs. On Windows platform, functions must be marked __declspec(dllexport) in order to be visible at DLL boundary.

[teju85]

Good point. For now, if we can hide symbols from:

1. treelite (and its dependencies)
2. faiss
3. spdlog

then that should be sufficient, for starters. Would you like to take this up, @hcho3 ?

[hcho3]

Is this issue blocking RAFT adoption?

[teju85]

No, shouldn't be a blocking issue, but certainly very useful if this got done soon.

[hcho3]

Got it. I'll take a look at this issue once I make progress on speeding up Treelite serialization.

[teju85]

Great! Thanks for volunteering. Agreed, this can certainly wait after the treelite-speedups work is over.

[hcho3]

Feel free to assign this issue to me.

[JohnZed]

I believe we already do this for libdmlc and libprotobuf since we had issues with that? See https://github.com/rapidsai/cuml/blob/branch-0.14/cpp/CMakeLists.txt around line 318. Wish there were a more cmake-native way of doing it though.

[teju85]

So, we should probably extend this for treelite, faiss and spdlog also?

[github-actions]

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

[github-actions]

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.