fix: correct LightGBM improvement tolerance semantics - #2578
fix: correct LightGBM improvement tolerance semantics#2578ranadeepsingh wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey @ranadeepsingh 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
There was a problem hiding this comment.
Pull request overview
Fixes LightGBM early-stopping “improvementTolerance” handling for lower-is-better metrics (e.g., RMSE/MAE) so that the early-stopping counter only resets when the metric improves by more than the configured tolerance, matching the semantics already used for higher-is-better metrics.
Changes:
- Introduces a shared
TrainUtils.isImprovementhelper to apply symmetric improvement logic for higher- vs lower-is-better metrics. - Updates validation-evaluation early-stopping logic to use the new improvement predicate.
- Adds focused unit tests and clarifies the
improvementToleranceparameter description.
Show a summary per file
| File | Description |
|---|---|
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala |
Adds isImprovement and updates early-stopping comparison to require improvement exceeding tolerance for both metric directions. |
lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala |
Adds unit tests covering lower-/higher-is-better metrics, boundary behavior, and zero-tolerance defaults. |
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala |
Clarifies improvementTolerance parameter documentation to reflect the corrected behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
## Summary Expand improvement-tolerance coverage across representative LightGBM metrics and tolerance values. Preserve disabled early stopping when earlyStoppingRound is zero, validate both early-stopping parameters, and document their accepted ranges. ## Prompting Intent The engineer requested broader parameter testing to ensure the issue microsoft#2565 fix does not introduce downstream regressions. Cover related defaults, boundaries, metric families, invalid values, and early-stopping-round interactions before updating the pull request. ## Linked Sources - GitHub issue: microsoft#2565 - Pull request: microsoft#2578 - LightGBM 3.3.5 parameters: https://lightgbm.readthedocs.io/en/v3.3.5/Parameters.html#early-stopping-round ## Rationale Correct tolerance semantics classify more rounds as non-improving, so the wrapper must explicitly preserve LightGBM's zero-means-disabled behavior. Shared Spark parameter validators reject values that LightGBM does not support, while deterministic matrix tests cover the decision logic without depending on platform-specific native binaries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2578 +/- ##
==========================================
- Coverage 84.79% 84.62% -0.17%
==========================================
Files 334 334
Lines 17801 17806 +5
Branches 1619 1623 +4
==========================================
- Hits 15094 15068 -26
- Misses 2707 2738 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Expanded validation after reviewing parameter interactions:
|
|
Retried only the failed jobs in Azure build 228836638. The recommendation job recovered, while the shared Python collection failures and GPU resize timeout reproduced. The baseline fixes are now isolated and validated:
No unrelated CI dependency or infrastructure changes were added to this LightGBM bug-fix PR. |
## Summary Require lower-is-better validation metrics to improve by more than improvementTolerance before resetting the early-stopping counter. Clarify the parameter documentation and add focused regression coverage for both metric directions and zero tolerance. ## Prompting Intent Investigate GitHub issue microsoft#2565 from a new branch based on master, determine whether the report is valid, and implement a complete fix suitable for an upstream SynapseML pull request. ## Linked Sources - GitHub issue: microsoft#2565 ## Rationale The existing higher-is-better comparison already treats improvementTolerance as a minimum delta, while lower-is-better metrics accepted small regressions. A package-internal comparison helper makes the intended symmetric behavior directly testable without adding a slow native LightGBM fixture or changing public APIs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Expand improvement-tolerance coverage across representative LightGBM metrics and tolerance values. Preserve disabled early stopping when earlyStoppingRound is zero, validate both early-stopping parameters, and document their accepted ranges. ## Prompting Intent The engineer requested broader parameter testing to ensure the issue microsoft#2565 fix does not introduce downstream regressions. Cover related defaults, boundaries, metric families, invalid values, and early-stopping-round interactions before updating the pull request. ## Linked Sources - GitHub issue: microsoft#2565 - Pull request: microsoft#2578 - LightGBM 3.3.5 parameters: https://lightgbm.readthedocs.io/en/v3.3.5/Parameters.html#early-stopping-round ## Rationale Correct tolerance semantics classify more rounds as non-improving, so the wrapper must explicitly preserve LightGBM's zero-means-disabled behavior. Shared Spark parameter validators reject values that LightGBM does not support, while deterministic matrix tests cover the decision logic without depending on platform-specific native binaries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fb8dc60 to
20630db
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Related Issues/PRs
Fixes #2565
What changes are proposed in this pull request?
Correct LightGBM early-stopping behavior when
improvementToleranceis positive for lower-is-better metrics such as RMSE and MAE.The previous comparison treated small regressions as improvements and reset the early-stopping counter. This change makes the comparison symmetric with higher-is-better metrics: the metric must improve by more than
improvementTolerancebefore the counter resets.The patch also:
earlyStoppingRound = 0behavior, which disables early stopping;NaNtolerances;Focused tests cover 26 representative lower-is-better and higher-is-better metric names across zero, small, medium, and large tolerances. They also cover exact tolerance boundaries, insufficient improvements, regressions, default values, disabled stopping, positive stopping-round boundaries, and invalid inputs.
How is this patch tested?
Commands:
The existing native LightGBM integration test was also attempted locally, but the packaged
lib_lightgbm.dylibcould not be loaded on macOS.Azure Pipelines build 228836638 passed all six LightGBM Scala unit-test jobs and the LightGBM R test job. The overall build failed for unrelated infrastructure/dependency issues affecting multiple components: Maven Central returned HTTP 429 during setup, and Python test collection failed on an
mlflow/google.protobufincompatibility before LightGBM Python tests executed.Failed-job retry update:
UnitTests recommendationpassed on attempt 2, confirming the Maven HTTP 429 was transient. The six shared Python jobs still failed during collection on the same MLflow/protobuf incompatibility, and the Databricks GPU cluster again timed out while resizing. These repository-wide baseline failures are handled separately by #2580 and #2579. PR #2580 has now passed full Azure build 228877170, including the LightGBM Python tests; PR #2579 independently passed its Databricks GPU job. This PR remains dependency-free and focused on #2565.Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?