Upgrade the MLPACK 2.0

[thirdwing]

All serialization part should be removed.

https://github.com/mlpack/mlpack/blob/master/src/mlpack/core/data/serialization_shim.hpp#L122

[eddelbuettel]

Yes, I am looking at some examples and tests in MLPACK and it is a little sad that they don't work in RcppMLPACK -- ie I just hit upon StandardCoverTree and that is apparently 2.0.* only.

[thirdwing]

This can be fixed by embedding the boost::serialization source code.

I will find some time to upgrade this.

[eddelbuettel]

That's because boost_serialization requires header + library rather than just templates?

[thirdwing]

I don't think it is header-only.

https://github.com/boostorg/serialization/tree/develop/src

[eddelbuettel]

I think we are saying the same thing.

[eddelbuettel]

Thinking out loud here: could we maybe informally make this work by relying on BH and 'just' adding the Boost Serialization parts this needs? Burden would then be on us to keep this in sync with the (infrequent) BH updates.

Alternatively, one could depend on the system Boost installation but that may exclude Windows and other platforms. Do you get easily installable and reliable Boost libraries from (home)brew (or alternatives) on OS X?

[thirdwing]

On OSX it is quite easy to install boost, but it is quite annoying on Windows.

I prefer using the BH package and embedding the boost.serialization part.

[eddelbuettel]

I just thought about another way: use an external MLPACK. On Ubuntu 16.04, I got a version 2.0.1 preinstalled. I am now working in (local) branch where I

• removed inst/include/mlpack/
• removed src/mlpack/
• greatly simplified src/Makevars, added linking to -lmlpack
• made one change to src/kmeans.cpp (as it now wants a Row rather than Col

and it all builds. You'd probably loose Windows, or possibly do one of those tricks on pulling in a pre-build static library. Not sure about OS X.

Also possible cans of worms with version drift between Armadillo and MLPACK.

[eddelbuettel]

FWIW I pushed this to a public branch: https://github.com/eddelbuettel/RcppMLPACK/tree/feature/external-mlpack

I may experiment with calling this RcppMLPACKlite to signal that it uses an external MLPACK. Not sure if I find time to work on it though ...

[rcurtin]

I am not sure if there is anything I can do to help here, and I am certain I don't 100% understand the environment in which you are working, but if you wanted to strip serialization support easily, you could replace CreateNVP in serialization_shim.hpp with a function that does nothing, and then remove the overloads of data::Load() and data::Save() in src/mlpack/core/data/load.hpp and src/mlpack/core/data/save.hpp that take a model (and therefore use boost::serialization), then remove everything related to serialization from CMakeLists.txt. I am not sure if that's a desirable solution---or even a solution that does what you need---but I thought I would toss it out there since I'm familiar with how that code is written. :)

[eddelbuettel]

Hi @rcurtin -- good to see you here.

This may have legs. Let's talk more. For starters:

• "we" (as in folks taking code like MLPACK and Armadillo into R) do not even use CMakeLists: for Armadillo, we get all LAPACK/BLAS etc pp from R (as we, after all, "merely" create dynamically loadable modules, rather than executables) and Armadillo gets shipped as 'just headers' -- much easier -- and never link
• skipping / #ifdef-ing save and load makes a lot of sense in that case too as we have R as a 'REPL' around it to load and save data
• similarly Boost is used pretty widely ... but never with linking which is our hold-up here for linking
• lastly, seen Cereal ? (not that I advocate a rewrite; I think I looked at that before and you need Boost Serialization for your restarts or something I now forget)

One thing which Conrad very kindly accomodated was the override of std::cout. R does not like us to use native I/O (but rather use its, so that it is nicely synchronize). Similarly assert() and exit() are verboten (which makes complete sense when you consider that R is the shell and environment).

Does this make sense? I would love to work on this to get MLPACK more tightly into R.

[rcurtin]

I'm happy to help out how I can without compromising our own design. I think that we can work easily within those confines to make life easier for you.

I looked at Cereal, but I chose to go with Boost since we were already requiring other parts of boost anyway. I believe the serialization shim (made so that we can write Serialize() functions instead of serialize() and conform with our style guidelines) was easier with Boost too; it's been a while and I can't remember for sure.

I'm not so excited about #ifdef'ing all of the Serialize() functions; is there any other way? If you are basically just taking the mlpack headers and copying/modifying them to provide as RcppMLPACK, then it seems that the strategy I proposed above could be useful. Maybe not---you would know more than me.

As for the other issues, please go ahead and open some issues on the mlpack Github page; we can discuss them each separately there, and I think it will be a better way to fix the issues (since it'll possibly be more than just my opinion being posted).

[eddelbuettel]

To clarify:

• Conrad is so awesome that all change we need / needed are upstream. I copy his source in rather than patching each release. Now, he release so often that I'd go insane otherwise ... 😬
• We do have a bunch of headers we include first and which condition Armadillo. That evolved over time, and works great. Longer term I'd love to get there.
• But incremental approaches are fine. Lemme tinker some more with a pure external MLPACK (context of my last few posts here) and see how we do there.
• Longer term shipping just headers and not requiring linking (and hence foregoing Boost Serialization) is the way to go for R as we have this benefit of a 'platform' (ie can assume compiler and libraries everywhere)

[rcurtin]

Ok, let me know how your tinkering goes. I like the concept of being awesome, and I also like the concept of upstream being responsive to the needs of downstream (Conrad's help in that manner has been invaluable to us also). So when you are ready, let's start discussing how things in mlpack can change to make it easier for RcppMLPACK. :)

[eddelbuettel]

@rcurtin I have something up and running now over here. It's really just a fork as a distinct master to experiment. But it takes an external -lmlpack. (And I currently use 2.0.2 as there was a Debian package for it which I rebuilt for Ubuntu use in Travis (14.04) and on my 16.04 machines. If you hint that I should really upgrade to 2.0.3 I can....).

Now, when we run R CMD check ... over the tarball created by R CMD build ... from a source repo -- and you can just browse this here thanks to Travis I get

   File ‘RcppMLPACK/libs/RcppMLPACK.so’:
     Found ‘_ZSt4cout’, possibly from ‘std::cout’ (C++)
       Object: ‘kmeans.o’

   Compiled code should not call entry points which might terminate R nor
   write to stdout/stderr instead of to the console, nor the system RNG.

   See ‘Writing portable packages’ in the ‘Writing R Extensions’ manual.

That seems to come from the sole std::cout use in core/util/prefixedoutstream_impl.hpp. Any chance we can that one use a fallback to something else?

For example, we can use the trick we settled on with Conrad and provide an overrideable fallback:

#if !defined(ARMA_DEFAULT_OSTREAM)
#define ARMA_DEFAULT_OSTREAM std::cout
#endif
    ARMA_DEFAULT_OSTREAM << std::endl;

Now, when I include RcppArmadillo.h as well (to get this from RcppArmadolloConfig.h, really) it all passes.

Is that something we could entertain? If I'd be happy to pivot over to an issue at your repo. Also, maybe follow the the RcppMLPACK for over here if you could.

(Edited to add missing ! in condition above)