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

Add options to enable use of GPU with LightGBM #492

Closed
wants to merge 3 commits into from

Conversation

mjmckp
Copy link

@mjmckp mjmckp commented Jul 5, 2018

Note: requires a build of LightGBM with GPU support. Addresses #500

@dnfclas
Copy link

dnfclas commented Jul 5, 2018

CLA assistant check
All CLA requirements met.

@shauheen shauheen requested a review from codemzs July 5, 2018 13:32
@shauheen
Copy link
Contributor

shauheen commented Jul 5, 2018

Thanks @mjmckp . Would you please file an issue and associate with this PR. I understand that @codemzs , @guolinke and you are also discussing this in #452 . We should move the discussion into a separate issue.

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;
Copy link
Contributor

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.

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Jul 5, 2018

Thank you for your contribution, @mjmckp
From what I understand from your comments in previous PR, is fact what GPU support required specifically compiled DLL, which user should manually put into specific folder.

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.

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

@mjmckp
Copy link
Author

mjmckp commented Jul 6, 2018

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.

Copy link
Member

@codemzs codemzs left a 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.

@guolinke
Copy link
Contributor

guolinke commented Jul 6, 2018

We don't have GPU binaries now due to the limitation of CI systems.
refer to: microsoft/LightGBM#544 (comment)

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.

@huanzhang12
Copy link

@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.

@guolinke
Copy link
Contributor

guolinke commented Jul 7, 2018

@huanzhang12 Is current GPU build static linking ?

@huanzhang12
Copy link

@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>
Copy link
Member

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.

@Zruty0
Copy link
Contributor

Zruty0 commented Oct 5, 2018

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.

@mjmckp
Copy link
Author

mjmckp commented Oct 7, 2018

This PR is stuck waiting for a NuGet package for LightGBM that supports GPU.

@Zruty0
Copy link
Contributor

Zruty0 commented Oct 12, 2018

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.

@Zruty0 Zruty0 closed this Oct 12, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants