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

Refactor CMake scripts. #4323

Merged
merged 8 commits into from
Apr 15, 2019
Merged

Refactor CMake scripts. #4323

merged 8 commits into from
Apr 15, 2019

Conversation

trivialfis
Copy link
Member

@trivialfis trivialfis commented Apr 2, 2019

  • Demo for using CMake with XGBoost.
  • Document for using CMake with XGBoost.
  • Use CMake CUDA language.
  • Use CMake to handle Doxygen.
  • Split up CMakeList.
  • Export install target.
  • Remove build.sh
  • Workaround for gpu_hist test.
  • Remove build_config.h.in
  • Bump CMake for GPU to 3.12
  • Move compilation database related code to tidy.py

TODOs:

  • Bring back clang-tidy.
  • Enable CLI test in Jenkins.
  • Get rabit build on Windows.
  • Clean up.
  • Fix R installation hack.
  • Correct tidy flags.

@trivialfis
Copy link
Member Author

trivialfis commented Apr 2, 2019

@RAMitchell Thanks for the suggestion, I added your workaround for dmlc registrations. But the added inline keyword for GPU kernel generates a warning.

@jschueller
Copy link

jschueller commented Apr 2, 2019

Hi, it could install the python package in a dir relative to CMAKE_INSTALL_PREFIX (lib/pythonx.y/site-packages on unix and Lib/site-packages on win32).

@trivialfis
Copy link
Member Author

@jschueller That would be part of Python setuptools. I don't think CMake is the right tool to install Python package.

@jschueller
Copy link

Correct me if I'm wrong but it seems the python bindings is just made of a bunch of .py files, I guess cmake could copy them to the site dir.

@trivialfis
Copy link
Member Author

@jschueller setuptools does a lot more than copying scripts. It helps compile Python scripts into bytecodes, creates a Packages and handles its dependencies.

@trivialfis
Copy link
Member Author

trivialfis commented Apr 4, 2019

@hcho3 Related to #4331. I bumped CMake to 3.8 but another attempt by @RAMitchell uses 3.9, see #4316. I'm not sure this PR will work out smoothly as you can see from the tests, my build failed in various ways depending on different platforms, while built perfectly on my machine. I believe there are lots of different bugs from different platforms. So please don't put too much weight on this PR for now, I'm not sure I can workaround all of them. It's a miracle that our machine works at all ...

@hcho3
Copy link
Collaborator

hcho3 commented Apr 4, 2019

@trivialfis Okay, in that case, should we keep the current CMake version (3.6.0) for now?

@trivialfis
Copy link
Member Author

@hcho3 Yep, thanks. I will upgrade it when it's done.

@trivialfis
Copy link
Member Author

trivialfis commented Apr 5, 2019

List of known issues in CMake CUDA lang:

  • Chosen compiler by exporting CC and CXX doesn't get propagated to CUDA.
  • CXX_STANDARD does not get propagated to CUDA.
  • Flags like -Xcompiler xxx or -gencode arch,code ... that contain space does not do well with target_compile_options.
  • One cannot configure host compiler via CMAKE_CUDA_HOST_COMPILER before [email protected].
  • POSITION_INDEPENDENT_CODE does not get propagated to CUDA in [email protected]
  • Failed to link cuda runtime before [email protected]. Probably due to our use of object library.

List of other known issues:

  • Rabit redefined ssize_t.

Checked item means I can workaround it.

@trivialfis
Copy link
Member Author

trivialfis commented Apr 5, 2019

As mentioned by @RAMitchell and @hcho3, I should write a short introduction about rationale about this rewrite. Here it's. ;-)

Goal

tl;dr: Enable C APIs with easy to use build system interface:

find_package(xgboost REQUIRED)
target_link_libraries(mytarget xgboost::xgboost)

The above two lines should get include path and linking done.

Why does it take a rewrite to accomplish this

There's a trend about Modern CMake, where one specify build options target by target. The extra effort is not only to make the code look nicer, but a fundamental requirement for making a usable shared library. Since we are building C APIs, user of the interface should not know anything about what libraries xgboost needs, what extra flags are required to build a project with xgboost, nor the CXX standard we use. For instance, users of xgboost should not need to pass OpenMP flags if they don't use it in their own code. That means we need to seal all these stuffs behind the build system.

With CMake, all requirements of using a library is abstracted in a installed config file like xgboost-config.cmake, which is used by find_package command. And the target based style (aka modern cmake) CMake is to make sure we only export absolute necessary build specification to our interface. In xgboost, that will be one include directory xgboost/include and one linkable object libxgboost.so. Stuffs like libdmlc.a, dmlc/include or nvtx.h should not be part of our interface. Therefore, commands like include_directories, link_libraries and global variables that don't distinguish between interface and internal should not be used any more.

What problems this won't solve

Symbols visibility. For a shared library, providing restricted but stable ABI is important for easy distribution. In some cases one would pass flags like gcc -fvisibility=hidden to make all symbols hidden from interface except the ones marked by __attribute__(visibility("default")). There are other complications in this like whether dmlc::Error should be visible to other users. This PR doesn't address this problem, and not likely in the future since we have to go through dmlc-core and rabit to make a clear sense about the implications.

Extras

  • Now we run doxygen via CMake.
  • Enable a test for cli.

@trivialfis
Copy link
Member Author

@RAMitchell Could you take a look at the above known issues? I felt like there are too many things to work around ...

@hcho3
Copy link
Collaborator

hcho3 commented Apr 7, 2019

@trivialfis Can we just use a recent version of CMake to avoid known issues? Personally, I'd like to see parallel compilation enabled for Windows.

@trivialfis
Copy link
Member Author

@hcho3 Currently I build it with CMake 3.12, which ships with Ubuntu 18.10. Do you think it's a little too aggressive? @RAMitchell

@trivialfis
Copy link
Member Author

Currently it's the last item

Failed to link cuda runtime before [email protected]. Probably due to our use of object library.

I can't work around. I don't know how to link CUDA runtime manually. Another way to do it might be using static library with PIC for libxgboost at first then convert it into shared library later .. But it's getting more and more weird.

@hcho3
Copy link
Collaborator

hcho3 commented Apr 8, 2019

@trivialfis I'm okay with using CMake 3.12 for CUDA code.

@RAMitchell
Copy link
Member

@trivialfis I am ok with requiring cmake 3.12. I don't have any experience with your particular issue linking the run-time, but in the past I have built the project by combining GPU code into a static library and linking this into the xgboost shared library. This was a workaround for linking issues due to dmlc register, at the time I could not make it work using object libraries.

demo/regression/machine.conf Outdated Show resolved Hide resolved
* Remove CMake CUDA wrapper.
* Bump CMake version for CUDA.
* Use CMake to handle Doxygen.
* Split up CMakeList.
* Export install target.
* Use modern CMake.
* Remove build.sh
* Workaround for gpu_hist test.
* Use cmake 3.12.
@trivialfis
Copy link
Member Author

@hcho3 @RAMitchell Ready for another review. The build system is quite huge by now, CMake has its limitation. If you find anything that is not clear or over complicated in the code, please let me know. I don't want anyone get stumbled by the build system.

@codecov-io
Copy link

codecov-io commented Apr 12, 2019

Codecov Report

Merging #4323 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4323      +/-   ##
==========================================
+ Coverage   67.92%   67.93%   +<.01%     
==========================================
  Files         132      132              
  Lines       12240    12240              
==========================================
+ Hits         8314     8315       +1     
+ Misses       3926     3925       -1
Impacted Files Coverage Δ
src/c_api/c_api.cc 23.53% <ø> (ø) ⬆️
src/objective/multiclass_obj.cu 93.68% <0%> (+1.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edae664...d1c2e58. Read the comment docs.

@trivialfis
Copy link
Member Author

Added a short intro in demo/c-api/readme.md for using CMake with xgboost.

@trivialfis
Copy link
Member Author

trivialfis commented Apr 14, 2019

@RAMitchell Would you be able to try it on Windows with CUDA when time allows?

Copy link
Member

@RAMitchell RAMitchell left a comment

Choose a reason for hiding this comment

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

Looks good to me, although I cannot speak for the JVM or R packages.

cmake/Doc.cmake Show resolved Hide resolved
demo/c-api/README.md Show resolved Hide resolved
@hcho3 hcho3 requested a review from CodingCat April 15, 2019 05:25
CMakeLists.txt Show resolved Hide resolved
cmake/Utils.cmake Show resolved Hide resolved
Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

Approving for now, but we may want to add GPU test for R in the future.

@trivialfis
Copy link
Member Author

Ping @CodingCat

@hcho3 hcho3 merged commit 207f058 into dmlc:master Apr 15, 2019
@trivialfis
Copy link
Member Author

Thanks for all the help!

@trivialfis trivialfis deleted the cmake-rebase branch April 15, 2019 17:40
@hcho3 hcho3 mentioned this pull request Apr 21, 2019
18 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Jul 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants