Add options to enable use of GPU with LightGBM - #492

Closed
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu
Closed

Add options to enable use of GPU with LightGBM#492
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu

Conversation

@mjmckp

@mjmckpmjmckp commented Jul 5, 2018

Copy link
Copy Markdown

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

@dnfclas

dnfclas commented Jul 5, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@shauheen
shauheen requested a review from codemzsJuly 5, 2018 13:32
@shauheen

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Ivanidzo4ka commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

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.

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕐

@mjmckp

Copy link
Copy Markdown
Author

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.

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

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

guolinke commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

We don't have GPU binaries now due to the limitation of CI systems.
refer to: lightgbm-org/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
Copy Markdown

@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
Copy Markdown
Contributor

@huanzhang12 Is current GPU build static linking ?

@huanzhang12

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@Zruty0

Copy link
Copy Markdown
Contributor

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.

@Zruty0Zruty0 closed this Oct 12, 2018
@ghostghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for freeto 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

@mjmckp@dnfclas@shauheen@Ivanidzo4ka@guolinke@huanzhang12@Zruty0@codemzs@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add options to enable use of GPU with LightGBM - #492

Closed
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu
Closed

Add options to enable use of GPU with LightGBM#492
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu

Conversation

@mjmckp

@mjmckpmjmckp commented Jul 5, 2018

Copy link
Copy Markdown

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

@dnfclas

dnfclas commented Jul 5, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@shauheen
shauheen requested a review from codemzsJuly 5, 2018 13:32
@shauheen

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Ivanidzo4ka commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

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.

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕐

@mjmckp

Copy link
Copy Markdown
Author

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.

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

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

guolinke commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

We don't have GPU binaries now due to the limitation of CI systems.
refer to: lightgbm-org/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
Copy Markdown

@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
Copy Markdown
Contributor

@huanzhang12 Is current GPU build static linking ?

@huanzhang12

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@Zruty0

Copy link
Copy Markdown
Contributor

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.

@Zruty0Zruty0 closed this Oct 12, 2018
@ghostghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for freeto 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

@mjmckp@dnfclas@shauheen@Ivanidzo4ka@guolinke@huanzhang12@Zruty0@codemzs@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add options to enable use of GPU with LightGBM - #492

Closed
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu
Closed

Add options to enable use of GPU with LightGBM#492
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu

Conversation

@mjmckp

@mjmckpmjmckp commented Jul 5, 2018

Copy link
Copy Markdown

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

@dnfclas

dnfclas commented Jul 5, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@shauheen
shauheen requested a review from codemzsJuly 5, 2018 13:32
@shauheen

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Ivanidzo4ka commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

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.

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕐

@mjmckp

Copy link
Copy Markdown
Author

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.

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

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

guolinke commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

We don't have GPU binaries now due to the limitation of CI systems.
refer to: lightgbm-org/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
Copy Markdown

@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
Copy Markdown
Contributor

@huanzhang12 Is current GPU build static linking ?

@huanzhang12

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@Zruty0

Copy link
Copy Markdown
Contributor

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.

@Zruty0Zruty0 closed this Oct 12, 2018
@ghostghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for freeto 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

@mjmckp@dnfclas@shauheen@Ivanidzo4ka@guolinke@huanzhang12@Zruty0@codemzs@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add options to enable use of GPU with LightGBM - #492

Closed
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu
Closed

Add options to enable use of GPU with LightGBM#492
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu

Conversation

@mjmckp

@mjmckpmjmckp commented Jul 5, 2018

Copy link
Copy Markdown

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

@dnfclas

dnfclas commented Jul 5, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@shauheen
shauheen requested a review from codemzsJuly 5, 2018 13:32
@shauheen

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Ivanidzo4ka commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

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.

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕐

@mjmckp

Copy link
Copy Markdown
Author

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.

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

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

guolinke commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

We don't have GPU binaries now due to the limitation of CI systems.
refer to: lightgbm-org/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
Copy Markdown

@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
Copy Markdown
Contributor

@huanzhang12 Is current GPU build static linking ?

@huanzhang12

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@Zruty0

Copy link
Copy Markdown
Contributor

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.

@Zruty0Zruty0 closed this Oct 12, 2018
@ghostghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for freeto 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

@mjmckp@dnfclas@shauheen@Ivanidzo4ka@guolinke@huanzhang12@Zruty0@codemzs@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add options to enable use of GPU with LightGBM - #492

Closed
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu
Closed

Add options to enable use of GPU with LightGBM#492
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu

Conversation

@mjmckp

@mjmckpmjmckp commented Jul 5, 2018

Copy link
Copy Markdown

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

@dnfclas

dnfclas commented Jul 5, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@shauheen
shauheen requested a review from codemzsJuly 5, 2018 13:32
@shauheen

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Ivanidzo4ka commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

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.

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕐

@mjmckp

Copy link
Copy Markdown
Author

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.

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

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

guolinke commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

We don't have GPU binaries now due to the limitation of CI systems.
refer to: lightgbm-org/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
Copy Markdown

@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
Copy Markdown
Contributor

@huanzhang12 Is current GPU build static linking ?

@huanzhang12

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@Zruty0

Copy link
Copy Markdown
Contributor

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.

@Zruty0Zruty0 closed this Oct 12, 2018
@ghostghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for freeto 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

@mjmckp@dnfclas@shauheen@Ivanidzo4ka@guolinke@huanzhang12@Zruty0@codemzs@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add options to enable use of GPU with LightGBM - #492

Closed
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu
Closed

Add options to enable use of GPU with LightGBM#492
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu

Conversation

@mjmckp

@mjmckpmjmckp commented Jul 5, 2018

Copy link
Copy Markdown

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

@dnfclas

dnfclas commented Jul 5, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@shauheen
shauheen requested a review from codemzsJuly 5, 2018 13:32
@shauheen

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Ivanidzo4ka commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

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.

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕐

@mjmckp

Copy link
Copy Markdown
Author

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.

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

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

guolinke commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

We don't have GPU binaries now due to the limitation of CI systems.
refer to: lightgbm-org/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
Copy Markdown

@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
Copy Markdown
Contributor

@huanzhang12 Is current GPU build static linking ?

@huanzhang12

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@Zruty0

Copy link
Copy Markdown
Contributor

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.

@Zruty0Zruty0 closed this Oct 12, 2018
@ghostghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for freeto 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

@mjmckp@dnfclas@shauheen@Ivanidzo4ka@guolinke@huanzhang12@Zruty0@codemzs@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add options to enable use of GPU with LightGBM - #492

Closed
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu
Closed

Add options to enable use of GPU with LightGBM#492
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu

Conversation

@mjmckp

@mjmckpmjmckp commented Jul 5, 2018

Copy link
Copy Markdown

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

@dnfclas

dnfclas commented Jul 5, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@shauheen
shauheen requested a review from codemzsJuly 5, 2018 13:32
@shauheen

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Ivanidzo4ka commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

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.

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕐

@mjmckp

Copy link
Copy Markdown
Author

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.

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

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

guolinke commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

We don't have GPU binaries now due to the limitation of CI systems.
refer to: lightgbm-org/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
Copy Markdown

@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
Copy Markdown
Contributor

@huanzhang12 Is current GPU build static linking ?

@huanzhang12

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@Zruty0

Copy link
Copy Markdown
Contributor

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.

@Zruty0Zruty0 closed this Oct 12, 2018
@ghostghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for freeto 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

@mjmckp@dnfclas@shauheen@Ivanidzo4ka@guolinke@huanzhang12@Zruty0@codemzs@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add options to enable use of GPU with LightGBM - #492

Closed
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu
Closed

Add options to enable use of GPU with LightGBM#492
mjmckp wants to merge 3 commits into
dotnet:masterfrom
mjmckp:usegpu

Conversation

@mjmckp

@mjmckpmjmckp commented Jul 5, 2018

Copy link
Copy Markdown

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

@dnfclas

dnfclas commented Jul 5, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@shauheen
shauheen requested a review from codemzsJuly 5, 2018 13:32
@shauheen

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Ivanidzo4ka commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

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.

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕐

@mjmckp

Copy link
Copy Markdown
Author

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.

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

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

guolinke commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

We don't have GPU binaries now due to the limitation of CI systems.
refer to: lightgbm-org/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
Copy Markdown

@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
Copy Markdown
Contributor

@huanzhang12 Is current GPU build static linking ?

@huanzhang12

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@Zruty0

Copy link
Copy Markdown
Contributor

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.

@Zruty0Zruty0 closed this Oct 12, 2018
@ghostghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for freeto 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

@mjmckp@dnfclas@shauheen@Ivanidzo4ka@guolinke@huanzhang12@Zruty0@codemzs@sfilipi