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

[Rabit] One dmlc-core. #4360

Closed
trivialfis opened this issue Apr 12, 2019 · 9 comments
Closed

[Rabit] One dmlc-core. #4360

trivialfis opened this issue Apr 12, 2019 · 9 comments

Comments

@trivialfis
Copy link
Member

trivialfis commented Apr 12, 2019

@chenqin @RAMitchell @hcho3

It was suggested that we can duplicate dmlc-core in #4352 but I disagree with the choice. The problem is in c++'s hairy linkage specification. If we have out-of-sync dmlc-core in two places, the behaviour is undefined. Specifically with two functions having the same signature linker will choose one of them and discard the other. I made a demonstration program for others to verify. Will upload soon.

@trivialfis
Copy link
Member Author

@trivialfis
Copy link
Member Author

We need to come up with a solution for the upgraded rabit.

@CodingCat
Copy link
Member

CodingCat commented Apr 12, 2019

Ok...then we should think about how to resolve the issue @RAMitchell raised

Two potential ways as initial thoughts:

  1. Take the pain, update dmlc, upgrade submodule in Rabit, upgrade submodule(rabit) in XGBoost

  2. let’s forget about rabit, keep rabit as an independent project for whoever else wants to use, but swallow everything from rabit to xgboost as an communication module

@CodingCat CodingCat reopened this Apr 12, 2019
@trivialfis
Copy link
Member Author

trivialfis commented Apr 12, 2019

@CodingCat

For 1. Indeed painful.
For 2. If we fix a bug in our cloned rabit, we still want to port the fix to original rabit right? That's just copying files in disguise...

CMake has a feature called External project, that will fit normal development. But it also makes packaging for R CRAN (which uses amalgamation and requires source distribution) and Python PIP difficult.

@chenqin
Copy link
Contributor

chenqin commented Apr 12, 2019

Rabit is a bit of everything but mostly flexible interface (sync/checkpoint/restore) Take an example, I can totally see running GPU direct implementation behind Rabit as possibility.

I think easiest thing we can do is to split dmlc-core headers used in rabit and rename (to avoid confusion in xgboost we had seen before) this will cut dependency between dmlc-core with rabit at cost of rabit needs to sync with dmlc-core from time to time to avoid xgboost rabit upgrade issue. It leaves rabit a bit more work and makes entire flow smooth.

The downside is clearly if dmlc-core headers changed like io.h rabit can only find out when it upgrade.(I think i can do a dummy script and auto sync with dmlc-core master with a bit naming hacking.

@CodingCat
Copy link
Member

@trivialfis I am actually proposing accept more and more diversity of cloned rabit and the original rabit, if people wants to fix there, fine..and we can do whatever beneficial to xgboost in, as an example, src/communication/

@trivialfis
Copy link
Member Author

trivialfis commented Apr 24, 2019

@CodingCat I think @chenqin is doing a lots of work on rabit that I cannot do myself, and it seems jvm package always has tons of work to occupy your hand, so merging rabit into XGBoost won't advance it. My suggestion is add an option to rabit's build system for using existing dmlc-core instead of another git submodule.

@chenqin
Copy link
Contributor

chenqin commented Apr 24, 2019

per comments in pr, by default built with xgboost dmlc-core, if use rabit without xgboost or mxnet it can choose to build with dmlc-core as subproject with make RABIT_BUID_DMLC=1

IMO, this can address duplicated code base while keep rabit always in sync with dmlc-core.

dmlc/rabit#91

@trivialfis
Copy link
Member Author

Closed by dmlc/rabit#91 .

@lock lock bot locked as resolved and limited conversation to collaborators Jul 25, 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

No branches or pull requests

3 participants