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

improving LightGBM, XGBoost experience with Dask #104

Closed
jameslamb opened this issue Oct 26, 2020 · 24 comments
Closed

improving LightGBM, XGBoost experience with Dask #104

jameslamb opened this issue Oct 26, 2020 · 24 comments

Comments

@jameslamb
Copy link
Member

👋 hello from Chicago!

I'm a LightGBM maintainer and an engineer at Saturn Cloud. I'd like to devote some cycles to improving the experience of using XGBoost and LightGBM on Dask.

I'm planning to talk with the XGBoost maintainers about helping out on the issues noted in dask/dask-xgboost#39 , and starting a similar discussion proposing that we migrate https://github.com/dask/dask-lightgbm into LightGBM directly and maintain it there.

Success for this would look like formally deprecating dask-xgboost and dask-lightgbm eventually and focusing all development in XGBoost and LightGBM (similar to how xgboost-spark and xgboost-flink are maintained in the main xgboost project).

What do folks here think about this? Are there other groups besides the maintainers of projects I've mentioned above that should be involved in this conversation?

Thanks for your time and consideration.

@TomAugspurger
Copy link
Member

Thanks for opening this. I'm generally +1 on the core libraries (XGBoost, LightGBM) handling this if they're willing.

There might be some building blocks that should go into dask.distributed (e.g. dask/distributed#3075).

cc @striajan and @SfinxCZ from the dask-lightgbm side.

@martindurant
Copy link
Member

Great to hear from you. I think both these efforts are seen as high profile and important from the community's point of view.

Yes, this is probably the best place to start the conversation aside from the two repos (but maybe xref on dask-ml for a slightly different set of eyes). If you want a more general conversation around the strategy and Dask's involvement in the projects, you might want t attend our community meeting on the first Thursday of the month.

@SfinxCZ
Copy link

SfinxCZ commented Oct 26, 2020

Hello @jameslamb,

great to hear that you are interested in helping us with the dask-lightgbm package.

The idea of merging dask-lightgbm into main LightGBM repo seems reasonable to me. I agree with @TomAugspurger that main building blocks could be moved to dask.distributed. Another option is to move the common parts of the code to dask-ml which could be then set as optional dependency for the LightGBM package.

Second option that came to mi mind is to merge dask-xgboost and dask-lightgbm into single library (or moving to dask-ml), since a lot of the code can be reused, and specify LightGBM and xgboost as its optional dependencies and use it as middle-man between LightGBM,XGBoost and Dask libraries.

In general I like the idea. The only thing that I am not sure is where to separate the libraries and what should be maintained where. What are your plans (in LightGBM) for supporting different frameworks for distributed computing? Do you have/plan to have support for spark, flink, etc.? If you already have the support in your code, the effort you proposed could follow similar path.

@kkraus14
Copy link
Member

cc @RAMitchell @hcho3 @trivialfis from the xgboost side

@jameslamb
Copy link
Member Author

Thanks!

adding @guolinke , @StrikerRUS from LightGBM

What are your plans (in LightGBM) for supporting different frameworks for distributed computing? Do you have/plan to have support for spark, flink, etc.?

Today in LightGBM, the only option for distributed training that is maintained in the main project is our own (LightGBM::network), similar to how xgboost has rabit.

We produce a library for JVM languages using SWIG. This is then used by the mmlspark project to provide a Spark interface for LightGBM. I'm not aware of any Flink integration with LightGBM...I can at least confidently say there isn't one maintained in microsoft/LightGBM.

LightGBM doesn't currently directly support Dask...users who want to use LightGBM with Dask go to dask-lightgbm, maintained in dask/dask-lightgbm.

There are also kubeflow fairing operators for LightGBM, maintained in kubeflow/fairing.


Second option that came to mind is to merge dask-xgboost and dask-lightgbm into single library (or moving to dask-ml), since a lot of the code can be reused

I think that keeping the work in the xgboost and LightGBM repos is a better option, since those projects know the most about their own distributed training setup (rabit for xgboost, LightGBM::network for LightGBM). And then looking for opportunities to reuse as much code from distributed and dask-ml as possible, for example dask/distributed#3075 that @TomAugspurger mentioned.

This seems like the conclusion that this thread reached: dask/dask-xgboost#39

I think doing this also reduces the difference between the experience with, say, lightgbm vs. dask-lightgbm.

One reason that I've opened this issue is to say that I'm willing to do the legwork to coordinate between XGBoost and LightGBM to borrow from each other and to find opportunities to push things from the modelling frameworks upstream into distributed or dask-ml.

@SfinxCZ
Copy link

SfinxCZ commented Oct 26, 2020

I've checked the xgboost code for dask integration and it seems reasonable to use the same approach.

So, I am ok that dask-lightgbm functionality should be merged directly into LightGBM core library.

@datametrician
Copy link

datametrician commented Oct 28, 2020

Has anyone discussed deprecating dask-xgboost to avoid confusion?
Disregard I read dask/dask-xgboost#39

@TomAugspurger
Copy link
Member

TomAugspurger commented Oct 28, 2020 via email

@jameslamb
Copy link
Member Author

Yep! They're listed in this comment: dask/dask-xgboost#39 (comment).

I have some free cycles to work on this, and @hcho3 I'm willing to help with those issues in XGBoost if you need another set of hands 😀

@hcho3
Copy link

hcho3 commented Oct 28, 2020

@jameslamb Thanks for your offer. Currently the only blocking issue is dmlc/xgboost#5765. in the latest master branch, xgboost.dask now supports early stopping and predict_proba().

@jameslamb
Copy link
Member Author

I've checked the xgboost code for dask integration and it seems reasonable to use the same approach.

So, I am ok that dask-lightgbm functionality should be merged directly into LightGBM core library.

Thanks very much @SfinxCZ ! I've talked with other LightGBM maintainers in microsoft/LightGBM#2791 and they're open to the idea as well 🎉

Could you open a pull request into https://github.com/microsoft/LightGBM with the contents of dask_lightgbm?

We can talk in the PR about:

  • which code should be merged
  • how much you and other dask-lightgbm maintainers would want to still be involved once that code makes it into a LightGBM release
  • transferring any ideas you have for improvements to issues in the LightGBM backlog

And actually, before we do that...are there other dask-lightgbm maintainers that we need to get agreement from as well?

@datametrician
Copy link

@jameslamb does that mean LightGBM will potentially have MNMG support :)

@jameslamb
Copy link
Member Author

@datametrician it could be a route to that, yes

@SfinxCZ
Copy link

SfinxCZ commented Nov 1, 2020

@jameslamb I've created new PR microsoft/LightGBM#3515 where I've put the core functionality + unit tests. I would suggest to move the discussion regarding the migration of the code to the PR.

Regarding other contributors, I am not sure if others are interested in the discussion (maybe @striajan ?), but I would suggest to start the moving process and if others choose to remain silent I would assume that they agree :-).

@mrocklin
Copy link
Member

mrocklin commented Nov 1, 2020 via email

@striajan
Copy link

striajan commented Nov 2, 2020

Thanks for including me into the discussion. To put my two cents in, integration of this repository into the main LightGBM repository sounds great, even though I do not plan to work on it in the future because of moving to a different company. Maybe @shcherbin would be interested in this discussion?

@hcho3
Copy link

hcho3 commented Nov 12, 2020

The last blocking issue (dmlc/xgboost#5765) has been addressed, so we can proceed with migration from dask-xgboost to xgboost.dask. We are planning to release a Release Candidate for the next release (XGBoost 1.3.0) by end of this month.

@jameslamb
Copy link
Member Author

Wanted to update those following this issue.

Thanks to a lot of effort from @SfinxCZ , the main parts of dask-lightgbm were merged into LightGBM today: microsoft/LightGBM#3515.

I think there will be substantial changes to it between now and the 3.2.0 release of lightgbm, but it's there now 😀

@jameslamb
Copy link
Member Author

I've opened up a Request For Comment in LightGBM, about how LightGBM should get the Dask client you want to use. If anyone here has time / interest, we'd appreciate comments at microsoft/LightGBM#3808.

Thanks to @jsignell for already adding her opinion.

@jameslamb
Copy link
Member Author

lightgbm 3.2.0 was released today 🎉

This is the first release of lightgbm that includes a Dask integration (which began with @SfinxCZ graciously upstreaming dask-lightgbm in microsoft/LightGBM#3515).

You can see the full details of this release at https://github.com/microsoft/LightGBM/releases/tag/v3.2.0.

Thanks to everyone on this issue for your help. I especially want to shout out @ffineis, @jmoralez, and @StrikerRUS for putting a lot of time and energy into this first lightgbm.dask release.

@jakirkham
Copy link
Member

jakirkham commented Mar 23, 2021

Thanks for the update James! 😀 Nice work everyone! 👏

Should we go ahead and close this issue out then? 😉

@mrocklin
Copy link
Member

mrocklin commented Mar 23, 2021 via email

@jameslamb
Copy link
Member Author

Should we go ahead and close this issue out then?

@jakirkham I think it can be closed, yeah!

There is still some work to be done but all the relevant conversations are already happening in the specific projects that might be affected, so I don't think we need this issue any more.

I'm going to close this out, thanks again to everyone for your help!

@jakirkham
Copy link
Member

Yeah it sounds like with XGBoost that dask-xgboost issue will get closed out on the next release. Looks like dask-lightgbm just needs release. Cc'd some folks on the Dask-ML issue to see if they have thoughts there

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

No branches or pull requests

10 participants