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

R README installation instruction hierarchy #3320

Closed
lorenzwalthert opened this issue Aug 19, 2020 · 7 comments · Fixed by #3360
Closed

R README installation instruction hierarchy #3320

lorenzwalthert opened this issue Aug 19, 2020 · 7 comments · Fixed by #3360
Assignees

Comments

@lorenzwalthert
Copy link

lorenzwalthert commented Aug 19, 2020

The README in master/R-package has a title hierarchy that is a bit confusing to me. Here I summarized the part of it that I find hard to follow:

Installation
------------
...

### Installing the CRAN package
...

### Installing Precompiled Binaries

...

### Preparation (<- this is a bit confusing to me. 
                      Should it be hierarchy lower and have a header 'Building and installing from source'  
                      or similar that also contains the '### Install'?)

...

#### Windows Preparation

...

#### Windows Toolchain Options

...

#### Mac OS Preparation

...

### Install

...


Examples
--------

...

I am excited about recent efforts to distribute this package via CRAN, so maybe this README is WIP anyways and the issue is redunant. Also, since it's quite a long README and contains some information for maintainers and not end users, maybe it's worth considering to move some of this content to vignettes. In any case, thanks for this package.

@jameslamb
Copy link
Collaborator

Hi @lorenzwalthert , thanks for raising this and for using LightGBM!

This README has grown quite large, you're definitely right. We've tried to preserve prior section headings as much as possible, so that old links people have bookmarked or posted in stack overflow answers still work.

The CRAN maintainers are taking a much-deserved break right now (https://cran.r-project.org/), but when they're back on August 24 we'll be submitting again, and I think that we have a very good chance of being accepted this time.

Our challenge is that the CMake-based path for installation has been around for a while, and we cannot break it. It can also produce a more efficient package. For example, building on Windows with Visual Studio (which is not supported for CRAN), produces a package that can better take advantage of parallelism than the CRAN one compiled with MINGW.

All that said....I think moving all the installation options to a vignette all by itself is a great idea! xgboost has a similar situation, with both CRAN-based and CMake-based installation, and I like that they've separated it out into its own thing: https://xgboost.readthedocs.io/en/latest/build.html#id14

To preserve backwards compatibility with old links, I think we'd leave all the same headings in the README but have them to point to a new installation vignette on our pkgdown site. Maybe as a part of #1944

Could you propose a sequence of headings that you think would be easier to navigate?

@lorenzwalthert
Copy link
Author

lorenzwalthert commented Aug 19, 2020

Thanks @jameslamb. I did not think of back-linking 🙃.

To preserve backwards compatibility with old links, I think we'd leave all the same headings in the README but have them to point to a new installation vignette

If you want to preserve all these links, that sounds reasonable. Or thinking about which headers were there a long time already and can't be removed, while some more recent ones are probably not back-linked as much.

I don't know if you can change the hierarchy of the header with preserving linking, but as I said above, you have Installing the CRAN package, then Installing Precompiled Binaries and then Preparation. I was wondering what I need to prepare now? I.e. it seems as preparation and install should be subsumed under a new header that has the same level as the first two ### installation headers (as I indicated above) to describe option 3, if that was the idea. If not, then maybe I just don't get it and things are fine 😄. If you don't want to change the headers at all, maybe some more context could be given directly under it, so the reader knows what follows. I.e

### Preparation

As an alternative to the CRAN source installation and the binary installation from GitHub described above, 
the next sections describes how to build and install the package from source, which comes with additional
built tool requirements and configuration options.

(no idea if that's entirely correct but this kind of information would be useful for people who don't have a clue about different compiler and built tool chains). Then, people like me would be shied away and head back to the save place further up in the REAME and choose the CRAN source installation 😄.
And hopefully soon, thanks to your endeavour, soon just type install.packages("lightgbm") without noticing all the work that went into making this package available from CRAN.

@jameslamb
Copy link
Collaborator

jameslamb commented Aug 19, 2020

oooooo I see. Ok so basically after the ### Installing the CRAN package, it isn't obvious that you're DONE and can just use it. So maybe it should be like

## Installation Options

For the easiest installation, go to <link to CRAN package instructions>.

If you experience any issues with that, try <link to precompiled binaries instructions>.

To build a GPU-enabled version of the package, follow the steps in <link to CMake preparation> and then go to
<R + GPU preparation>.

If you are on a Windows system with Visual Studio, you can build a more efficient
version of the library by following <link to MSVC installation instructions>.

If any of the above options do not work for you or do not meet your needs,
go to <building with CMake instructions>.

@lorenzwalthert
Copy link
Author

lorenzwalthert commented Aug 20, 2020

Yes. For me, it's clear that the first two ### headers are alternative methods for installation (because the follow the pattern "installation..." in their title) but the third header does not really fit in this pattern, so such an explanation could definitively help.

@jameslamb
Copy link
Collaborator

ok makes sense, thank you! I'll take a shot at re-writing it and will ask for your feedback on the PR.

I hope that a week from now, I can tell you we're on CRAN 😎

@jameslamb
Copy link
Collaborator

@lorenzwalthert Thanks again for using LightGBM and for taking the time to work with me on this issue.

Could you please take a look at the PR I just opened (#3360 ) and let me know what you think? Basically I've proposed reorganizing it like this:

image

jameslamb added a commit that referenced this issue Oct 10, 2020
…3320) (#3360)

* [R-package] [docs] Reorganize installation instructions (fixes #3320)

* more changes

* remove ability to use precompiled lib_lightgbm

* remove language about installing from the CRAN section

* move installation stuff

* Apply suggestions from code review

Co-authored-by: Nikita Titov <[email protected]>

* fix anchor

Co-authored-by: Nikita Titov <[email protected]>
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants