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

[ci] [R-package] add CI jobs covering more CRAN "additional checks", fix R_NO_REMAP warnings (fixes #6369) #6523

Merged
merged 22 commits into from
Jul 14, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jul 5, 2024

Contributes to #6522
Fixes #6116
Fixes #6369
Replaces #6381 and #6276

This attempts to cover more of the CRAN "Additional Checks" (link) with the new rhub container images.

I originally started this just to try to get a CI job that could have caught #6522. But then realized that this could also be extended, with minimal extra complexity, to significantly improve the coverage of the CRAN checks.

So this PR now does the following:

./include/LightGBM/utils/log.h:19:9: warning: 'R_NO_REMAP' macro redefined [-Wmacro-redefined]
   19 | #define R_NO_REMAP
      |         ^
<command line>:8:9: note: previous definition is here
    8 | #define R_NO_REMAP 1
      |         ^
1 warning generated.

(ref: dmlc/xgboost#10461)

Notes for Reviewers

Happy to say that this would have caught #6522!

I saw exactly that WARNING in the build logs: https://github.com/microsoft/LightGBM/actions/runs/9803295968/job/27069256021?pr=6523

And that then went away once I merged in #6514 : https://github.com/microsoft/LightGBM/actions/runs/9803499820/job/27069753658?pr=6523

@jameslamb jameslamb changed the title WIP: [ci] [R-package] add CI jobs testing newer compilers WIP: [ci] [R-package] add CI jobs testing newer compilers (fixes #6369) Jul 5, 2024
@jameslamb jameslamb changed the title WIP: [ci] [R-package] add CI jobs testing newer compilers (fixes #6369) WIP: [ci] [R-package] add CI jobs covering more CRAN "additional checks", fix R_NO_REMAP warnings (fixes #6369) Jul 5, 2024
@jameslamb jameslamb marked this pull request as ready for review July 5, 2024 05:45
@jameslamb jameslamb changed the title WIP: [ci] [R-package] add CI jobs covering more CRAN "additional checks", fix R_NO_REMAP warnings (fixes #6369) [ci] [R-package] add CI jobs covering more CRAN "additional checks", fix R_NO_REMAP warnings (fixes #6369) Jul 5, 2024
@jameslamb jameslamb requested a review from StrikerRUS July 5, 2024 05:45
Rscript -e "install.packages(c('R6', 'data.table', 'jsonlite', 'knitr', 'markdown', 'Matrix', 'RhpcBLASctl', 'testthat'), repos = 'https://cran.rstudio.com', Ncpus = parallel::detectCores())"
sh build-cran-package.sh
R CMD check --as-cran --run-donttest lightgbm_*.tar.gz || exit 1
R CMD check --as-cran --run-donttest lightgbm_*.tar.gz || true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please elaborate on this particular change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

R CMD check redirects the output of compiling the library + installing it to a separate file. To be able to always see those logs in CI, we cat out that file:

echo ""
echo "install logs:"
echo ""
cat lightgbm.Rcheck/00install.out
echo ""
if grep -q -E "NOTE|WARNING|ERROR" lightgbm.Rcheck/00check.log; then
echo "NOTEs, WARNINGs, or ERRORs have been found by R CMD check"
exit 1
fi

This || exit 1 prevents those lines from ever being reached.

Setting it to || true will allow them to run, and then we'd be relying on the check on NOTEs / WARNINGs / ERRORs to appropriately raise a non-0 exit code and fail the build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, got it! I guess that NOTEs also cause exit code to be non-zero, that's why we cannot rely on it anymore like here

# fails tests if either ERRORs or WARNINGs are thrown by
# R CMD CHECK
check_succeeded="yes"
R CMD check ${PKG_TARBALL} \
--as-cran \
--run-donttest \
|| check_succeeded="no"
echo "R CMD check build logs:"
BUILD_LOG_FILE=lightgbm.Rcheck/00install.out
cat ${BUILD_LOG_FILE}
if [[ $check_succeeded == "no" ]]; then
exit 1
fi

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm really glad you asked about this, because I realized some of the assumptions being made in this existing code are not correct.

I tested today with R 4.3.3 and found that NOTEs and WARNINGs do not lead to a non-0 exit code for R CMD check. ERRORs do.

So I think we do still want something like this ||, to be sure that you can see all the build logs (content of 00install.out) in CI logs for a build where R CMD check found ERRORs.

To avoid duplicating that in multiple places, I just pushed commits moving that into a new script, .ci/run-r-cmd-check.sh. In that script, I made "allowed number of NOTEs" parameterizable, so we can continue not allowing any NOTEs in most CI jobs, but selectively allow 1 in these new jobs.

.github/workflows/r_package.yml Outdated Show resolved Hide resolved
@@ -230,18 +219,12 @@ if [[ $R_BUILD_TYPE == "cmake" ]]; then
cat $BUILD_LOG_FILE \
| grep --count "R version passed into FindLibR.cmake: ${R_VERSION}"
)
if [[ $used_correct_r_version -ne 1 ]]; then
if [[ $passed_correct_r_version_to_cmake -ne 1 ]]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a fix found in #6501. That PR was abandoned accidentally and I'd still like to give @Kunal-Singh-Dadhwal an opportunity to submit that work again.

But I want this particular fix, to get more confidence that the R CI jobs are working as expected.

context: #6501 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@jameslamb Thanks for the opportunity will make a commit really soon

@jameslamb jameslamb mentioned this pull request Jul 12, 2024
26 tasks
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.

Thanks for rethinking this PR!

LGTM except two minor comments below.

.github/workflows/r_package.yml Outdated Show resolved Hide resolved
.github/workflows/r_package.yml Outdated Show resolved Hide resolved
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.

🚀

@StrikerRUS StrikerRUS merged commit 5cec690 into master Jul 14, 2024
45 checks passed
@StrikerRUS StrikerRUS deleted the ci/gcc14-job branch July 14, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ci] [R-package] clang-18 jobs failing [ci] [R-package] update to new R-hub container images
3 participants