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-package] Remove ability to install precompiled lib_lightgbm (fixes #3320) #3360

Merged
merged 19 commits into from
Oct 10, 2020

Conversation

jameslamb
Copy link
Collaborator

This PR has two changes that I couldn't see a clean way to separate, so I've combined them:

I think that the use of a precompiled lib_lightgbm.so in the R package has been broken for a while: #1730, #2714 . This was a valuable option before we started linking to R when building the library (added in #2901 ) and you could just download lib_lightgbm.so from the releases page. Now, it is really difficult to manually build a library that works well with the R package.

I think we should eliminate documentation and code about using a pre-built lib_lightgbm, and focus on making customizations you can't get with the CRAN package easier via #2441 (like I recommended in #3354 (comment))

current README

image

proposal

image

@lorenzwalthert
Copy link

lorenzwalthert commented Sep 7, 2020

Looks good overall. One minor thing: I think the distinction between CPU and GPU packages is very important and hence one could mention that the CRAN package and pre-compiled binaries don't support GPU training (If I got that right). One can figure that out from the header on how to build a GPU accelerated version, but explicit is better than implicit often.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments.

R-package/README.md Outdated Show resolved Hide resolved
R-package/README.md Outdated Show resolved Hide resolved
R-package/README.md Outdated Show resolved Hide resolved
R-package/README.md Show resolved Hide resolved
R-package/README.md Show resolved Hide resolved
@jameslamb
Copy link
Collaborator Author

Looks good overall. One minor thing: I think the distinction between CPU and GPU packages is very important and hence one could mention that the CRAN package and pre-compiled binaries don't support GPU training (If I got that right). One can figure that out from the header on how to build a GPU accelerated version, but explicit is better than implicit often.

thanks @lorenzwalthert . I disagree with this. As you mentioned in #3320, the README is already hard to follow and has a lot of information. I want to make this as simple as possible.

I think anyone arriving at this page looking to build for the GPU will be drawn to "Installing a GPU-enabled Build" in the list of options.

image

@jameslamb jameslamb requested a review from StrikerRUS September 8, 2020 02:50
@jameslamb
Copy link
Collaborator Author

I just merged master into this. This is ready for review and does not need to wait for #3338

R-package/README.md Show resolved Hide resolved
@jameslamb
Copy link
Collaborator Author

I just merged master into this. Since we've agreed on #3360 (comment), I'll merge this one it builds.

@jameslamb jameslamb merged commit 785eb60 into microsoft:master Oct 10, 2020
@jameslamb jameslamb deleted the docs/r-install branch October 10, 2020 23:50
@github-actions
Copy link

This pull request 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 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

R README installation instruction hierarchy
4 participants