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] changes to support read/write performance in CMake builds #4

Closed
wants to merge 4 commits into from

Conversation

jameslamb
Copy link

Opening this PR to aid in the discussion on microsoft#3405, about whether bringing dependencies fmt and fast_double_parser into LightGBM will cause issues for getting {lightgbm} (LightGBM's R package) onto CRAN.

With the changes below, it's possible to build a {lightgbm} that passes almost all tests.

CMake-based installation

git submodule --init --recursive
Rscript build_r.R
R CMD check lightgbm_*.tar.gz --as-cran

CRAN installation with configure created by Autoconf

git submodule --init --recursive
sh build-cran-package.sh
R CMD check lightgbm_*.tar.gz --as-cran

R CMD check results

The CMake-based installation did not have any issues on my Mac.

The CRAN installation has this NOTE that would prevent the R package from getting to CRAN:

* checking compiled code ... NOTE
File ‘lightgbm/libs/lightgbm.so’:
  Found ‘___stderrp’, possibly from ‘stderr’ (C)
    Object: ‘external_libs/fmt/format.o’
  Found ‘___stdoutp’, possibly from ‘stdout’ (C)
    Object: ‘external_libs/fmt/format.o’

Compiled code should not call entry points which might terminate R nor
write to stdout/stderr instead of to the console, nor use Fortran I/O
nor system RNGs.

For a lot of background on this issue, see microsoft#2901.

Notes for reviewers

Let's talk about this on microsoft#3405 instead of here, to avoid splitting the conversation.

@AlbertoEAF
Copy link

AlbertoEAF commented Oct 6, 2020

Had to rebase+squash manually. Merged to my MR through a fast-forward. Don't know why it didn't detect the rebase&squash&merge.

@AlbertoEAF AlbertoEAF closed this Oct 6, 2020
@AlbertoEAF
Copy link

@jameslamb given that you're adding more changes to master in other PR's related to the same issue, now I'm not sure I was supposed to have merged this to my MR, can you confirm?

Thank you

@jameslamb
Copy link
Author

given that you're adding more changes to master in other PRs related to the same issue

@AlbertoEAF what does this refer to? Can you give me an example?

now I'm not sure I was supposed to have merged this to my MR, can you confirm?

It's fine that you merged it, we can move all of the discussion back to microsoft#3405 now.

@AlbertoEAF
Copy link

AlbertoEAF commented Oct 6, 2020

Great ;)

@AlbertoEAF what does this refer to? Can you give me an example?

I was talking of the other PR's you have mentioned in the main PR microsoft#3405:

@jameslamb
Copy link
Author

Oh I see. I wouldn't call those "related to the same issue" and they don't conflict with the ongoing work in microsoft#3405

@jameslamb jameslamb deleted the r/model-locale branch October 11, 2020 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants