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

Merge logging facilities for rabit and xgboost. #6101

Closed
wants to merge 4 commits into from

Conversation

trivialfis
Copy link
Member

  • Enable clang-tidy on CI.
  • Remove most of the debug logging.
  • Remove unused utilities.
  • Move standalone tests CMake config into top level for linking.

* Enable clang-tidy on CI.
* Remove most of the debug logging.
* Remove unused utilities.
* Move standalone tests CMake config into top level for linking.
@trivialfis trivialfis mentioned this pull request Sep 9, 2020
7 tasks
CMakeLists.txt Outdated
Comment on lines 342 to 361
if (GOOGLE_TEST AND (NOT WIN32))
# rabit mock based integration tests
list(REMOVE_ITEM rabit_libs "rabit_mock_static") # remove here to avoid installing it
set(tests lazy_recover local_recover model_recover)

foreach(test ${tests})
add_executable(${test} rabit/test/${test}.cc)
set_output_directory(${test} ${xgboost_BINARY_DIR})
target_link_libraries(${test} rabit_mock_static xgboost)
set_target_properties(${test} PROPERTIES CXX_STANDARD 14 CXX_STANDARD_REQUIRED ON)
add_test(NAME ${test} COMMAND ${test} WORKING_DIRECTORY ${xgboost_BINARY_DIR})
endforeach()

if(RABIT_BUILD_MPI)
add_executable(speed_test_mpi test/speed_test.cc)
target_link_libraries(speed_test_mpi rabit_mpi)
add_test(NAME speed_test_mpi COMMAND speed_test_mpi WORKING_DIRECTORY ${xgboost_BINARY_DIR})
endif(RABIT_BUILD_MPI)
endif (GOOGLE_TEST AND (NOT WIN32))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we keep this block in rabit/CMakeLists.txt ? What is the benefit of moving it to the top CMakeLists.txt ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@hcho3

Can we keep this block in rabit/CMakeLists.txt ? What is the benefit of moving it to the top CMakeLists.txt ?

Because

Move standalone tests CMake config into top level for linking.

Now the logging facility comes from XGBoost, linking to libxgboost.so is required for these standalone tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the sake of modularity, I prefer that this block would stay in rabit/CMakeList.txt. Do we lose access to xgboost target from rabit/CMakeLists.txt ?

This comment was marked as outdated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind, we actually want xgboost/logging.h, not dmlc/logging.h. So we do need libxgboost.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. The verbosity is defined in XGBoost so we need the logging facility from XGBoost instead of dmlc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we still move the block back to rabit/CMakeLists.txt? Does it interfere with linking with xgboost?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me think about it. As now rabit is part of XGBoost, so they will have intertwined linkage. I'm actually considering removing this static library.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, let's think about it a bit more.

Some choices I can think of:

Option 1. Remove the rabit CMake target entirely and merge all the source and header files to objxgboost.
Option 2. Separate the logging portion of XGBoost and create a new CMake object library target objxgboost_logging. Then link this library to both Rabit and objxgboost.

@trivialfis Would you like me to review this pull request as it is, for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you like me to review this pull request as it is, for now?

No, I converted it to draft. It's already having linkage issue on Jenkins and I don't know how to resolve it.

@trivialfis trivialfis marked this pull request as draft September 9, 2020 07:07
@trivialfis trivialfis closed this Sep 18, 2020
@trivialfis trivialfis deleted the fix-tidy branch September 18, 2020 05:31
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