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

cover change with gtest unittests, clean up cmake script and code includes #106

Merged
merged 28 commits into from Sep 26, 2019

Conversation

@chenqin
Copy link
Contributor

commented Sep 17, 2019

This PR do two things

  • clear unused environment variables, accept those parameters as arguments
  • add gtest unittests, cleanup includes and cmake scripts to work with xgboost project

@trivialfis @hcho3

@chenqin chenqin changed the title cleanup unused environment variables rand to smooth traffic to tracker cleanup unused environment variables rand to smooth traffic send to tracker Sep 17, 2019
@trivialfis trivialfis self-requested a review Sep 18, 2019
src/allreduce_base.cc Outdated Show resolved Hide resolved
src/allreduce_base.cc Show resolved Hide resolved
Copy link
Member

left a comment

Please use deterministic waiting and point me to some unittests.

@hcho3

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

@chenqin Can we use exponential backoff to ease congestion? Unlike random waiting, it will have deterministic behavior.

@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@chenqin Can we use exponential backoff to ease congestion? Unlike random waiting, it will have

removed random with expoential back off plus rank %10 to avoid ddos tracker

@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Please use deterministic waiting and point me to some unittests.

working on googletest to support unittests

@trivialfis

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

chenqin added 4 commits Sep 19, 2019
@chenqin chenqin force-pushed the chenqin:master branch from ffe3389 to bd8227f Sep 19, 2019
@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@chenqin chenqin changed the title cleanup unused environment variables rand to smooth traffic send to tracker cover change with gtest unittests, smooth tracker reconnect Sep 19, 2019
@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@chenqin chenqin referenced this pull request Sep 19, 2019
6 of 10 tasks complete
Copy link
Member

left a comment

Will give a proper review tomorrow, one thing though, try find_package first then download gtest ...

@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

Will give a proper review tomorrow, one thing though, try find_package first then download gtest ...

I tried to use find_package along with exteranl project which doesn't ends well.
gtest download is shelled with RABIT_BUILD_TESTS

trivialfis and others added 7 commits Sep 21, 2019
Clean up CMake scripts. will work on a bit cleanup
@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

@trivialfis let's get this in to unblock xgb CMake house cleaning

@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

done with cleanup, xgboost now build with rabit

@hcho3 hcho3 requested a review from CodingCat Sep 23, 2019
Copy link
Member

left a comment

we should avoid mixing things together, especially "cleanup/refactoring" and "new feature adding"

and the other thing I don't understand is. the description says "clear unused environment variables, accept those parameters as arguments" why it contains so many CMake file changes

src/allreduce_base.cc Outdated Show resolved Hide resolved
@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

we should avoid mixing things together, especially "cleanup/refactoring" and "new feature adding"

and the other thing I don't understand is. the description says "clear unused environment variables, accept those parameters as arguments" why it contains so many CMake file changes

removed from this pr, let's do back off in separate pr

@chenqin chenqin changed the title cover change with gtest unittests, smooth tracker reconnect cover change with gtest unittests, clean up cmake script and code includes Sep 23, 2019
@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

we should avoid mixing things together, especially "cleanup/refactoring" and "new feature adding"

and the other thing I don't understand is. the description says "clear unused environment variables, accept those parameters as arguments" why it contains so many CMake file changes

fixed, remvoed code change touch sleep, this pr focus on make it buildable with xgboost project and adding googletest backed unit tests

@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@@ -0,0 +1,20 @@
# code copied from https://crascit.com/2015/07/25/cmake-gtest/

This comment has been minimized.

Copy link
@trivialfis

trivialfis Sep 24, 2019

Member

Doesn't seem necessary anymore. I'm OK for keeping it here for the convenience of others.

This comment has been minimized.

Copy link
@hcho3

hcho3 Sep 25, 2019

Contributor

@trivialfis I added this because this was the easiest way to install Google Test on Windows platform. Sorry, I forgot that this PR is in Rabit. I added something like this in dmlc-core, and it's quite convenient. I suggest we keep it.

test/cpp/allreduce_base_test.cpp Show resolved Hide resolved

option(DMLC_ROOT "Specify root of external dmlc core.")
# by default point to xgboost/dmlc-core
set(DMLC_ROOT ${CMAKE_CURRENT_LIST_DIR}/../dmlc-core)

This comment has been minimized.

Copy link
@trivialfis

trivialfis Sep 24, 2019

Member

You spent so much effort on rabit I assume you don't want to make it XGBoost exclusive right? Since last time I check rabit is only using some headers from dmlc-core, there's no linkage involved. So technically rabit can use dmlc-core in system path, say /usr/ or /home/chenqin/.local/ or any other directories that follow the bin/include/lib/lib64 pattern without any problem, and you can set above path as DMLC_ROOT for your convenience. With this, you can simply throw an error if DMLC_ROOT is not specified.

In the future (not for this PR), I may add this to rabit as dmlc-core has very good support for CMake target.

if (NOT DMLC_ROOT)
  find_package(dmlc)
  target_link_libraries(rabit dmlc::dmlc)  # This line also handles include directory.
endif (NOT DMLC_ROOT)
ENDIF(R_LIB OR MINGW OR WIN32)

if(RABIT_BUILD_MPI)
find_package(MPI REQUIRED)
if (NOT MPI_CXX_FOUND)
message(FATAL_ERROR "CXX Interface for MPI is required for building MPI backend.")

This comment has been minimized.

Copy link
@trivialfis

trivialfis Sep 24, 2019

Member

I wrote this line, but do we actually want to support MPI backend? I thought it's a proof of concept for the paper. And now that the MPI CXX interface is deprecated, there will be some more works for bring it up. @chenqin What do you think?

This comment has been minimized.

Copy link
@chenqin

chenqin Sep 24, 2019

Author Contributor

We might discuss this in another thread how to support MPI runtime. The rationale is there are lots of similar frameworks highly optimized such as facebook/gloo that just do MPI optimization. None of which offers failed single worker recover. But I don't have clear picture who is using rabit other than distributed XGBoost.

@trivialfis

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

Other than the line making rabit XGBoost exclusive, LGTM. Since this PR is a whole with dmlc/xgboost#4876 , will merge them together.

@hcho3
hcho3 approved these changes Sep 25, 2019
@trivialfis trivialfis merged commit ddcc2d8 into dmlc:master Sep 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@trivialfis

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

@chenqin Merged. You can focus on the jvm-package PR. ;-)

@trivialfis

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

And, a big thanks!

nateagr added a commit to criteo-forks/rabit that referenced this pull request Oct 9, 2019
* Clean up CMake scripts and related include paths.
* Add unittests.
nateagr added a commit to criteo-forks/rabit that referenced this pull request Oct 9, 2019
* Clean up CMake scripts and related include paths.
* Add unittests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.