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] [ci] remove unnecessary include in linear_tree_learner (fixes #6264) #6265

Merged
merged 16 commits into from
Jan 17, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jan 11, 2024

Fixes #6264.

Please see the linked issue for details. In short, CRAN has begun testing projects using clang-18 (which is not yet released) targeting libc++-18 instead of the system C++ stdlib. Those checks resulted in this compiler warning:

/usr/local/clang-trunk/bin/../include/c++/v1/__fwd/string_view.h:22:41: warning: 'char_traitsfmt::detail::char8_type' is deprecated: char_traits for T not equal to char, wchar_t, char8_t, char16_t or char32_t is non-standard and is provided for a temporary period. It will be removed in LLVM 19, so please migrate off of it. [-Wdeprecated-declarations]

If we don't submit a fix before January 24, {lightgbm} will be archived on CRAN

This PR proposes the following:

  • adds a CI job that reproduces the warning (build link)
  • fixes the underlying issue

Notes for Reviewers

It appears that the underlying issue was "linear_tree_learning.h includes <string> unnecessarily". I guess that leads clang to expand a template in fmt/core.h to many unnecessary types, some of which are considered deprecated in v18 of libc++.

I think?

I'm not really sure, but I do know that:

  • the CI job(s) added in this PR reproduce the failure from CRAN
  • removing that unnecessary include causes them to pass

@jameslamb jameslamb changed the title WIIP: [R-package] [ci] fix clang18 warning WIP: [R-package] [ci] fix clang18 warning Jan 11, 2024
@jameslamb
Copy link
Collaborator Author

🎉 I was able to reproduce the warning from CRAN in CI!

https://github.com/microsoft/LightGBM/actions/runs/7497815594/job/20411976864?pr=6265

Now we can push potential fixes here and use that to test.

@jameslamb
Copy link
Collaborator Author

jameslamb commented Jan 12, 2024

To start, I tried just upgrading to the latest fmt (v10.2.1) (following the steps in #6074).

Nevermind, I decided to revert this. The less we change in the next release, the lower the risk of CRAN finding an issue and archiving the package.

@jameslamb jameslamb changed the title WIP: [R-package] [ci] fix clang18 warning WIP: [R-package] [ci] fix clang18 warning (fixes #6264) Jan 12, 2024
@jameslamb jameslamb changed the title WIP: [R-package] [ci] fix clang18 warning (fixes #6264) [R-package] [ci] fix clang18 warning (fixes #6264) Jan 12, 2024
@jameslamb jameslamb marked this pull request as ready for review January 12, 2024 06:26
@jameslamb
Copy link
Collaborator Author

@guolinke @shiyu1994 could you please help with a review?

I'd like to merge this and then begin a 4.3.0 release as soon as possible, to minimize the risk of the package being archived on CRAN.

echo "install logs:"
echo ""
cat lightgbm.Rcheck/00install.out
echo ""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without this, compiler invocations and most warnings are not visible in CI logs because R CMD check redirects them to this file instead of printing them.

I think this will be generically useful for CI in the future, and in fact we already do that for most other R CI jobs:

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

@jameslamb jameslamb added fix and removed in progress labels Jan 12, 2024
@jameslamb jameslamb changed the title [R-package] [ci] fix clang18 warning (fixes #6264) [R-package] [ci] remove unnecessary include in linear_tree_learner (fixes #6264) Jan 12, 2024
@fabsig
Copy link
Contributor

fabsig commented Jan 16, 2024

FWIW: I am the main author of GPBoost which includes a fork of LightGBM. I got the same warning from CRAN and adapted it as suggested in this commit (simply dropping #include <string> from linear_tree_learner.h). Unfortunately, this does not fix the issue. I am still getting the following response from CRAN:

Thanks, we now see with clang 18:
  • checking whether package ‘gpboost’ can be installed ... [28m/70m] WARNING
    Found the following significant warnings:

/usr/local/clang-trunk/bin/../include/c++/v1/__fwd/string_view.h:22:41:
warning: 'char_traitsfmt::detail::char8_type' is deprecated:
char_traits for T not equal to char, wchar_t, char8_t, char16_t or
char32_t is non-standard and is provided for a temporary period. It will
be removed in LLVM 19, so please migrate off of it.
[-Wdeprecated-declarations]

from

/usr/local/clang-trunk/bin/clang++ -std=gnu++17
-I"/data/gannet/ripley/R/R-clang
-trunk/include" -DNDEBUG -I./include -DEIGEN_MPL2_ONLY -DMM_PREFETCH=1
-DMM_MALL
OC=1 -DUSE_SOCKET -DLGB_R_BUILD -isystem /usr/local/clang-trunk/include
-I/usr/
local/clang/include -fopenmp -pthread -fpic -O3 -Wall -pedantic
-frtti -Wp,-D
_FORTIFY_SOURCE=3 -c treelearner/tree_learner.cpp -o
treelearner/tree_learner.o
/usr/local/clang-trunk/bin/clang++ -std=gnu++17
-I"/data/gannet/ripley/R/R-clang
-trunk/include" -DNDEBUG -I./include -DEIGEN_MPL2_ONLY -DMM_PREFETCH=1
-DMM_MALL
OC=1 -DUSE_SOCKET -DLGB_R_BUILD -isystem /usr/local/clang-trunk/include
-I/usr/
local/clang/include -fopenmp -pthread -fpic -O3 -Wall -pedantic
-frtti -Wp,-D
_FORTIFY_SOURCE=3 -c treelearner/voting_parallel_tree_learner.cpp -o
treelearne
r/voting_parallel_tree_learner.o
In file included from treelearner/voting_parallel_tree_learner.cpp:5:
In file included from ./include/LightGBM/utils/common.h:12:
In file included from ./include/LightGBM/utils/log.h:14:
In file included from
/usr/local/clang-trunk/bin/../include/c++/v1/iostream:43:
In file included from /usr/local/clang-trunk/bin/../include/c++/v1/ios:223:
In file included from
/usr/local/clang-trunk/bin/../include/c++/v1/__locale:23:
In file included from
/usr/local/clang-trunk/bin/../include/c++/v1/string:625:
In file included from
/usr/local/clang-trunk/bin/../include/c++/v1/string_view:2
13:
/usr/local/clang-trunk/bin/../include/c++/v1/__fwd/string_view.h:22:41:
warning: 'char_traitsfmt::detail::char8_type' is deprecated:
char_traits for T not equal to char, wchar_t, char8_t, char16_t or
char32_t is non-standard and is provided for a temporary period. It will
be removed in LLVM 19, so please migrate off of it.
[-Wdeprecated-declarations]
22 | template <class _CharT, class _Traits = char_traits<_CharT> >
| ^
./include/LightGBM/fmt/core.h:391:50: note: in instantiation of template
type alias 'std_string_view' requested here
391 | S,
detail::std_string_view>::value)>
| ^
./include/LightGBM/fmt/format.h:542:63: note: in instantiation of
template class 'fmt::basic_string_viewfmt::detail::char8_type'
requested here
542 | inline size_t count_code_points(basic_string_view<char8_type> s) {
| ^

Please fix and resubmit.

@jameslamb
Copy link
Collaborator Author

Thanks for posting @fabsig !

I still believe we should try to submit a version of LightGBM with this PR on it to CRAN. I'm confident in the change because the new CI jobs added here raised the same compilation warning + R CMD check failure as CRAN did, and now don't.

Maybe there are other sources of this warning in GPBoost... different paths or order of includes, different uses of fmt, different version of fmt, etc.

@guolinke @shiyu1994 could you please help with a review today? I'd like to try to submit to CRAN tonight. We only have 7 more days to fix this before the package is archived there.

@jameslamb
Copy link
Collaborator Author

thanks @guolinke !

@jameslamb jameslamb merged commit 255c93b into master Jan 17, 2024
43 checks passed
@jameslamb jameslamb deleted the fix/clang-18-warning branch January 17, 2024 05:28
@fabsig
Copy link
Contributor

fabsig commented Jan 19, 2024

@jameslamb: I did indeed have a different version of fmt and now it also works for GPBoost. Thanks a lot for your comment! This saved my day 👍

@jameslamb
Copy link
Collaborator Author

Glad it helped @fabsig !

I submitted a version of {lightgbm} with this PR's fixes a few days ago, and it's passing CRAN's checks so far: https://cran.r-project.org/web/checks/check_results_lightgbm.html.

Including, most notably, that the clang18 check issue is no longer shown there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R-package] CRAN warnings about use of 'char_traits<fmt::detail::char8_type>'
3 participants