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

[jvm-packages] update rabit, surface new changes to spark, add parity and failure tests #4876

Open
wants to merge 46 commits into
base: master
from

Conversation

@chenqin
Copy link
Contributor

commented Sep 19, 2019

This pr doesn't change or cover checkpoint restore behavior.

It allow user set of rabit parameters to xgb4j-spark within configurations.
By default, those parameters were set consistent with values in rabit project.

@chenqin chenqin changed the title expose sets of rabit configurations to spark layer [jvm-packages] expose sets of rabit configurations to spark layer Sep 19, 2019
@chenqin chenqin force-pushed the chenqin:master branch from 206ad37 to 65e95a9 Sep 20, 2019
@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

this pr is blocked by bug fix in rabit
dmlc/rabit#106

@chenqin chenqin force-pushed the chenqin:master branch from cb23fd3 to 127eb79 Sep 23, 2019
@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

@trivialfis do you happen to know how to set default value of raibt option DMLC_ROOT in xgboost Cmake file?
I tried use following way and seems not working, only when I pass with cmake -DDMLC_ROOT=absolute path to dmlc-core .. seems working fine.

# rabit
set(DMLC_ROOT ${PROJECT_SOURCE_DIR}/dmlc-core)
add_subdirectory(${PROJECT_SOURCE_DIR}/rabit)
set_target_properties(rabit
  PROPERTIES
  CXX_STANDARD 11
  CXX_STANDARD_REQUIRED ON
  POSITION_INDEPENDENT_CODE ON)
list(APPEND LINKED_LIBRARIES_PRIVATE rabit)
@trivialfis

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

@chenqin I built XGBoost successfully on my local machine in #4880 .

chenqin added 3 commits Sep 23, 2019
@chenqin chenqin force-pushed the chenqin:master branch from 55be586 to 288e25b Sep 23, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Sep 23, 2019

Codecov Report

Merging #4876 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4876   +/-   ##
=======================================
  Coverage   77.63%   77.63%           
=======================================
  Files          11       11           
  Lines        2039     2039           
=======================================
  Hits         1583     1583           
  Misses        456      456

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 c741600...711eb92. Read the comment docs.

@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

done with cleanup, xgboost now build with rabit

@hcho3

This comment has been minimized.

Copy link
Collaborator

commented Sep 23, 2019

@chenqin Nicely done!

Copy link
Member

left a comment

if we update rabit version, we should highlight in the title and description

@@ -3,7 +3,7 @@
url = https://github.com/dmlc/dmlc-core
[submodule "rabit"]
path = rabit
url = https://github.com/dmlc/rabit
url = https://github.com/chenqin/rabit

This comment has been minimized.

Copy link
@CodingCat

CodingCat Sep 23, 2019

Member

can we revert this?

This comment has been minimized.

Copy link
@hcho3

hcho3 Sep 23, 2019

Collaborator

@CodingCat This PR depends on dmlc/rabit#106.

This comment has been minimized.

Copy link
@chenqin

chenqin Sep 24, 2019

Author Contributor

will swing back after dmlc/rabit#106 and chenqin/rabit@edac142 merged

This comment has been minimized.

Copy link
@chenqin

chenqin Oct 16, 2019

Author Contributor

and this change dmlc/rabit#120

@chenqin chenqin changed the title [jvm-packages] expose sets of rabit configurations to spark layer [jvm-packages] update rabit point to https://github.com/dmlc/rabit/pull/106, surface rabit related configurations to spark Sep 24, 2019
chenqin added 4 commits Sep 24, 2019
trivialfis added a commit to trivialfis/xgboost that referenced this pull request Sep 27, 2019
* Update rabit.
* Update dmlc-core.
* Split up model IO and serialization.
* Add version, CompiledWithCuda, string dump for json.
* Add a data generator.
* Apply cmake changes from dmlc#4876.
* Don't use vector.
* Use a wrapper around `dmlc::Parameter`.
* Replace non-model parameter with XGBoostParameter.
@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

per feedback from @CodingCat, we added tests which verify newly surfaced parameters user passed in get properly populated into Rabit.

we have tests cover those paramter passed in were parsed and taken into consideration tests https://github.com/dmlc/rabit/blob/master/test/cpp/allreduce_base_test.cpp

we have integration test which covers those parameter actually were taking effects in rabit https://github.com/dmlc/rabit/blob/master/test/test.mk#L15

cc @hcho3

trivialfis added a commit to trivialfis/xgboost that referenced this pull request Sep 28, 2019
* Update rabit.
* Update dmlc-core.
* Split up model IO and serialization.
* Add version, CompiledWithCuda, string dump for json.
* Add a data generator.
* Apply cmake changes from dmlc#4876.
* Use a wrapper around `dmlc::Parameter`.
* Replace non-model parameter with XGBoostParameter.
* Create a model status.
* Drop old parameter Save/Load.
chenqin added 3 commits Sep 30, 2019
trivialfis added a commit to trivialfis/xgboost that referenced this pull request Oct 7, 2019
* Update rabit.
* Update dmlc-core.
* Split up model IO and serialization.
* Add version, CompiledWithCuda, string dump for json.
* Add a data generator.
* Apply cmake changes from dmlc#4876.
* Use a wrapper around `dmlc::Parameter`.
* Replace non-model parameter with XGBoostParameter.
* Create a model status.
* Drop old parameter Save/Load.
@chenqin chenqin force-pushed the chenqin:master branch from 944aeae to 97120f1 Oct 11, 2019
trivialfis added a commit to trivialfis/xgboost that referenced this pull request Oct 12, 2019
* Update rabit.
* Update dmlc-core.
* Split up model IO and serialization.
* Add version, CompiledWithCuda, string dump for json.
* Add a data generator.
* Apply cmake changes from dmlc#4876.
* Use a wrapper around `dmlc::Parameter`.
* Replace non-model parameter with XGBoostParameter.
* Create a model status.
* Drop old parameter Save/Load.
chenqin added 3 commits Oct 13, 2019
@chenqin chenqin force-pushed the chenqin:master branch from eb91e94 to 046e0e4 Oct 14, 2019
@trivialfis

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

Do you have anything else needs to be merged in rabit first? Otherwise I would like to upgrade rabit first so I have more time for testing.

@chenqin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

Do you have anything else needs to be merged in rabit first? Otherwise I would like to upgrade rabit first so I have more time for testing.

Should be good, please upgrade rabit and start testing

@chenqin chenqin force-pushed the chenqin:master branch 3 times, most recently from 1f5763a to 88b0063 Oct 14, 2019
@chenqin chenqin force-pushed the chenqin:master branch from 88b0063 to cfb9ced Oct 14, 2019
chenqin added 5 commits Oct 15, 2019
@chenqin chenqin changed the title [jvm-packages] update rabit point to https://github.com/dmlc/rabit/pull/106, surface rabit related configurations to spark [jvm-packages] update rabit, surface new changes to spark, add parity and failure tests Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.