Lets try auto generated .R and .cpp files #9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For what it is worth, and as a general rule, I don't bother with either Windows or macOS at Travis. I also had some serious issues with timeouts for one large (and very C++-heavy with "expensive" C++) project where I ... eventually gave up, switched to a solution where I supply my own Docker container and use If macOS and Windows are enormous pain (and I saw your Travis build times on the weekend) I would not be opposed to skipping them for now to allow general progress. Now @rcurtin and @coatless may feel different about this but IIRC they are also both card carrying members of the "GitHub actions fixes everything" school I have not yet attended... |
|
I actually don't know much about Github Actions... someday I will have to learn though. :) @Yashwants19 do you have a link to the build or anything? If you're building with multiple cores, you can probably use just 1 instead and it will help reduce RAM usage. But I am quite surprised that the R compilation is using so much RAM. (update: oh... this is a PR not an issue. There is the build link... heh... let me take a look...) |
|
Hmm, I must not be looking in the right place though. The only error I see here is and not anything about a failed memory allocation. |
Aye, I'm the only one that experiments with it since I lack a dedicated home server for running CRON jobs.
I'm also not opposed to this. At some point, we probably will need to clear it with CRAN on the amount of time required to build the package.
Need to download the https://ci.appveyor.com/project/Yashwants19/rcppmlpack/builds/33823899/artifacts Full text: 00install.out from RcppMLPACK.RcheckI do not really use Appveyor (go GitHub Actions!). That said, on TravisCI, I would frequently set the following build environment flags: env:
global:
- MAKEFLAGS="-j 2"
- R_BUILD_ARGS="--no-build-vignettes"
- R_CHECK_ARGS="--no-build-vignettes"
- _R_CHECK_FORCE_SUGGESTS=0 where |
|
Is it actually necessary to include However, I am not sure if that applies to this repository---if this build timeout is not from the auto-generated bindings but instead from the hand-maintained RcppMLPACK package, then what I wrote above doesn't apply. (It should, however, apply to the bindings we generate by the time the summer is over, thus meaning that we shouldn't have build time or memory issues... I hope!) |
Yashwants19
force-pushed
the
Lets-try-auto-generated
branch
from
8cc5c26
to
a1f42e5
Compare
Yashwants19
force-pushed
the
Lets-try-auto-generated
branch
from
3bab9bc
to
ff16a23
Compare
|
Just ran it locally (after again increasting Version: in DESCRIPTION to get a distinct tarball, as I have done before). Tests very cleanly. Nice work. I get I set of warnings (tickmarks from ✔ checking for sufficient/correct file permissions
✔ checking serialization versions ...
W checking whether package ‘RcppMLPACK’ can be installed (2.8s)
Found the following significant warnings:
./mlpack/core/data/load_impl.hpp:116:30: warning: ‘static arma::file_type arma::diskio::guess_file_type(std::istream&)’ is deprecated [-Wdeprecated-declarations]
./mlpack/core/data/load_impl.hpp:116:52: warning: ‘static arma::file_type arma::diskio::guess_file_type(std::istream&)’ is deprecated [-Wdeprecated-declarations]
./mlpack/core/data/load_impl.hpp:180:32: warning: ‘static arma::file_type arma::diskio::guess_file_type(std::istream&)’ is deprecated [-Wdeprecated-declarations]
./mlpack/core/data/load_impl.hpp:180:54: warning: ‘static arma::file_type arma::diskio::guess_file_type(std::istream&)’ is deprecated [-Wdeprecated-declarations]
./mlpack/core/data/load_impl.hpp:116:45: warning: ‘static arma::file_type arma::diskio::guess_file_type(std::istream&)’ is deprecated [-Wdeprecated-declarations]
./mlpack/core/data/load_impl.hpp:180:47: warning: ‘static arma::file_type arma::diskio::guess_file_type(std::istream&)’ is deprecated [-Wdeprecated-declarations]
See ‘/tmp/file2b76b946f6ad43/RcppMLPACK.Rcheck/00install.out’ for details.
✔ checking installed package size
✔ checking package directory ... Is that me? Is my MLPACK version too old or is that something we should look into? The rest is spotless. Very nice to see! |
Did you check, over this PR(branch) or master branch? Sorry I didn't mention this earlier, but earlier I mistakenly push these changes to master, then again revert these changes from master and make a PR with a new branch with these changes.
I think so these warning are because MLPACK is still supporting Armadillo 8.400.0, and we are using latest Armadillo. |
|
Now building in the slightly irregularly named 'Lets-try-auto-generated' (that is at least very descriptive ;-) ) that I was famililar from based on the traffic here. Seems like it takes a moment to build. Are we building MLPACK each time? [ To be continued... ] [ Holy crap it is still building. Only the package to build vignettes. You are one patient man. I could not develop this way. ] Eventually finished and can get tested at which point my use of and these docs I guess we can write (or better still, derive?) But it builds and that is Yuge. Congrats. |
Feel free to adapt the implementation of |
With appropriate flags to g++ it gets quieter. I'll file a separate issue. |
I was also having the same concern, Should we have to strip the shared-library, or we can manage this issue in some other way?
Yeah, this is my next goal for this week. |
Yashwants19
commented
Jul 6, 2020
src/mlpack/core/util/arma_config.hpp
Outdated
| @@ -15,8 +15,18 @@ | |||
| #ifndef MLPACK_CORE_UTIL_ARMA_CONFIG_HPP | |||
| #define MLPACK_CORE_UTIL_ARMA_CONFIG_HPP | |||
|
|
|||
| #define MLPACK_ARMA_NO64BIT_WORD | |||
| #ifdef ARMA_64BIT_WORD | |||
There was a problem hiding this comment.
This file is auto-generated while building mlpack, and this file is system dependent. But we are building the binding using R and not cmake, Hence we have to take care of this file on our own. I have tried to config this, please guide me whether my approach is correct or not.
There was a problem hiding this comment.
Right, the only reason this file exists is to warn the user if they compiled libmlpack.so with ARMA_64BIT_WORD set but are now compiling something against libmlpack.so without that define set (or vice versa). But since we are compiling everything with R, all at once, at the time that install.packages() is called (or similar), then this situation isn't possible. To be honest, I think that you could work around it by leaving the file as-is, and then simply "cheating" and doing
#define MLPACK_CORE_ARMA_UTIL_CONFIG_HPP
in RcppMLPACK.h before including mlpack/core.hpp or another mlpack header. That way, when this file is encountered, it will simply be skipped. This way, we avoid displaying any spurious warnings, and you don't need to do any modification of mlpack's sources. 👍
There was a problem hiding this comment.
Yes, sounds legit. Many moons ago we also sorted the (simpler) case of alwasy calling (Rcpp)Armadillo headers before Rcpp headers. We control the space and build---so we can just define as needed.
There was a problem hiding this comment.
This is working great thank you. I will also update this in the main(Yashwants19/mlpack) repository.
…s19/RcppMLPACK into Lets-try-auto-generated
|
Minor note: you also removed |
|
I have added |
rcurtin
approved these changes
Jul 18, 2020
There was a problem hiding this comment.
I'm not sure if you're waiting for me to approve this one, but at least from my end it looks good. However, @eddelbuettel is the R expert, not me, so he may have some additional comments. 😃
|
Looks like I ran it days ago as my checkout was current (apart from the belated c542914) . Will run it once more now but I think we can proceed... |
|
Yes thumbs up. So thumbs up. |
coatless
approved these changes
Jul 18, 2020
|
Okay lets merge this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @eddelbuettel @coatless @rcurtin, In this PR I have copy-paste the folder(build/src/mlpack/bindings/R/mlpack) generated by cmake in main(mlpack) repository with some small changes(Rename mlpack ---> RcppMLPACK and increase time of travis build) in it.
Currently I am facing some problem in windows build.