WIP: [R-package] new release to fix CRAN error on 32-bit Windows #3586

jameslamb · 2020-11-22T01:04:38Z

The R package is currently at risk of being dropped from CRAN, because one of the learning-to-rank tests is failing on 32-bit Windows + R 3.6.

https://cran.r-project.org/web/checks/check_results_lightgbm.html

#3585 documents the problem, and describes a "quick fix" to get the package fixed on CRAN as well as a "long-term fix" that will better solve this problem. This PR has the "quick fix".

Notes for Reviewers

I branched off of master at tag v3.1.0, to avoid including anything that has been merged in since that release. I think we should keep this PR open until {lightgbm} is fully accepted on CRAN with no issues. Once that happens and this can be merged, I'll update it to latest master and undo the changes to VERSION.txt.

I found that I'm able to reproduce the errors from CRAN using R Hub (#3585 (comment)), and I've confirmed with those same steps that this branch's changes resolve the problem.

~~Once either @guolinke or @Laurae2 or @StrikerRUS approves, I'll submit 3.1.0.1 to CRAN with an explanation about why we are submitting again so soon.~~ #3586 (comment)

guolinke · 2020-11-22T01:14:00Z

R-package/tests/testthat/test_learning_to_rank.R

@@ -48,14 +49,17 @@ test_that("learning-to-rank with lgb.train() works as expected", {
    }
    expect_identical(sapply(eval_results, function(x) {x$name}), eval_names)
    expect_equal(eval_results[[1L]][["value"]], 0.775)
-    if (!ON_SOLARIS) {
+    if (!(ON_SOLARIS || ON_32_BIT_WINDOWS)) {


maybe it is better to relax the constraint values? in case there are more tests may fail

do you mean change the numeric tolerance?

I think it's better to just completely skip these tests for now to be sure we don't get kicked off of CRAN or waste their time with too many submissions. I don't know the exact amount that the values are different by, and I'm not sure I can figure that out in a way that's guaranteed to be the same as CRAN's environment.

By the next LightGBM release, we can try to completely fix this instead of just skipping the tests.

To be clear, in GitHub Actions here these tests are still running on Linux, Mac, and 64-bit Windows, with multiple different compilers, and for R 3.6 and R 4.0. So this feature is still being thoroughly tested.

R-package/cran-comments.md

StrikerRUS · 2020-11-22T01:34:21Z

R-package/cran-comments.md

@@ -1,9 +1,21 @@
 # CRAN Submission History

-## v3.0.1 - Submission 1 - (November 15, 202)
+## v3.1.0.1 - Submission 1 - (November 21, 2020)


I'll submit 3.1.0.1 to CRAN with an explanation about why we are submitting again so soon.

I think it can be 3.1.1 regular release for the whole project as we haven't agreed on #3210 yet (4-part versioning).

I'm fine with doing a 3.1.1 release of the whole project, but that generates some extra work for you and @guolinke , so I thought it would be simpler to do an R-only version change.

adding a 4th version number just for the purpose of getting to CRAN is what we did in the last submission cycle (#3338 (comment)).

Let's let @guolinke be the tie-break (choose either 3.1.0.1 or 3.1.1 for the new CRAN submission). If it's 3.1.1, I can update this PR accodingly.

adding a 4th version number just for the purpose of getting to CRAN is what we did in the last submission cycle

Please note that CI cannot be run due to merge conflicts. You simply cannot generate artifact that should be uploaded to CRAN (you can do it only manually on your machine, but then what was the reason to add r_artifacts.yml workflow?..)

you can do it only manually on your machine, but then what was the reason to add r_artifacts.yml workflow?

So that checks that need to be run manually, like the Solaris ones, don't have to always be done by me, in case I'm unavailable. Anyone could generate the package on GitHub Actions with that comment, then upload it to https://builder.r-hub.io/.

I just looked at the list of recently-merged PRs and it doesn't seem like there are any new features, just small fixes, so I'm fine with making this 3.1.1. I'll update this branch to the latest master and change the version.

we can update it to 3.1.1, but only submit it cran.

we can update it to 3.1.1, but only submit it cran.

I guess it will be confusing for users. And we will have a bunch of issues with a content like "When 3.1.1 version will be released at PyPI?"

I just check the commit log, there are only “maintenance” change, not any bug fixes or new features. Therefore, I don't think we should release a new version.

besides, releasing a new version in such a short time seems we have an emergence bug fix.

I don't think we should release a new version.

Ok, then I think we should use 3.1.0.1 as the version number for CRAN. We have to increment the version number when we re-submit.

By the way @guolinke , has CRAN emailed you? I think they should be done running all their checks.

There are 12 checks done at https://cran.r-project.org/web/checks/check_results_lightgbm.html

which is how many all other packages have (for example, {jsonlite})

But I agree with @StrikerRUS 's comment in #3586 (comment) that we shouldn't submit again until CRAN asks.

I just check the commit log, there are only “maintenance” change, not any bug fixes or new features.

Today we merged 2 bugfixes. And there can be more "fix" or "features" PRs if we wait some days for CRAN response.

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>

StrikerRUS · 2020-11-22T02:02:17Z

I'll submit 3.1.0.1 to CRAN with an explanation about why we are submitting again so soon.

BTW, shouldn't we wait for Sanitizers and Valgrind results to not bother CRAN maintainers with frequent submissions?

jameslamb · 2020-11-22T02:06:16Z

I'll submit 3.1.0.1 to CRAN with an explanation about why we are submitting again so soon.

BTW, shouldn't we wait for Sanitizers and Valgrind results to not bother CRAN maintainers with frequent submissions?

I think that they might have already run. Unfortunately I don't know how to tell for sure. When the package was posted on November 18, under the "check results" at https://cran.r-project.org/web/checks/check_results_lightgbm.html you could still see an entry for valgrind, but it was the old entry.

Now that entry has been removed, and there's no mention of valgrind.

But now that you make me think about it, you're right....let's wait to re-submit until they email me or @guolinke (I'm not sure who will get the email this time) telling us what we have to do to stay on CRAN.

…ix/cran-tests

guolinke · 2020-11-23T02:36:40Z

@jameslamb I didn't receive any emails from CRAN yet.

jameslamb · 2020-11-23T16:36:13Z

I just moved the test changes in this PR to #3588 , so we can get that fix onto master soon and have other PRs integrate it.

I've marked this PR work-in-progress and changed the title, since it will have to stay open until CRAN asks us for a new submission

guolinke · 2020-11-26T03:33:37Z

I think we can set a timeline for this, for example, if CRAN still doesn't ask us in two weeks, we can directly release 3.1.1, and submit it to cran as well.

jameslamb · 2020-11-26T03:40:53Z

I think we can set a timeline for this, for example, if CRAN still doesn't ask us in two weeks, we can directly release 3.1.1, and submit it to cran as well.

I agree!

If CRAN hasn't asked us to resubmit by the end of this week, I'd feel comfortable documenting that in the repo and publicizing it on social media. Since even if they surprise us in a few days and ask for a resubmission, we're very confident that we know how to fix it (unlike last time when I wasn't really sure).

StrikerRUS · 2020-11-29T16:08:34Z

I think CRAN team hasn't plans to response this time. Can we start releasing 3.1.1 version?
I'd like to include

in 3.1.1 as well.

jameslamb · 2020-11-29T20:24:12Z

I think CRAN team hasn't plans to response this time. Can we start releasing 3.1.1 version?
I'd like to include

Check max_bin, etc. match config when using binary #3592

[R-package] construct dataset earlier in lgb.train and lgb.cv (fixes #3583) #3598

in 3.1.1 as well.

I agree! Unfortunately I opened this PR as a branch from my fork (because originally it was just the test fix). I'm going to close this PR and open the release 3.1.1 PR on a LightGBM branch we can all push to.

github-actions · 2023-08-24T04:01:39Z

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.

[R-package] skip learning-to-rank tests on 32-bit Windows

b5b6704

jameslamb requested review from guolinke and StrikerRUS November 22, 2020 01:04

jameslamb requested a review from Laurae2 as a code owner November 22, 2020 01:04

jameslamb added fix maintenance and removed fix labels Nov 22, 2020

guolinke reviewed Nov 22, 2020

View reviewed changes

StrikerRUS approved these changes Nov 22, 2020

View reviewed changes

Update R-package/cran-comments.md

9b1ccaf

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>

jameslamb added 3 commits November 22, 2020 02:21

v3.1.1

faab95d

merge master

248bb5f

Merge branch 'fix/cran-tests' of github.com:jameslamb/LightGBM into f…

d4905c9

…ix/cran-tests

jameslamb requested a review from StrikerRUS November 22, 2020 02:26

jameslamb mentioned this pull request Nov 22, 2020

[ci] test 32-bit R in CI #3588

Merged

linting

97109cd

jameslamb added the awaiting response label Nov 23, 2020

jameslamb changed the title ~~[R-package] skip learning-to-rank tests on 32-bit Windows~~ WIP: [R-package] new release to fix CRAN error on 32-bit Windows Nov 23, 2020

Merge branch 'master' into fix/cran-tests

c25b839

jameslamb closed this Nov 29, 2020

jameslamb mentioned this pull request Nov 29, 2020

release 3.1.1 #3611

Merged

16 tasks

jameslamb deleted the fix/cran-tests branch November 30, 2020 02:34

jameslamb mentioned this pull request Nov 30, 2020

[R-package] {lightgbm} is now available on CRAN (fixes #629) #3612

Merged

github-actions bot removed the awaiting response label Aug 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: [R-package] new release to fix CRAN error on 32-bit Windows #3586

WIP: [R-package] new release to fix CRAN error on 32-bit Windows #3586

jameslamb commented Nov 22, 2020 •

edited

Loading

guolinke Nov 22, 2020

jameslamb Nov 22, 2020

StrikerRUS Nov 22, 2020

jameslamb Nov 22, 2020

StrikerRUS Nov 22, 2020

jameslamb Nov 22, 2020

guolinke Nov 22, 2020 •

edited

Loading

StrikerRUS Nov 22, 2020

guolinke Nov 23, 2020

guolinke Nov 23, 2020

jameslamb Nov 23, 2020

StrikerRUS Nov 23, 2020

StrikerRUS commented Nov 22, 2020

jameslamb commented Nov 22, 2020

guolinke commented Nov 23, 2020

jameslamb commented Nov 23, 2020

guolinke commented Nov 26, 2020

jameslamb commented Nov 26, 2020

StrikerRUS commented Nov 29, 2020

jameslamb commented Nov 29, 2020

github-actions bot commented Aug 24, 2023

WIP: [R-package] new release to fix CRAN error on 32-bit Windows #3586

WIP: [R-package] new release to fix CRAN error on 32-bit Windows #3586

Conversation

jameslamb commented Nov 22, 2020 • edited Loading

Notes for Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guolinke Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrikerRUS commented Nov 22, 2020

jameslamb commented Nov 22, 2020

guolinke commented Nov 23, 2020

jameslamb commented Nov 23, 2020

guolinke commented Nov 26, 2020

jameslamb commented Nov 26, 2020

StrikerRUS commented Nov 29, 2020

jameslamb commented Nov 29, 2020

github-actions bot commented Aug 24, 2023

jameslamb commented Nov 22, 2020 •

edited

Loading

guolinke Nov 22, 2020 •

edited

Loading