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.

Sign up for GitHub

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

Jump to bottom

remove support for Solaris (fixes #5216) #5226

Merged
merged 2 commits into from
May 22, 2022
Merged

remove support for Solaris (fixes #5216) #5226

merged 2 commits into from
May 22, 2022

Conversation

jameslamb
Copy link
Collaborator

Fixes #5216 .
Related to #3513.

Per the discussion in #5217 (comment), this PR proposes removing support for the Solaris operating system in LightGBM.

That support was added only to get {lightgbm} (the R package) onto CRAN, so that it would be easier for R users to install.
There is strong evidence (provided in that comment) that CRAN no longer requires packages to be installable on Solaris.

I'm proposing removing this support for the following reasons:

  1. R Hub is the only easy-to-use option we have found for testing that LightGBM is portable to Solaris, and the fact that CRAN no longer requires such portability means that incidents like [ci] Solaris R hub build is failing: "installation of package ‘Matrix’ had non-zero exit status" #5216 which prevent us from testing on R Hub could easily happen again
  2. LightGBM is already struggling from a lack of maintainer activity, and removing a source of maintenance burden will allow us to focus our limited time and attention on higher-value areas of the project.

guolinke
guolinke approved these changes May 21, 2022
View reviewed changes
Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

I like the PR that deletes codes 😆

StrikerRUS
StrikerRUS approved these changes May 22, 2022
View reviewed changes
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.

Excellent, thanks a lot! Really like to see these changes!

R-package/tests/testthat/test_learning_to_rank.R
expect_true(abs(eval_results[[2L]][["value"]] - 0.745986) < TOLERANCE)
expect_true(abs(eval_results[[3L]][["value"]] - 0.7351959) < TOLERANCE)
}
})

test_that("learning-to-rank with lgb.cv() works as expected", {
testthat::skip_if(
ON_SOLARIS || ON_32_BIT_WINDOWS
ON_32_BIT_WINDOWS
, message = "Skipping on Solaris and 32-bit Windows"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
, message = "Skipping on Solaris and 32-bit Windows"
, message = "Skipping on 32-bit Windows"

@guolinke guolinke merged commit b077415 into master May 22, 2022
@guolinke guolinke deleted the remove-solaris branch May 22, 2022 01:28
@jameslamb
Copy link
Collaborator Author

Thanks for merging this @guolinke , but I didn't get a chance to address the suggestion in #5226 (comment). I'll open a follow-up PR to address that.

In the future, can you please avoid merging before all suggestions have been addressed?

@guolinke
Copy link
Collaborator

Thanks for merging this @guolinke , but I didn't get a chance to address the suggestion in #5226 (comment). I'll open a follow-up PR to address that.

In the future, can you please avoid merging before all suggestions have been addressed?

Sorry, I just saw 2 approves, will check the details next time.

@jameslamb
Copy link
Collaborator Author

no problem! @StrikerRUS, @jmoralez and I sometimes approve but with minor suggestions, meaning "I approve assuming you fix this minor suggestion", to avoid another round of reviews.

@guolinke
Copy link
Collaborator

no problem! @StrikerRUS, @jmoralez and I sometimes approve but with minor suggestions, meaning "I approve assuming you fix this minor suggestion", to avoid another round of reviews.

Actually, I tried the GitHub App on my mobile phone to review issues and codes yesterday, and didn't realize it hides the approved comment by default 🤣 .

@jameslamb jameslamb mentioned this pull request May 23, 2022
Merged
This was referenced May 23, 2022
Merged
Merged
jameslamb pushed a commit that referenced this pull request Jul 29, 2022
@btrotta
Use double precision in threaded calculation of linear tree coefficie…
44d3718 
…nts (fixes #5226) (#5368)
@jameslamb jameslamb mentioned this pull request Aug 29, 2022
Merged
@jameslamb jameslamb mentioned this pull request Oct 7, 2022
Closed
40 tasks
@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 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@StrikerRUS StrikerRUS StrikerRUS approved these changes

@guolinke guolinke guolinke approved these changes

@jmoralez jmoralez Awaiting requested review from jmoralez jmoralez is a code owner

@shiyu1994 shiyu1994 Awaiting requested review from shiyu1994 shiyu1994 is a code owner

Assignees
No one assigned
Labels
breaking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ci] Solaris R hub build is failing: "installation of package ‘Matrix’ had non-zero exit status"
3 participants
@jameslamb @guolinke @StrikerRUS