Fix build on macOS Mojave #1923

henry0312 · 2018-12-22T05:22:25Z

Fixed microsoft#1898 - https://iscinumpy.gitlab.io/post/omp-on-high-sierra/ - https://cliutils.gitlab.io/modern-cmake/chapters/packages/OpenMP.html - Homebrew/homebrew-core#20589

henry0312 · 2018-12-22T05:27:41Z

CMakeLists.txt

@@ -211,8 +211,8 @@ endif(USE_MPI)

 if(USE_OPENMP)
    if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
-        TARGET_LINK_LIBRARIES(lightgbm ${OpenMP_libomp_LIBRARY})


https://gist.github.com/henry0312/c2e0550caf7acfce2e60a24984c322e9#file-make1-log

henry0312 · 2018-12-22T05:27:56Z

CMakeLists.txt

@@ -211,8 +211,8 @@ endif(USE_MPI)

 if(USE_OPENMP)
    if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
-        TARGET_LINK_LIBRARIES(lightgbm ${OpenMP_libomp_LIBRARY})
-        TARGET_LINK_LIBRARIES(_lightgbm ${OpenMP_libomp_LIBRARY})
+        TARGET_LINK_LIBRARIES(lightgbm OpenMP::OpenMP_CXX)


https://gist.github.com/henry0312/c2e0550caf7acfce2e60a24984c322e9#file-make2-log

henry0312 · 2018-12-22T05:32:19Z

docs/Installation-Guide.rst

+     cmake \
+       -DOpenMP_C_FLAGS="-Xpreprocessor -fopenmp -I/usr/local/opt/libomp/include" \
+       -DOpenMP_C_LIB_NAMES="omp" \
+       -DOpenMP_CXX_FLAGS="-Xpreprocessor -fopenmp -I/usr/local/opt/libomp/include" \


https://github.com/Homebrew/homebrew-core/blob/master/Formula/libomp.rb#L36

henry0312 · 2018-12-22T05:57:37Z

This is much better than #1919.

StrikerRUS · 2018-12-22T18:32:13Z

@henry0312

This is much better than #1919.

Totally agree! :-)

Maybe it should be mentioned that this fearful command is needed only for Mojave? Other users can still use simple cmake .., right?

henry0312 · 2018-12-23T03:43:43Z

Maybe it should be mentioned that this fearful command is needed only for Mojave?

Yes, the cmake options is needed only for Mojave, although they works fine for High Sierra too.

Other users can still use simple cmake .., right?

yes.

StrikerRUS · 2018-12-23T18:05:31Z

Yes, the cmake options is needed only for Mojave, although they works fine for High Sierra too.

OK, I see. Please document it.

Also, I think that we need to add second attempt for compilation with new CMake options for Apple here:
https://github.com/Microsoft/LightGBM/blob/2323cb3befa0515dd266d4a1393a37c833c11b83/python-package/setup.py#L164-L168

Something like:

if Apple and status != 0:
    second attempt

python-package/setup.py

henry0312 · 2018-12-24T07:58:53Z

Okay, I updated.

StrikerRUS

Okay, I updated.

Thanks! I left some comments.

docs/Installation-Guide.rst

StrikerRUS · 2018-12-24T10:34:57Z

python-package/setup.py

+        if not (cc and cc.startswith('gcc') and cxx and cxx.startswith('g++')):
+            # Apple Clang
+            # https://github.com/Homebrew/homebrew-core/pull/20589
+            cmake_cmd.extend([


We shouldn't use hardcoded paths for the first build attempt. Let's use hardcoded paths only after first attempt fail:

... else: # Linux, Darwin (macOS), etc. logger.info("Starting to compile with CMake.") if system() == 'Darwin' and not nomp and not (cc and cc.startswith('gcc') and cxx and cxx.startswith('g++')): status = silent_call(cmake_cmd) # first build attempt with CMake OpenMP autodetection (current behavior) if status != 0: logger.warning("Compilation failed.") logger.info("Starting to compile with OpenMP path guesses.") clear_path(os.path.join(CURRENT_DIR, "build_cpp")) <your modification here> # second attempt with hardcoded OpenMP paths for Homebrew else: silent_call(cmake_cmd, raise_error=True, error_msg='Please install CMake and all required dependencies first') ...

We shouldn't use hardcoded paths for the first build attempt.

Why is this?
I think this trial is a really waste.

This workaround was tested only on High Sierra and Mojave. We don't know how it works on earlier versions. However, the current method was tested by the time and there were no any complaints.

There could be some other problems with this workaround which we don't know.

User can install OpenMP in another folder.

CMake team will repair OpenMP detection sometime and the current method will work fine again without any new hotfixes.

To be honest, it is correct for macOS users to set cmake options for AppleClang with OpenMP.
In ohter words, the current way (cmake ..) is wrong (than this change) because libomp requires the options.

@henry0312 @StrikerRUS What about something like this instead in README.md or whatever else for comments? (and let the user change the paths?)

DOpenMP_C_LIB_NAMES="omp" DOpenMP_CXX_FLAGS="-Xpreprocessor -fopenmp -I/usr/local/opt/libomp/include" DOpenMP_CXX_LIB_NAMES="omp" DOpenMP_omp_LIBRARY=/usr/local/opt/libomp/lib/libomp.dylib bash -c 'cmake ..'

@Laurae2 I think that is one of options that we take, but that won't allow us to run just python setup.py install.
It is useful for many users.
And the fixed documetns are in https://github.com/Microsoft/LightGBM/pull/1923/files#diff-1f74fbb61fb97279854ec1f63430b9bb (to be more fixed for some comments).

@henry0312

In ohter words, the current way (cmake ..) is wrong (than this change) because libomp requires the options.

Maybe... But if everything works (OK, worked before 10.14), why should we ask users do more things then needed?

@henry0312 @Laurae2 Hmm, what do you think about new setup args openmp-include-dir and openmp-library like we already have for GPU version?

Then we can do the following:

1. Try to compile with default "cmake .." for earlier macOS versions and future CMake changes. 2. If new args are provided, compile with them. 3. If not 2. (not provided or failed), then try to compile with path guesses for the maximum backward compatibility and user's happiness.

why should we ask users do more things then needed?

No, no.
This change will never ask users to do more anything.
For users, nothing will be changed.
For us, this change will correct building options. That's all.

henry0312 · 2018-12-24T15:51:52Z

I updated docs and fixed setup.py again.

docs/Installation-Guide.rst

StrikerRUS

I think that it's more than enough for a hotfix! Thanks a lot @henry0312 !

Speaking about Python args, I'll create a separate PR soon.

chivee

LGTM

henry0312 · 2018-12-26T09:26:13Z

I've waited for #1898 (comment).
We can merge this PR after the check.

StrikerRUS · 2019-01-25T22:37:46Z

@henry0312 The feedback has been received. Can we merge now?

#2) * [ci] removed temp brew hotfix and deprecated sudo option (microsoft#1951) * removed brew hotfix and deprecated sudo option on Travis * removed brew hotfix on Azure * updated Boost docs (microsoft#1955) * removed warnings about types in comparison ([-Wsign-compare]) (microsoft#1953) * removed comparison warning * fixed spacing * [docs] ask to provide LightGBM version for issue (microsoft#1958) * [R] Fix multiclass demo (microsoft#1940) * Fix multiclass custom objective demo * Use option not to boost from average instead of setting init score explicitly * Reference microsoft#1846 when turning off boost_from_average * Add trailing whitespace * [R] Correcting lgb.prepare output comment (microsoft#1831) * Correcting lgb.prepare output comment * updated Roxygen files * [docs] bump xcode version in docs (microsoft#1952) * fix typo * [docs] Added the links to the libraries used (microsoft#1962) * Added links to the libraries used. * Fixing the header * Fixes * ot -> to * [docs] fixed minor typos in documentation (microsoft#1959) * fixed minor typos in documentation * fixed typo in gpu_tree_learner.cpp * Update .gitignore * support to override some parameters in Dataset (microsoft#1876) * add warnings for override parameters of Dataset * fix pep8 * add feature_penalty * refactor * add R's code * Update basic.py * Update basic.py * fix parameter bug * Update lgb.Dataset.R * fix a bug * Fix build on macOS Mojave (microsoft#1923) * Fix build on macOS Mojave Fixed microsoft#1898 - https://iscinumpy.gitlab.io/post/omp-on-high-sierra/ - https://cliutils.gitlab.io/modern-cmake/chapters/packages/OpenMP.html - Homebrew/homebrew-core#20589 * update setup.py * update docs * fix setup.py * update docs * update docs * update setup.py * update docs * [tests][python] added tests for metrics' behavior and fixed case for multiclass task with custom objective (microsoft#1954) * added metrics test for standard interface * simplified code * less trees * less trees * use dummy custom objective and metric * added tests for multiclass metrics aliases * fixed bug in case of custom obj and num_class > 1 * added metric test for sklearn wrapper * [python][R][docs] added possibility to install with Visual Studio 2019 Preview (microsoft#1956) * Found error from microsoft#1939 (microsoft#1974) * fix more edge cases in mape (microsoft#1977) * fix R's overflow (microsoft#1960) * [tests][python] added test for huge string model (microsoft#1964) * added test for huge string model * fixed tree sizes field * simplified model structure * fixed test and added try/except * fix nan in eval results (microsoft#1973) * always save the score of the first round in early stopping fix microsoft#1971 * avoid using std::log on non-positive numbers * remove unnecessary changes * add tests * Update test_sklearn.py * enhanced tests * fix microsoft#1981 * [python] added OpenMP options for python-package installation (microsoft#1975) * added OpenMP options for python-package installation * fixed grammar typo * improved model loading routines (microsoft#1979) * [ci] refined command status check (microsoft#1980) * refined command status check * refined Appveyor * redirect all warnings to stdout * cpplint whitespaces and new lines (microsoft#1986) * fix microsoft#1994 * [docs] Fixed OpenCL Debian package name typo (microsoft#1995) [docs] Fixed OpenCL Debian package name typo * [python] convert datatable to numpy directly (microsoft#1970) * convert datatable to numpy directly * fix according to comments * updated more docstrings * simplified isinstance check * Update compat.py * [R-package] Fix demos not using lgb.Dataset.create.valid (microsoft#1993) * Hand edit broken commit * Hand edit broken commit * Hand edit broken commit * Hand edit broken commit * 2.2.3 release (microsoft#1987) * Update DESCRIPTION * Update DESCRIPTION * update version number at master branch (microsoft#1996) * Update VERSION.txt * Update .appveyor.yml * Update DESCRIPTION * Initial attempt to implement appending features in-memory to another data set The intent is for this to enable munging files together easily, without needing to round-trip via numpy or write multiple copies to disk. In turn, that enables working more efficiently with data sets that were written separately. * Implement Dataset.dump_text, and fix small bug in appending of group bin boundaries. Dumping to text enables us to compare results, without having to worry about issues like features being reordered. * Add basic tests for validation logic for add_features_from. * Remove various internal mapping items from dataset text dumps These are too sensitive to the exact feature order chosen, which is not visible to the user. Including them in tests appears unnecessary, as the data dumping code should provide enough coverage. * Add test that add_features_from results in identical data sets according to dump_text. * Add test that booster behaviour after using add_features_from matches that of training on the full data This checks: - That training after add_features_from works at all - That add_features_from does not cause training to misbehave * Expose feature_penalty and monotone_types/constraints via get_field These getters allow us to check that add_features_from does the right thing with these vectors. * Add tests that add_features correctly handles feature_penalty and monotone_constraints. * Ensure add_features_from properly frees the added dataset and add unit test for this Since add_features_from moves the feature group pointers from the added dataset to the dataset being added to, the added dataset is invalid after the call. We must ensure we do not try and access this handle. * Remove some obsolete TODOs * Tidy up DumpTextFile by using a single iterator for each feature This iterators were also passed around as raw pointers without being freed, which is now fixed. * Factor out offsetting logic in AddFeaturesFrom * Remove obsolete TODO * Remove another TODO This one is debatable, test code can be a bit messy and duplicate-heavy, factoring it out tends to end badly. Leaving this for now, will revisit if adding more tests later on becomes a mess. * Add documentation for newly-added methods. * Initial work towards add_data_from This currently only merges the feature groups and updates num_data_. It does not deal with Metadata or non-dense bins yet. * Fix bug where dense bin copy of num_data_ wasn't updated * Small bug fix in dense_bin.hpp, initial implementation of Merge for 4-bits bin. * Add unit test for dense bin case of add_data_from, and refactor tests slightly. * Initial implementation of Merge for sparse bins and unit tests for it. * Ensure we test merging sparse data sets after loading them from binary This seems silly, but push_buffers_ aren't populated if the data was loaded from a binary file. This forces us to reconstruct the index,value form of the data in the target bin before merging. Adding this test ensures that code is covered. * Add labels to text dumps. * Add weights to text dumps. * Ensure add_data_from properly merges labels. * Ensure metadata appends weights correctly, and unit test for it. * Implement metadata merging for query bits This is currently not covered by unit tests. * Check datasets are aligned before merging. This catches the majority of obvious errors, e.g. not having the same number of features or having different bin mappings. * Add test that booster behaviour is preserved by add_data_from. * Add configuration parameters for CEGB. * Add skeleton CEGB tree learner Like the original CEGB version, this inherits from SerialTreeLearner. Currently, it changes nothing from the original. * Track features used in CEGB tree learner. * Pull CEGB tradeoff and coupled feature penalty from config. * Implement finding best splits for CEGB This is heavily based on the serial version, but just adds using the coupled penalties. * Set proper defaults for cegb parameters. * Ensure sanity checks don't switch off CEGB. * Implement per-data-point feature penalties in CEGB. * Implement split penalty and remove unused parameters. * Merge changes from CEGB tree learner into serial tree learner * Represent features_used_in_data by a bitset, to reduce the memory overhead of CEGB, and add sanity checks for the lengths of the penalty vectors.

Fix build on macOS Mojave

2ad6f07

Fixed microsoft#1898 - https://iscinumpy.gitlab.io/post/omp-on-high-sierra/ - https://cliutils.gitlab.io/modern-cmake/chapters/packages/OpenMP.html - Homebrew/homebrew-core#20589

henry0312 force-pushed the fix_build_on_mojave branch from 857fca4 to 2ad6f07 Compare December 22, 2018 05:24

henry0312 commented Dec 22, 2018

View reviewed changes

henry0312 requested review from guolinke and StrikerRUS December 22, 2018 06:01

henry0312 added 3 commits December 24, 2018 15:03

update setup.py

c121905

update docs

ac3f5f1

fix setup.py

2180fc4

henry0312 commented Dec 24, 2018

View reviewed changes

python-package/setup.py Outdated Show resolved Hide resolved

StrikerRUS requested changes Dec 24, 2018

View reviewed changes

henry0312 added 3 commits December 25, 2018 00:01

update docs

c723f3f

update docs

2131023

update setup.py

d2a3c20

StrikerRUS reviewed Dec 24, 2018

View reviewed changes

docs/Installation-Guide.rst Show resolved Hide resolved

update docs

2e96aa9

StrikerRUS approved these changes Dec 25, 2018

View reviewed changes

StrikerRUS requested review from chivee, Laurae2 and guolinke and removed request for guolinke December 25, 2018 11:41

chivee approved these changes Dec 26, 2018

View reviewed changes

StrikerRUS mentioned this pull request Dec 30, 2018

Install issue OS X / Not loading while installed ?! #1933

Closed

henry0312 merged commit b968499 into microsoft:master Jan 26, 2019

henry0312 deleted the fix_build_on_mojave branch January 26, 2019 04:41

StrikerRUS mentioned this pull request Jan 27, 2019

[python] added OpenMP options for python-package installation #1975

Merged

jameslamb mentioned this pull request Feb 26, 2019

Installation error on R by Could NOT find OpenMP_C #2026

Closed

StrikerRUS mentioned this pull request Jun 25, 2019

Update OS X compability #2241

Closed

StrikerRUS mentioned this pull request Jul 6, 2019

Better XGBoost installation on Mac OSX? dmlc/xgboost#4477

Closed

lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build on macOS Mojave #1923

Fix build on macOS Mojave #1923

henry0312 commented Dec 22, 2018 •

edited

Loading

henry0312 Dec 22, 2018

henry0312 Dec 22, 2018

henry0312 Dec 22, 2018

henry0312 commented Dec 22, 2018

StrikerRUS commented Dec 22, 2018

henry0312 commented Dec 23, 2018

StrikerRUS commented Dec 23, 2018

henry0312 commented Dec 24, 2018 •

edited

Loading

StrikerRUS left a comment •

edited

Loading

StrikerRUS Dec 24, 2018 •

edited

Loading

henry0312 Dec 24, 2018

StrikerRUS Dec 24, 2018 •

edited

Loading

henry0312 Dec 24, 2018

Laurae2 Dec 24, 2018 •

edited

Loading

henry0312 Dec 24, 2018

StrikerRUS Dec 24, 2018 •

edited

Loading

henry0312 Dec 24, 2018

henry0312 commented Dec 24, 2018

StrikerRUS left a comment

chivee left a comment

henry0312 commented Dec 26, 2018

StrikerRUS commented Jan 25, 2019

Fix build on macOS Mojave #1923

Fix build on macOS Mojave #1923

Conversation

henry0312 commented Dec 22, 2018 • edited Loading

henry0312 Dec 22, 2018

Choose a reason for hiding this comment

henry0312 Dec 22, 2018

Choose a reason for hiding this comment

henry0312 Dec 22, 2018

Choose a reason for hiding this comment

henry0312 commented Dec 22, 2018

StrikerRUS commented Dec 22, 2018

henry0312 commented Dec 23, 2018

StrikerRUS commented Dec 23, 2018

henry0312 commented Dec 24, 2018 • edited Loading

StrikerRUS left a comment • edited Loading

Choose a reason for hiding this comment

StrikerRUS Dec 24, 2018 • edited Loading

Choose a reason for hiding this comment

henry0312 Dec 24, 2018

Choose a reason for hiding this comment

StrikerRUS Dec 24, 2018 • edited Loading

Choose a reason for hiding this comment

henry0312 Dec 24, 2018

Choose a reason for hiding this comment

Laurae2 Dec 24, 2018 • edited Loading

Choose a reason for hiding this comment

henry0312 Dec 24, 2018

Choose a reason for hiding this comment

StrikerRUS Dec 24, 2018 • edited Loading

Choose a reason for hiding this comment

henry0312 Dec 24, 2018

Choose a reason for hiding this comment

henry0312 commented Dec 24, 2018

StrikerRUS left a comment

Choose a reason for hiding this comment

chivee left a comment

Choose a reason for hiding this comment

henry0312 commented Dec 26, 2018

StrikerRUS commented Jan 25, 2019

henry0312 commented Dec 22, 2018 •

edited

Loading

henry0312 commented Dec 24, 2018 •

edited

Loading

StrikerRUS left a comment •

edited

Loading

StrikerRUS Dec 24, 2018 •

edited

Loading

StrikerRUS Dec 24, 2018 •

edited

Loading

Laurae2 Dec 24, 2018 •

edited

Loading

StrikerRUS Dec 24, 2018 •

edited

Loading