mlr-archive / mlr-tutorial

The mlr package online tutorial
http://mlr-org.github.io/mlr/
20 stars 11 forks source link

Build tutorial using make / Makefile #144

Closed mb706 closed 6 years ago

mb706 commented 6 years ago

This uses make to create the documentation; also create the documentation in a local folder that is in .gitignore (#138)

The advantages of this are:

Drawbacks are:

This should solve the Travis timeout issues.

Also, this fixes the renamed Task ids which was also started in #140 and contains #143 (change in generatePartialDependenceData) as well as part of #139.

mb706 commented 6 years ago

How does this work?

Instead of having one build build script that does everything, there are the knitIt.Rexec, purlIt.Rexec, and convertIt.Rexec scripts that build the *.md files, build the raw .R code files, and convert the .md files that use pdf images to md files that use svg images. These scripts are executed by the Makefile in the right order & only when necessary. Another script publish.Rexec does the handling of moving the devel folder to a new name when the mlr version updates.

The build script now just calls make -j *number_of_cores* with a small info message.

mb706 commented 6 years ago

Travis runs in 30 minutes (of which 10 minutes are setup & package installation time). Since the old build system has had issues with Travis timeouts I recommend to merge this ASAP.

mb706 commented 6 years ago

Note: I had to comment out a plot because the behaviour of plotPartialDependence apparently changed its behaviour. I think it would be good to merge this PR because then all the other changes in the pipeline can also be merged easier, but this FIXME would then need to be fixed soon afterwards.

mb706 commented 6 years ago

Note 2: This only contains a part of #139, this was necessary to get this to pass on Travis, but @pat-s should probably have a look after this is merged if additional changes should be made.

jakob-r commented 6 years ago

So to clarify... @pat-s made a solution to render the tutorial with pkgdown and now you put quite some time in improving the old build which would become outdated when we use pkgdown. Depending on what option we choose one of you would have wasted a good amount of time.

Please help me to understand.

mb706 commented 6 years ago

Well you don't have to use this ;-) This PR would be a good interim solution before pkgdown goes live. It makes it possible to merge some of the PRs that are in the pipeline because of Travis timeout issues right now.

jakob-r commented 6 years ago

Okay. Of course it's good to improve the build process and "keep the system alive" with that. I just wanted to make sure that no one will be offended. It's a bit confusing that all these Rmd changes are also part of this PR but i can understand, that you wanted to get it working.

mb706 commented 6 years ago

Chicken and egg problem: Right now the (gh-main branch of the) tutorial is in a state where it does not run with the current mlr version (because of renamed Task IDs etc.), but the PRs fixing that are not getting merged because Travis times out (and, by now, because multiple things are broken at once).

I can also take the modified .Rmd files out of this pull request, but that would again leave the tutorial in a state where it does not compile.

pat-s commented 6 years ago

I'm not offended atlhough I don't really see the need for this currently. We agreed to switch to pkgdown so I think the time could have been invested into something else.

Regarding the chicken and egg problem: If we merge https://github.com/mlr-org/mlr/pull/2153 (what I just did) then all the PR in mlr-tutorial that fix the issues can pass.

mb706 commented 6 years ago

That fixes the plotPartialDependence issue, and Travis runs fine with this.

I now took out the .Rmd changes to better split PRs by functionality--This obviously causes Travis to fail, but see the green tick mark above, this PR in itself works fine. Right now the remaining necessary changes are part of #139, as well as #140 and #143. Probably none of them can pass individually, but I'm pretty sure #140 and #143 can be merged right away (and the handling_of_spatial_data.Rmd part of #139?).

pat-s commented 6 years ago

Since we have the pkgdown site now, we can close here.