-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Make mlpack completely header-only #3250
Conversation
… properties of libmlpack.so.
@rcurtin I thought more about the "include everything in one header" issue (follow-up to #3233). Rather than making the "one header" approach mandatory and converting all of the codebase, I suggest making it an option. With this, both the old and new ways of including mlpack functionality would work. More specifically:
|
@conradsnicta I did some serious digging into this issue. Fundamentally simply the cost of including all of mlpack's headers is not too expensive although it is noticeable. If I make a header that includes everything---except the code that enables serialization for ANN layers (more on that later)---and I include this in the
(I found that gcc's precompiled headers did not help much.) So fundamentally it is not too painful to include all of the headers, and I agree that your approach of supplying an Now, about the crazy serialization numbers: this is what surprised me, though in retrospect it makes perfect sense. Including this file causes compile times to take a minimum of one minute and use a minimum of 3GB of RAM. It also makes the resulting programs much larger! The reason, as it turns out, is that the compiler must instantiate every single layer that we are allowing to be serializable. This is because cereal must have a constructor definition for any polymorphic class that it might be serializing, since deserialization might encounter an arbitrary layer type. So basically what this boils down to is that any program that includes that file must contain compiled versions of every layer type---which places an insane demand on the compiler, especially if the user is not ever even using a neural network (or serializing one). There is no reasonable way to avoid that compilation cost, or to automatically compile only the layers that a user has explicitly serialized---since a program could feasibly just be loading a model, a user may never actually manually instantiate a layer. So, this presents a dilemma that I plan to solve like this:
(Also, a side note: when the serialization issue is handled correctly, these runtimes are way better than before #2777: compiling Anyway, happy to hear any comments on this approach. However I will implement in a separate PR, since it'll be a lot of moving includes around and other trudgery. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is huge, awesome work.
@rcurtin Thanks for digging into this. Even though there is a slowdown when including everything, I bet that many people would be willing to use this for the sake of convenience and simplicity. For the ANN serialistion problem, there are a few approaches. First, I suggest making a simple define-based option to enable serialisation of ANNs, which is then detected by mlpack headers. This is instead of users directly using
and then mlpack would internally call the appropriate set of Second, serialisation is not execution-time critical, so we can direct the compiler not to spend too much time optimising the produced code. This can be accomplished by adding attributes to serialisation-related functions. As an example, say we want to have the attributed named
Then a serialisation function would be decorated with
I've used a similar approach to decorate Yet another option would be to rewrite the ANN serialisation code, so that all the ANN layers are first converted into a contiguous block of memory (akin to a raw dump). Then Cereal would be used only to serialise that block of memory, instead of hooking into the guts of ANN code. |
Yep, that's exactly what I'm thinking!
This is a nice idea but I think that it would not apply here. The issue is not so much that the code generated by |
@rcurtin Great work on this. I wanted to review it this weekend, but I had no chance. I was facing some DNS issues. |
No worries, there are a few follow-up PRs in progress if you want to review those 😄 and, if you find any issues with this PR, I'll handle any comments that you post. 👍 |
This PR does the last step---removes
libmlpack.so
entirely. This will probably require some adaptation downstream in the examples and models repositories, but, that should be pretty easy (just don't link againstlibmlpack.so
anymore).Most of this PR is CMake reconfiguration and simplification: now that there is no
libmlpack.so
, there's a lot less that we have to do.A shortlist of notable modifications here:
libmlpack.so
is gone, and so linking for bindings and tests is now a little bit simpler.The
arma_config_check.hpp
file, which made sure that the same compilation settings were used inlibmlpack.so
were used when mlpack was included, are no longer necessary, and so all related CMake infrastructure has been removed.The pkgconfig generator is modified so it no longer includes
-lmlpack
in the linker command.mlpack_export.hpp
is no longer needed---so everything related to that is now gone.Documentation is updated to reflect that mlpack is now header-only (maybe it could be updated in more places---I would be interested in people's comments on where to do that).
Finally, there is now only one source file that ever changes as a result of CMake:
src/mlpack/util/gitversion.hpp
. This is already generated directly intosrc/
, and not intobuild/include/
, so there is no compelling reason to make the first step of every build to copy every mlpack header intobuild/include/
. Therefore, I removed themlpack_headers
target, and now there is no more step to copy all of the headers. This should accelerate builds, I hope, or at least remove some of the tedium... the only "downside" is that users used to including thebuild/include/
directory (if they build mlpack without installing, for instance, like I often do), will need to just includesrc/
instead---a minor change.