-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add options to enable use of GPU with LightGBM #492
Conversation
…ghtGBM that supports GPU training)
public int GPUDeviceId = -1; | ||
|
||
[Argument(ArgumentType.AtMostOnce, HelpText = "Use double precision math on GPU? Note: only used when UseGPU is true.", ShortName = "gpu_use_dp")] | ||
public bool GPUUseDoublePrecision = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's overkill, but I would prefer to have interface ILightGbmExecutionDevice (or something like that) with two implementations one is LightGbmOnCpu with zero parameters and second LightGbmOnGpu with all current parameters, and interface method SetupOption(Dictionary<string, object> options).
So here you can create ISupportLightGbmExecutionDeviceFactory derived from IComponentFactory. Then needed you can create instance from factory with .CreateComponent, and use instance SetupOption method in ToDictionary.
as example you can check IEarlyStoppingCriterionFactory in FastTree project.
Thank you for your contribution, @mjmckp Which isn't obvious from user perspective, and it's not mentioned anywhere in this PR. Is it still true, or I misunderstand something. If it's true, is it dll machine specific or it's a platform specific? If it's machine specific, then we need somehow deliver building information to a user. If it's platform specific, I would rather help @guolinke with build infrastructure and create LightGBM.GPU nuget which we can consume. This is great PR, and I would assume it would make your life easier, but I don't want our abstract user to be frustrated. Imagine someone setting up this GPU flags, and a) GPU not get used b) code falling with cryptic exception. As an user I would be highly unsatisfied by this behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
…uires compatible build of LightGBM.
Thanks for the feedback @Ivanidzo4ka, please see latest commit. Regarding the LightGBM dlls, I think they are platform specific, hopefully @guolinke can provide some guidance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mjmckp ! As discussed before we cannot merge this PR until we have fixed the nuget to contain dlls that support GPUs otherwise it is just a bad user experience even though you have added the requirement in the comments. I understand there is an urge to have this in ML.NET and we will try our best to get it in but we want to do it the right way.
We don't have GPU binaries now due to the limitation of CI systems. Another problem is, GPU version LightGBM needs Boost and OpenCL. Are they already in ML.NET ? Or users need to install them ? ping @huanzhang12. is that possible to build GPU version for windows, linux and OSX ? I think we only need the build, not need to run it and test it, and the NVIDIA version is okay. |
@guolinke Yes we should be able to build the packages, as long as we can test them in a CI system. But there might be dependency issues, for example, if we build against a specific version of Boost, an user also need to have Boost (with the exact, or probably a higher version) installed. Especially for Windows, this can be a trouble. We should either make a statically linked build, or include these libraries in the final package for release. I prefer using static linking. |
@huanzhang12 Is current GPU build static linking ? |
@guolinke I don't think we are building static executable. We probably need to tweak CMakeList.txt to add this option (I am not sure if such an option is already available in CMakeList.txt). |
/// Execution device for training (CPU or GPU). | ||
/// NOTE: GPU training requires compatible build of LightGBM as described here: | ||
/// https://github.com/Microsoft/LightGBM/blob/master/docs/Installation-Guide.rst#build-gpu-version | ||
/// </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth moving this comment on the actual LightGBMArguments file, since this file is generated. Mabye on the ExecutionDevice classes.
Hi @mjmckp , thanks for your contribution! Are you planning to work further on this PR, or should we close it? You can reopen and continue when you are ready. |
This PR is stuck waiting for a NuGet package for LightGBM that supports GPU. |
Given that last comment from @huanzhang12 was in July, I would assume that this work is not prioritized. I will close this PR, as it's accumulating diffs. Feel free to reopen once you have time, and once LightGBM has a GPU-enabled NuGet. |
Note: requires a build of LightGBM with GPU support. Addresses #500