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

Adapt virtually everything else in mlpack to header-only #3233

Merged
merged 31 commits into from Jul 31, 2022

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Jun 21, 2022

This PR nearly finishes making everything in mlpack header-only! It is a follow-up to #3091 and #3195. I've converted all files except one (trivial) file to header-only, so now libmlpack.so contains I think only one function definition. (I didn't remove that function, because I wanted to separate CMake changes into a separate PR, and if libmlpack.so has no files, CMake doesn't like that and fails to build. So, I left just one...)

Not every adaptation here was simple, so I'll detail each non-trivial change below:

  • The Log object used to be a class, but now it's a namespace that contains translation-unit-local static singletons Debug, Info, Warn, and Fatal. From a developer/user perspective, nothing is changed and they are still used the same way.

  • Any other singletons, like IO and Timer, are now held in each individual translation unit, as opposed to just one in libmlpack.so. That's okay, and all of our bindings work just fine with that.

  • The CheckInputMatrices() function in util/ had a dependency on data/, and forward declarations would not solve this. So, I instead made CheckInputMatrices() call out to a forward-defined function in data/ called CheckCategoricalParam(). The functionality is the same as before.

  • The backtrace code used some global objects and the Backtrace class itself had all its functions declared as static. But it was only ever used in a non-static context, so I just moved the global objects inside Backtrace and it works fine.

@conradsnicta
Copy link
Contributor

Nice work. If this hasn't been addressed previously, I suggest simplifying how mlpack is consumed by users, so that a straightforward #include <mlpack.hpp> is sufficient. In other words, the same as ensmallen and armadillo.

This simplification would replace users manually selecting a subset of headers from the multitude of available headers. The manual selection is tedious and can be error prone, especially for newbies or time-constrained users that just want to get their stuff running quickly.

@rcurtin
Copy link
Member Author

rcurtin commented Jun 22, 2022

Let me do some timing tests to convince myself that including extra headers from all of mlpack doesn't make a significant compilation time difference (I suspect it won't), and then I will do that. That, plus updating the CMake configuration, I think are the last things that need to be done before we can release mlpack 4.

src/mlpack/core/util/forward.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/check_categorical_param.hpp Outdated Show resolved Hide resolved
src/mlpack/base.hpp Outdated Show resolved Hide resolved
src/mlpack/base.hpp Outdated Show resolved Hide resolved
src/mlpack/base.hpp Outdated Show resolved Hide resolved
src/mlpack/base.hpp Outdated Show resolved Hide resolved
src/mlpack/base.hpp Outdated Show resolved Hide resolved
src/mlpack/base.hpp Outdated Show resolved Hide resolved
src/mlpack/methods/det/dt_utils_impl.hpp Outdated Show resolved Hide resolved
@conradsnicta
Copy link
Contributor

Let me do some timing tests to convince myself that including extra headers from all of mlpack doesn't make a significant compilation time difference (I suspect it won't), and then I will do that.

related: #3237 (comment)

@conradsnicta
Copy link
Contributor

@rcurtin This all looks fine to me. I still suggest to make mlpack easier to use, by including everything via #include <mlpack.hpp>. This will help with increasing uptake.

@rcurtin
Copy link
Member Author

rcurtin commented Jul 26, 2022

Thanks, I still have two things to do here---one is to try the <mlpack.hpp> solution you suggested, and another is to fix the static code analysis checks. I simply haven't really had time to sit down and do this, since I've been traveling for almost three weeks now, but I am hoping this week might be a bit better...

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@rcurtin
Copy link
Member Author

rcurtin commented Jul 31, 2022

Since all the tests pass, I'll merge this now---the idea of a complete mlpack.hpp to include everything is something I'll investigate in a separate PR. (Hopefully, today or tomorrow!)

@rcurtin rcurtin merged commit 19be49c into mlpack:master Jul 31, 2022
@rcurtin rcurtin deleted the util-header-only branch July 31, 2022 11:32
This was referenced Oct 14, 2022
@rcurtin rcurtin mentioned this pull request Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants