Add deterministic option for LightGBM - #7415

Merged
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic
Mar 14, 2025
Merged

Add deterministic option for LightGBM#7415
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic

Conversation

@michaelgsharp

Copy link
Copy Markdown
Contributor

Fixes#7326.

Adds the LightGBM deterministic option to the LightGBM Options.

@michaelgsharpmichaelgsharp self-assigned this Mar 12, 2025
CopilotAI review requested due to automatic review settings March 12, 2025 00:11

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

Pull Request Overview

This PR adds a deterministic option along with related options for the LightGBM trainer to ensure reproducible training outcomes. Key changes include:

  • Adding new options (Deterministic, ForceRowWise, and ForceColumnWise) in the LightGBM trainer options.
  • Updating the options mapping dictionary in the trainer base.
  • Updating tests to set the new options for LightGBM estimators.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.csAdded dictionary entries and properties for deterministic options.
test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.csUpdated tests to initialize Deterministic and ForceRowWise options.
Comments suppressed due to low confidence (2)

src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:243

  • [nitpick] Consider clarifying the help text for 'Deterministic' to state that it ensures reproducible training outcomes, rather than mentioning 'stable results'.
/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:69

  • Consider adding a test case that explicitly sets and verifies the behavior of the 'ForceColumnWise' option, as it is a new addition not covered in the existing tests.
Deterministic = true,

/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to use deterministic algorithm.")]
public bool Deterministic = false;

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.

public bool Deterministic = false;

I know this class is exposing fields directly, but I am wondering can the new added stuff be properties?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I believe we use reflection to instantiate these (like when you run this from ML.NET command line), and if its expecting fields it wouldn't work. I will double check on that before I merge this in.

If it is doing that, its possible that we could change things to either do both or only do properites and update all the options as well.

@ericstj

ericstj commented Mar 12, 2025

Copy link
Copy Markdown
Member

Microsoft.ML.RunTests.TestEntryPoints.EntryPointCatalog is failing on all legs. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7415-merge-504196c4ce1c401a92/Microsoft.ML.Core.Tests/1/console.55b4bee6.log?helixlogtype=result

�[m�[30;1m Output:
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_ep-list.tsv and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/core_ep-list.tsv
�[m�[37m Output matches baseline: '../Common/EntryPoints/core_ep-list.tsv'
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_manifest.json and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/netcoreapp/core_manifest.json
�[m�[37m *** Failure #1: Output and baseline mismatch at line 11895, expected ' "Name": "ParallelTrainer",' but got ' "Name": "Deterministic",' : '../Common/EntryPoints/core_manifest.json'

Looks like you need to update core_ep-list.tsv@michaelgsharp

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

@ericstj good catch. Its been updated.

@codecov

codecovBot commented Mar 12, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.96%. Comparing base (c36975c) to head (cd49ab2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #7415 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.01% 
==========================================
Files 1481 1481 Lines 273696 273708 +12 Branches 28285 28285 ==========================================
- Hits 188782 188769 -13 - Misses 77526 77546 +20 - Partials 7388 7393 +5 
FlagCoverage Δ
Debug68.96% <100.00%> (-0.01%)⬇️
production63.26% <100.00%> (-0.01%)⬇️
test89.46% <100.00%> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs80.30% <100.00%> (-0.03%)⬇️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs97.85% <100.00%> (+<0.01%)⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

/ba-g failed tests are known failures and build analysis still isn't configured correctly to go green.

@michaelgsharp
michaelgsharp merged commit adad40c into dotnet:mainMar 14, 2025
@michaelgsharp
michaelgsharp deleted the light-gbm-deterministic branch March 14, 2025 04:37
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 13, 2025
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.

Add option Deterministic to LightGbmBinaryTrainer.Options and LightGbmMulticlassTrainer.Options

5 participants

@michaelgsharp@ericstj@tarekgh@LittleLittleCloud
, '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 deterministic option for LightGBM - #7415

Merged
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic
Mar 14, 2025
Merged

Add deterministic option for LightGBM#7415
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic

Conversation

@michaelgsharp

Copy link
Copy Markdown
Contributor

Fixes#7326.

Adds the LightGBM deterministic option to the LightGBM Options.

@michaelgsharpmichaelgsharp self-assigned this Mar 12, 2025
CopilotAI review requested due to automatic review settings March 12, 2025 00:11

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

Pull Request Overview

This PR adds a deterministic option along with related options for the LightGBM trainer to ensure reproducible training outcomes. Key changes include:

  • Adding new options (Deterministic, ForceRowWise, and ForceColumnWise) in the LightGBM trainer options.
  • Updating the options mapping dictionary in the trainer base.
  • Updating tests to set the new options for LightGBM estimators.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.csAdded dictionary entries and properties for deterministic options.
test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.csUpdated tests to initialize Deterministic and ForceRowWise options.
Comments suppressed due to low confidence (2)

src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:243

  • [nitpick] Consider clarifying the help text for 'Deterministic' to state that it ensures reproducible training outcomes, rather than mentioning 'stable results'.
/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:69

  • Consider adding a test case that explicitly sets and verifies the behavior of the 'ForceColumnWise' option, as it is a new addition not covered in the existing tests.
Deterministic = true,

/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to use deterministic algorithm.")]
public bool Deterministic = false;

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.

public bool Deterministic = false;

I know this class is exposing fields directly, but I am wondering can the new added stuff be properties?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I believe we use reflection to instantiate these (like when you run this from ML.NET command line), and if its expecting fields it wouldn't work. I will double check on that before I merge this in.

If it is doing that, its possible that we could change things to either do both or only do properites and update all the options as well.

@ericstj

ericstj commented Mar 12, 2025

Copy link
Copy Markdown
Member

Microsoft.ML.RunTests.TestEntryPoints.EntryPointCatalog is failing on all legs. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7415-merge-504196c4ce1c401a92/Microsoft.ML.Core.Tests/1/console.55b4bee6.log?helixlogtype=result

�[m�[30;1m Output:
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_ep-list.tsv and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/core_ep-list.tsv
�[m�[37m Output matches baseline: '../Common/EntryPoints/core_ep-list.tsv'
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_manifest.json and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/netcoreapp/core_manifest.json
�[m�[37m *** Failure #1: Output and baseline mismatch at line 11895, expected ' "Name": "ParallelTrainer",' but got ' "Name": "Deterministic",' : '../Common/EntryPoints/core_manifest.json'

Looks like you need to update core_ep-list.tsv@michaelgsharp

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

@ericstj good catch. Its been updated.

@codecov

codecovBot commented Mar 12, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.96%. Comparing base (c36975c) to head (cd49ab2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #7415 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.01% 
==========================================
Files 1481 1481 Lines 273696 273708 +12 Branches 28285 28285 ==========================================
- Hits 188782 188769 -13 - Misses 77526 77546 +20 - Partials 7388 7393 +5 
FlagCoverage Δ
Debug68.96% <100.00%> (-0.01%)⬇️
production63.26% <100.00%> (-0.01%)⬇️
test89.46% <100.00%> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs80.30% <100.00%> (-0.03%)⬇️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs97.85% <100.00%> (+<0.01%)⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

/ba-g failed tests are known failures and build analysis still isn't configured correctly to go green.

@michaelgsharp
michaelgsharp merged commit adad40c into dotnet:mainMar 14, 2025
@michaelgsharp
michaelgsharp deleted the light-gbm-deterministic branch March 14, 2025 04:37
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 13, 2025
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.

Add option Deterministic to LightGbmBinaryTrainer.Options and LightGbmMulticlassTrainer.Options

5 participants

@michaelgsharp@ericstj@tarekgh@LittleLittleCloud
, '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 deterministic option for LightGBM - #7415

Merged
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic
Mar 14, 2025
Merged

Add deterministic option for LightGBM#7415
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic

Conversation

@michaelgsharp

Copy link
Copy Markdown
Contributor

Fixes#7326.

Adds the LightGBM deterministic option to the LightGBM Options.

@michaelgsharpmichaelgsharp self-assigned this Mar 12, 2025
CopilotAI review requested due to automatic review settings March 12, 2025 00:11

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

Pull Request Overview

This PR adds a deterministic option along with related options for the LightGBM trainer to ensure reproducible training outcomes. Key changes include:

  • Adding new options (Deterministic, ForceRowWise, and ForceColumnWise) in the LightGBM trainer options.
  • Updating the options mapping dictionary in the trainer base.
  • Updating tests to set the new options for LightGBM estimators.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.csAdded dictionary entries and properties for deterministic options.
test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.csUpdated tests to initialize Deterministic and ForceRowWise options.
Comments suppressed due to low confidence (2)

src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:243

  • [nitpick] Consider clarifying the help text for 'Deterministic' to state that it ensures reproducible training outcomes, rather than mentioning 'stable results'.
/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:69

  • Consider adding a test case that explicitly sets and verifies the behavior of the 'ForceColumnWise' option, as it is a new addition not covered in the existing tests.
Deterministic = true,

/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to use deterministic algorithm.")]
public bool Deterministic = false;

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.

public bool Deterministic = false;

I know this class is exposing fields directly, but I am wondering can the new added stuff be properties?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I believe we use reflection to instantiate these (like when you run this from ML.NET command line), and if its expecting fields it wouldn't work. I will double check on that before I merge this in.

If it is doing that, its possible that we could change things to either do both or only do properites and update all the options as well.

@ericstj

ericstj commented Mar 12, 2025

Copy link
Copy Markdown
Member

Microsoft.ML.RunTests.TestEntryPoints.EntryPointCatalog is failing on all legs. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7415-merge-504196c4ce1c401a92/Microsoft.ML.Core.Tests/1/console.55b4bee6.log?helixlogtype=result

�[m�[30;1m Output:
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_ep-list.tsv and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/core_ep-list.tsv
�[m�[37m Output matches baseline: '../Common/EntryPoints/core_ep-list.tsv'
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_manifest.json and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/netcoreapp/core_manifest.json
�[m�[37m *** Failure #1: Output and baseline mismatch at line 11895, expected ' "Name": "ParallelTrainer",' but got ' "Name": "Deterministic",' : '../Common/EntryPoints/core_manifest.json'

Looks like you need to update core_ep-list.tsv@michaelgsharp

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

@ericstj good catch. Its been updated.

@codecov

codecovBot commented Mar 12, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.96%. Comparing base (c36975c) to head (cd49ab2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #7415 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.01% 
==========================================
Files 1481 1481 Lines 273696 273708 +12 Branches 28285 28285 ==========================================
- Hits 188782 188769 -13 - Misses 77526 77546 +20 - Partials 7388 7393 +5 
FlagCoverage Δ
Debug68.96% <100.00%> (-0.01%)⬇️
production63.26% <100.00%> (-0.01%)⬇️
test89.46% <100.00%> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs80.30% <100.00%> (-0.03%)⬇️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs97.85% <100.00%> (+<0.01%)⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

/ba-g failed tests are known failures and build analysis still isn't configured correctly to go green.

@michaelgsharp
michaelgsharp merged commit adad40c into dotnet:mainMar 14, 2025
@michaelgsharp
michaelgsharp deleted the light-gbm-deterministic branch March 14, 2025 04:37
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 13, 2025
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.

Add option Deterministic to LightGbmBinaryTrainer.Options and LightGbmMulticlassTrainer.Options

5 participants

@michaelgsharp@ericstj@tarekgh@LittleLittleCloud
, '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 deterministic option for LightGBM - #7415

Merged
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic
Mar 14, 2025
Merged

Add deterministic option for LightGBM#7415
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic

Conversation

@michaelgsharp

Copy link
Copy Markdown
Contributor

Fixes#7326.

Adds the LightGBM deterministic option to the LightGBM Options.

@michaelgsharpmichaelgsharp self-assigned this Mar 12, 2025
CopilotAI review requested due to automatic review settings March 12, 2025 00:11

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

Pull Request Overview

This PR adds a deterministic option along with related options for the LightGBM trainer to ensure reproducible training outcomes. Key changes include:

  • Adding new options (Deterministic, ForceRowWise, and ForceColumnWise) in the LightGBM trainer options.
  • Updating the options mapping dictionary in the trainer base.
  • Updating tests to set the new options for LightGBM estimators.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.csAdded dictionary entries and properties for deterministic options.
test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.csUpdated tests to initialize Deterministic and ForceRowWise options.
Comments suppressed due to low confidence (2)

src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:243

  • [nitpick] Consider clarifying the help text for 'Deterministic' to state that it ensures reproducible training outcomes, rather than mentioning 'stable results'.
/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:69

  • Consider adding a test case that explicitly sets and verifies the behavior of the 'ForceColumnWise' option, as it is a new addition not covered in the existing tests.
Deterministic = true,

/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to use deterministic algorithm.")]
public bool Deterministic = false;

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.

public bool Deterministic = false;

I know this class is exposing fields directly, but I am wondering can the new added stuff be properties?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I believe we use reflection to instantiate these (like when you run this from ML.NET command line), and if its expecting fields it wouldn't work. I will double check on that before I merge this in.

If it is doing that, its possible that we could change things to either do both or only do properites and update all the options as well.

@ericstj

ericstj commented Mar 12, 2025

Copy link
Copy Markdown
Member

Microsoft.ML.RunTests.TestEntryPoints.EntryPointCatalog is failing on all legs. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7415-merge-504196c4ce1c401a92/Microsoft.ML.Core.Tests/1/console.55b4bee6.log?helixlogtype=result

�[m�[30;1m Output:
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_ep-list.tsv and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/core_ep-list.tsv
�[m�[37m Output matches baseline: '../Common/EntryPoints/core_ep-list.tsv'
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_manifest.json and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/netcoreapp/core_manifest.json
�[m�[37m *** Failure #1: Output and baseline mismatch at line 11895, expected ' "Name": "ParallelTrainer",' but got ' "Name": "Deterministic",' : '../Common/EntryPoints/core_manifest.json'

Looks like you need to update core_ep-list.tsv@michaelgsharp

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

@ericstj good catch. Its been updated.

@codecov

codecovBot commented Mar 12, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.96%. Comparing base (c36975c) to head (cd49ab2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #7415 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.01% 
==========================================
Files 1481 1481 Lines 273696 273708 +12 Branches 28285 28285 ==========================================
- Hits 188782 188769 -13 - Misses 77526 77546 +20 - Partials 7388 7393 +5 
FlagCoverage Δ
Debug68.96% <100.00%> (-0.01%)⬇️
production63.26% <100.00%> (-0.01%)⬇️
test89.46% <100.00%> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs80.30% <100.00%> (-0.03%)⬇️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs97.85% <100.00%> (+<0.01%)⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

/ba-g failed tests are known failures and build analysis still isn't configured correctly to go green.

@michaelgsharp
michaelgsharp merged commit adad40c into dotnet:mainMar 14, 2025
@michaelgsharp
michaelgsharp deleted the light-gbm-deterministic branch March 14, 2025 04:37
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 13, 2025
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.

Add option Deterministic to LightGbmBinaryTrainer.Options and LightGbmMulticlassTrainer.Options

5 participants

@michaelgsharp@ericstj@tarekgh@LittleLittleCloud
, '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 deterministic option for LightGBM - #7415

Merged
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic
Mar 14, 2025
Merged

Add deterministic option for LightGBM#7415
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic

Conversation

@michaelgsharp

Copy link
Copy Markdown
Contributor

Fixes#7326.

Adds the LightGBM deterministic option to the LightGBM Options.

@michaelgsharpmichaelgsharp self-assigned this Mar 12, 2025
CopilotAI review requested due to automatic review settings March 12, 2025 00:11

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

Pull Request Overview

This PR adds a deterministic option along with related options for the LightGBM trainer to ensure reproducible training outcomes. Key changes include:

  • Adding new options (Deterministic, ForceRowWise, and ForceColumnWise) in the LightGBM trainer options.
  • Updating the options mapping dictionary in the trainer base.
  • Updating tests to set the new options for LightGBM estimators.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.csAdded dictionary entries and properties for deterministic options.
test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.csUpdated tests to initialize Deterministic and ForceRowWise options.
Comments suppressed due to low confidence (2)

src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:243

  • [nitpick] Consider clarifying the help text for 'Deterministic' to state that it ensures reproducible training outcomes, rather than mentioning 'stable results'.
/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:69

  • Consider adding a test case that explicitly sets and verifies the behavior of the 'ForceColumnWise' option, as it is a new addition not covered in the existing tests.
Deterministic = true,

/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to use deterministic algorithm.")]
public bool Deterministic = false;

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.

public bool Deterministic = false;

I know this class is exposing fields directly, but I am wondering can the new added stuff be properties?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I believe we use reflection to instantiate these (like when you run this from ML.NET command line), and if its expecting fields it wouldn't work. I will double check on that before I merge this in.

If it is doing that, its possible that we could change things to either do both or only do properites and update all the options as well.

@ericstj

ericstj commented Mar 12, 2025

Copy link
Copy Markdown
Member

Microsoft.ML.RunTests.TestEntryPoints.EntryPointCatalog is failing on all legs. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7415-merge-504196c4ce1c401a92/Microsoft.ML.Core.Tests/1/console.55b4bee6.log?helixlogtype=result

�[m�[30;1m Output:
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_ep-list.tsv and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/core_ep-list.tsv
�[m�[37m Output matches baseline: '../Common/EntryPoints/core_ep-list.tsv'
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_manifest.json and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/netcoreapp/core_manifest.json
�[m�[37m *** Failure #1: Output and baseline mismatch at line 11895, expected ' "Name": "ParallelTrainer",' but got ' "Name": "Deterministic",' : '../Common/EntryPoints/core_manifest.json'

Looks like you need to update core_ep-list.tsv@michaelgsharp

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

@ericstj good catch. Its been updated.

@codecov

codecovBot commented Mar 12, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.96%. Comparing base (c36975c) to head (cd49ab2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #7415 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.01% 
==========================================
Files 1481 1481 Lines 273696 273708 +12 Branches 28285 28285 ==========================================
- Hits 188782 188769 -13 - Misses 77526 77546 +20 - Partials 7388 7393 +5 
FlagCoverage Δ
Debug68.96% <100.00%> (-0.01%)⬇️
production63.26% <100.00%> (-0.01%)⬇️
test89.46% <100.00%> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs80.30% <100.00%> (-0.03%)⬇️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs97.85% <100.00%> (+<0.01%)⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

/ba-g failed tests are known failures and build analysis still isn't configured correctly to go green.

@michaelgsharp
michaelgsharp merged commit adad40c into dotnet:mainMar 14, 2025
@michaelgsharp
michaelgsharp deleted the light-gbm-deterministic branch March 14, 2025 04:37
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 13, 2025
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.

Add option Deterministic to LightGbmBinaryTrainer.Options and LightGbmMulticlassTrainer.Options

5 participants

@michaelgsharp@ericstj@tarekgh@LittleLittleCloud
, '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 deterministic option for LightGBM - #7415

Merged
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic
Mar 14, 2025
Merged

Add deterministic option for LightGBM#7415
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic

Conversation

@michaelgsharp

Copy link
Copy Markdown
Contributor

Fixes#7326.

Adds the LightGBM deterministic option to the LightGBM Options.

@michaelgsharpmichaelgsharp self-assigned this Mar 12, 2025
CopilotAI review requested due to automatic review settings March 12, 2025 00:11

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

Pull Request Overview

This PR adds a deterministic option along with related options for the LightGBM trainer to ensure reproducible training outcomes. Key changes include:

  • Adding new options (Deterministic, ForceRowWise, and ForceColumnWise) in the LightGBM trainer options.
  • Updating the options mapping dictionary in the trainer base.
  • Updating tests to set the new options for LightGBM estimators.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.csAdded dictionary entries and properties for deterministic options.
test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.csUpdated tests to initialize Deterministic and ForceRowWise options.
Comments suppressed due to low confidence (2)

src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:243

  • [nitpick] Consider clarifying the help text for 'Deterministic' to state that it ensures reproducible training outcomes, rather than mentioning 'stable results'.
/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:69

  • Consider adding a test case that explicitly sets and verifies the behavior of the 'ForceColumnWise' option, as it is a new addition not covered in the existing tests.
Deterministic = true,

/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to use deterministic algorithm.")]
public bool Deterministic = false;

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.

public bool Deterministic = false;

I know this class is exposing fields directly, but I am wondering can the new added stuff be properties?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I believe we use reflection to instantiate these (like when you run this from ML.NET command line), and if its expecting fields it wouldn't work. I will double check on that before I merge this in.

If it is doing that, its possible that we could change things to either do both or only do properites and update all the options as well.

@ericstj

ericstj commented Mar 12, 2025

Copy link
Copy Markdown
Member

Microsoft.ML.RunTests.TestEntryPoints.EntryPointCatalog is failing on all legs. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7415-merge-504196c4ce1c401a92/Microsoft.ML.Core.Tests/1/console.55b4bee6.log?helixlogtype=result

�[m�[30;1m Output:
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_ep-list.tsv and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/core_ep-list.tsv
�[m�[37m Output matches baseline: '../Common/EntryPoints/core_ep-list.tsv'
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_manifest.json and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/netcoreapp/core_manifest.json
�[m�[37m *** Failure #1: Output and baseline mismatch at line 11895, expected ' "Name": "ParallelTrainer",' but got ' "Name": "Deterministic",' : '../Common/EntryPoints/core_manifest.json'

Looks like you need to update core_ep-list.tsv@michaelgsharp

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

@ericstj good catch. Its been updated.

@codecov

codecovBot commented Mar 12, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.96%. Comparing base (c36975c) to head (cd49ab2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #7415 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.01% 
==========================================
Files 1481 1481 Lines 273696 273708 +12 Branches 28285 28285 ==========================================
- Hits 188782 188769 -13 - Misses 77526 77546 +20 - Partials 7388 7393 +5 
FlagCoverage Δ
Debug68.96% <100.00%> (-0.01%)⬇️
production63.26% <100.00%> (-0.01%)⬇️
test89.46% <100.00%> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs80.30% <100.00%> (-0.03%)⬇️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs97.85% <100.00%> (+<0.01%)⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

/ba-g failed tests are known failures and build analysis still isn't configured correctly to go green.

@michaelgsharp
michaelgsharp merged commit adad40c into dotnet:mainMar 14, 2025
@michaelgsharp
michaelgsharp deleted the light-gbm-deterministic branch March 14, 2025 04:37
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 13, 2025
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.

Add option Deterministic to LightGbmBinaryTrainer.Options and LightGbmMulticlassTrainer.Options

5 participants

@michaelgsharp@ericstj@tarekgh@LittleLittleCloud
, '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 deterministic option for LightGBM - #7415

Merged
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic
Mar 14, 2025
Merged

Add deterministic option for LightGBM#7415
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic

Conversation

@michaelgsharp

Copy link
Copy Markdown
Contributor

Fixes#7326.

Adds the LightGBM deterministic option to the LightGBM Options.

@michaelgsharpmichaelgsharp self-assigned this Mar 12, 2025
CopilotAI review requested due to automatic review settings March 12, 2025 00:11

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

Pull Request Overview

This PR adds a deterministic option along with related options for the LightGBM trainer to ensure reproducible training outcomes. Key changes include:

  • Adding new options (Deterministic, ForceRowWise, and ForceColumnWise) in the LightGBM trainer options.
  • Updating the options mapping dictionary in the trainer base.
  • Updating tests to set the new options for LightGBM estimators.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.csAdded dictionary entries and properties for deterministic options.
test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.csUpdated tests to initialize Deterministic and ForceRowWise options.
Comments suppressed due to low confidence (2)

src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:243

  • [nitpick] Consider clarifying the help text for 'Deterministic' to state that it ensures reproducible training outcomes, rather than mentioning 'stable results'.
/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:69

  • Consider adding a test case that explicitly sets and verifies the behavior of the 'ForceColumnWise' option, as it is a new addition not covered in the existing tests.
Deterministic = true,

/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to use deterministic algorithm.")]
public bool Deterministic = false;

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.

public bool Deterministic = false;

I know this class is exposing fields directly, but I am wondering can the new added stuff be properties?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I believe we use reflection to instantiate these (like when you run this from ML.NET command line), and if its expecting fields it wouldn't work. I will double check on that before I merge this in.

If it is doing that, its possible that we could change things to either do both or only do properites and update all the options as well.

@ericstj

ericstj commented Mar 12, 2025

Copy link
Copy Markdown
Member

Microsoft.ML.RunTests.TestEntryPoints.EntryPointCatalog is failing on all legs. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7415-merge-504196c4ce1c401a92/Microsoft.ML.Core.Tests/1/console.55b4bee6.log?helixlogtype=result

�[m�[30;1m Output:
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_ep-list.tsv and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/core_ep-list.tsv
�[m�[37m Output matches baseline: '../Common/EntryPoints/core_ep-list.tsv'
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_manifest.json and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/netcoreapp/core_manifest.json
�[m�[37m *** Failure #1: Output and baseline mismatch at line 11895, expected ' "Name": "ParallelTrainer",' but got ' "Name": "Deterministic",' : '../Common/EntryPoints/core_manifest.json'

Looks like you need to update core_ep-list.tsv@michaelgsharp

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

@ericstj good catch. Its been updated.

@codecov

codecovBot commented Mar 12, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.96%. Comparing base (c36975c) to head (cd49ab2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #7415 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.01% 
==========================================
Files 1481 1481 Lines 273696 273708 +12 Branches 28285 28285 ==========================================
- Hits 188782 188769 -13 - Misses 77526 77546 +20 - Partials 7388 7393 +5 
FlagCoverage Δ
Debug68.96% <100.00%> (-0.01%)⬇️
production63.26% <100.00%> (-0.01%)⬇️
test89.46% <100.00%> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs80.30% <100.00%> (-0.03%)⬇️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs97.85% <100.00%> (+<0.01%)⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

/ba-g failed tests are known failures and build analysis still isn't configured correctly to go green.

@michaelgsharp
michaelgsharp merged commit adad40c into dotnet:mainMar 14, 2025
@michaelgsharp
michaelgsharp deleted the light-gbm-deterministic branch March 14, 2025 04:37
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 13, 2025
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.

Add option Deterministic to LightGbmBinaryTrainer.Options and LightGbmMulticlassTrainer.Options

5 participants

@michaelgsharp@ericstj@tarekgh@LittleLittleCloud
, '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 deterministic option for LightGBM - #7415

Merged
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic
Mar 14, 2025
Merged

Add deterministic option for LightGBM#7415
michaelgsharp merged 3 commits into
dotnet:mainfrom
michaelgsharp:light-gbm-deterministic

Conversation

@michaelgsharp

Copy link
Copy Markdown
Contributor

Fixes#7326.

Adds the LightGBM deterministic option to the LightGBM Options.

@michaelgsharpmichaelgsharp self-assigned this Mar 12, 2025
CopilotAI review requested due to automatic review settings March 12, 2025 00:11

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

Pull Request Overview

This PR adds a deterministic option along with related options for the LightGBM trainer to ensure reproducible training outcomes. Key changes include:

  • Adding new options (Deterministic, ForceRowWise, and ForceColumnWise) in the LightGBM trainer options.
  • Updating the options mapping dictionary in the trainer base.
  • Updating tests to set the new options for LightGBM estimators.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.csAdded dictionary entries and properties for deterministic options.
test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.csUpdated tests to initialize Deterministic and ForceRowWise options.
Comments suppressed due to low confidence (2)

src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:243

  • [nitpick] Consider clarifying the help text for 'Deterministic' to state that it ensures reproducible training outcomes, rather than mentioning 'stable results'.
/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:69

  • Consider adding a test case that explicitly sets and verifies the behavior of the 'ForceColumnWise' option, as it is a new addition not covered in the existing tests.
Deterministic = true,

/// Setting this to true should ensure the stable results when using the same data and the same parameters and different num_threads.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to use deterministic algorithm.")]
public bool Deterministic = false;

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.

public bool Deterministic = false;

I know this class is exposing fields directly, but I am wondering can the new added stuff be properties?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I believe we use reflection to instantiate these (like when you run this from ML.NET command line), and if its expecting fields it wouldn't work. I will double check on that before I merge this in.

If it is doing that, its possible that we could change things to either do both or only do properites and update all the options as well.

@ericstj

ericstj commented Mar 12, 2025

Copy link
Copy Markdown
Member

Microsoft.ML.RunTests.TestEntryPoints.EntryPointCatalog is failing on all legs. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7415-merge-504196c4ce1c401a92/Microsoft.ML.Core.Tests/1/console.55b4bee6.log?helixlogtype=result

�[m�[30;1m Output:
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_ep-list.tsv and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/core_ep-list.tsv
�[m�[37m Output matches baseline: '../Common/EntryPoints/core_ep-list.tsv'
�[m�[37m Comparing /private/tmp/helix/working/A4860978/w/ADD50A36/e/TestOutput/../Common/EntryPoints/core_manifest.json and /tmp/helix/working/A4860978/p/test/BaselineOutput/Common/EntryPoints/netcoreapp/core_manifest.json
�[m�[37m *** Failure #1: Output and baseline mismatch at line 11895, expected ' "Name": "ParallelTrainer",' but got ' "Name": "Deterministic",' : '../Common/EntryPoints/core_manifest.json'

Looks like you need to update core_ep-list.tsv@michaelgsharp

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

@ericstj good catch. Its been updated.

@codecov

codecovBot commented Mar 12, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.96%. Comparing base (c36975c) to head (cd49ab2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #7415 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.01% 
==========================================
Files 1481 1481 Lines 273696 273708 +12 Branches 28285 28285 ==========================================
- Hits 188782 188769 -13 - Misses 77526 77546 +20 - Partials 7388 7393 +5 
FlagCoverage Δ
Debug68.96% <100.00%> (-0.01%)⬇️
production63.26% <100.00%> (-0.01%)⬇️
test89.46% <100.00%> (+<0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs80.30% <100.00%> (-0.03%)⬇️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs97.85% <100.00%> (+<0.01%)⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelgsharp

Copy link
Copy Markdown
ContributorAuthor

/ba-g failed tests are known failures and build analysis still isn't configured correctly to go green.

@michaelgsharp
michaelgsharp merged commit adad40c into dotnet:mainMar 14, 2025
@michaelgsharp
michaelgsharp deleted the light-gbm-deterministic branch March 14, 2025 04:37
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 13, 2025
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.

Add option Deterministic to LightGbmBinaryTrainer.Options and LightGbmMulticlassTrainer.Options

5 participants

@michaelgsharp@ericstj@tarekgh@LittleLittleCloud