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

fix subset bug #2748

Merged
merged 6 commits into from
Feb 10, 2020
Merged

fix subset bug #2748

merged 6 commits into from
Feb 10, 2020

Conversation

guolinke
Copy link
Collaborator

@guolinke guolinke commented Feb 7, 2020

to fix #2744

@guolinke guolinke requested a review from chivee as a code owner February 7, 2020 04:26
@guolinke
Copy link
Collaborator Author

guolinke commented Feb 7, 2020

@jameslamb it seems the r tests randomly failed.

@StrikerRUS
Copy link
Collaborator

According to the logs, it cannot build dependencies:

* installing *source* package ‘data.table’ ...
** package ‘data.table’ successfully unpacked and MD5 sums checked
** using staged installation
zlib 1.2.11 is available ok
** libs
clang -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG   -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -I/usr/local/include -fopenmp -fPIC  -Wall -g -O2  -c assign.c -o assign.o
clang: error: unsupported option '-fopenmp'
make: *** [assign.o] Error 1
ERROR: compilation failed for package ‘data.table’
...
/Applications/Xcode-11.3.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../AMD.a(amd_l_dump.o) has no symbols
clang -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -DNTIMER  -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -I/usr/local/include  -fPIC  -Wall -g -O2  -c SuiteSparse_config.c -o SuiteSparse_config.o
ar -rucs ../SuiteSparse_config.a SuiteSparse_config.o
clang -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/Library/Frameworks/R.framework/Resources/lib -L/usr/local/lib -o Matrix.so CHMfactor.o Csparse.o TMatrix_as.o Tsparse.o init.o Mutils.o chm_common.o cs.o cs_utils.o dense.o dgCMatrix.o dgTMatrix.o dgeMatrix.o dpoMatrix.o dppMatrix.o dsCMatrix.o dsyMatrix.o dspMatrix.o dtCMatrix.o dtTMatrix.o dtrMatrix.o dtpMatrix.o factorizations.o ldense.o lgCMatrix.o sparseQR.o abIndex.o CHOLMOD.a COLAMD.a AMD.a SuiteSparse_config.a -L/Library/Frameworks/R.framework/Resources/lib -lRlapack -L/Library/Frameworks/R.framework/Resources/lib -lRblas -L/usr/local/gfortran/lib/gcc/x86_64-apple-darwin15/6.1.0 -L/usr/local/gfortran/lib -lgfortran -lquadmath -lm -F/Library/Frameworks/R.framework/.. -framework R -Wl,-framework -Wl,CoreFoundation
ld: warning: directory not found for option '-L/usr/local/gfortran/lib/gcc/x86_64-apple-darwin15/6.1.0'
ld: warning: directory not found for option '-L/usr/local/gfortran/lib'
ld: library not found for -lgfortran
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Matrix.so] Error 1
ERROR: compilation failed for package ‘Matrix’

I wonder why is it using clang while we set gcc

LightGBM/.travis.yml

Lines 45 to 48 in 87b6396

- if [[ $TRAVIS_OS_NAME == "osx" ]]; then
export OS_NAME="macos";
export COMPILER="gcc";
export R_MAC_VERSION=3.6.1;

@guolinke
Copy link
Collaborator Author

guolinke commented Feb 8, 2020

@StrikerRUS it seems some R packages are built by clang.

@jameslamb
Copy link
Collaborator

This looks like Rdatatable/data.table#2406 and the installation notes for data.table has some information on requiring OpenMP.

I noticed that only the Mac builds are failing on Azure and Travis.

I think one solution to make the test less flaky is to use the available precompiled binaries for data.table when on Mac, which is what Matt Dowle suggests in that data.table issue.

For your consideration, I just opened #2752 to do just that. If it build successfully, I think we should merge it and then rebase this PR.

@guolinke guolinke merged commit d8a34df into master Feb 10, 2020
Comment on lines +1394 to +1395
// FIXME: fix the multiple multi-val feature groups, they need to be merged
// into one multi-val group
Copy link
Collaborator

Choose a reason for hiding this comment

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

@guolinke Should a new separate issue be created for this to not lose it?

Copy link
Collaborator Author

@guolinke guolinke Feb 11, 2020

Choose a reason for hiding this comment

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

PR Already created.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, you mean #2754. Due to we have a lot of PRs modifying the same files, I overlooked it. Sorry about that!

@StrikerRUS StrikerRUS deleted the fix-subset-bug branch February 11, 2020 14:55
@guolinke guolinke added the fix label Mar 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subset function cause corruption using 2.3.2
3 participants