From 3c7098c54723b806a0ff0784d7d3bfce2e022d05 Mon Sep 17 00:00:00 2001 From: Ranadeep Singh Date: Wed, 29 Jul 2026 05:17:35 -0700 Subject: [PATCH 1/2] fix: correct LightGBM improvement tolerance semantics ## 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 #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: https://github.com/microsoft/SynapseML/issues/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> --- .../synapse/ml/lightgbm/TrainUtils.scala | 26 ++++++++----- .../ml/lightgbm/params/LightGBMParams.scala | 2 +- .../ml/lightgbm/split1/TrainUtilsSuite.scala | 39 +++++++++++++++++++ 3 files changed, 56 insertions(+), 11 deletions(-) create mode 100644 lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala diff --git a/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala b/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala index 71579a65ae5..8540bc9c02b 100644 --- a/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala +++ b/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala @@ -11,7 +11,20 @@ import org.slf4j.Logger import java.io._ import scala.annotation.tailrec -private object TrainUtils extends Serializable { +private[lightgbm] object TrainUtils extends Serializable { + + private val HigherIsBetterMetricPrefixes = Seq("auc", "ndcg@", "map@", "average_precision") + + private[lightgbm] def isImprovement(evalName: String, + evalScore: Double, + bestScore: Double, + improvementTolerance: Double): Boolean = { + if (HigherIsBetterMetricPrefixes.exists(evalName.startsWith)) { + evalScore - bestScore > improvementTolerance + } else { + bestScore - evalScore > improvementTolerance + } + } def createBooster(trainParams: BaseTrainParams, trainDataset: LightGBMDataset, @@ -144,16 +157,9 @@ private object TrainUtils extends Serializable { val evalResults: Array[(String, Double)] = state.booster.getEvalResults(state.evalNames, 1) val results: Array[(String, Double)] = evalResults.zipWithIndex.map { case ((evalName, evalScore), index) => log.info(s"Valid $evalName=$evalScore") - val cmp = - if (evalName.startsWith("auc") - || evalName.startsWith("ndcg@") - || evalName.startsWith("map@") - || evalName.startsWith("average_precision")) - (x: Double, y: Double, tol: Double) => x - y > tol - else - (x: Double, y: Double, tol: Double) => x - y < tol if (state.bestScores(index) == null - || cmp(evalScore, state.bestScore(index), state.ctx.trainingCtx.improvementTolerance)) { + || isImprovement(evalName, evalScore, state.bestScore(index), + state.ctx.trainingCtx.improvementTolerance)) { state.bestScore(index) = evalScore state.bestIteration(index) = state.iteration state.bestScores(index) = evalResults.map(_._2) diff --git a/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala b/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala index a0faabd07e8..2b84e6e41ab 100644 --- a/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala +++ b/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala @@ -191,7 +191,7 @@ trait LightGBMLearnerParams extends Wrappable { def setEarlyStoppingRound(value: Int): this.type = set(earlyStoppingRound, value) val improvementTolerance = new DoubleParam(this, "improvementTolerance", - "Tolerance to consider improvement in metric") + "Metric improvement must exceed this value to reset the early stopping counter") setDefault(improvementTolerance -> 0.0) def getImprovementTolerance: Double = $(improvementTolerance) def setImprovementTolerance(value: Double): this.type = set(improvementTolerance, value) diff --git a/lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala b/lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala new file mode 100644 index 00000000000..9f586cfef24 --- /dev/null +++ b/lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala @@ -0,0 +1,39 @@ +// Copyright (C) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE in project root for information. + +package com.microsoft.azure.synapse.ml.lightgbm.split1 + +import com.microsoft.azure.synapse.ml.lightgbm.TrainUtils +import org.scalatest.funsuite.AnyFunSuite + +class TrainUtilsSuite extends AnyFunSuite { + + test("Improvement tolerance requires sufficient improvement for lower-is-better metrics") { + val bestScore = 1.0 + val tolerance = 0.25 + + assert(TrainUtils.isImprovement("rmse", 0.5, bestScore, tolerance)) + assert(!TrainUtils.isImprovement("rmse", 0.75, bestScore, tolerance)) + assert(!TrainUtils.isImprovement("rmse", 0.875, bestScore, tolerance)) + assert(!TrainUtils.isImprovement("rmse", 1.125, bestScore, tolerance)) + } + + test("Improvement tolerance remains correct for higher-is-better metrics") { + val bestScore = 0.5 + val tolerance = 0.25 + + Seq("auc", "ndcg@1", "map@1", "average_precision").foreach { metric => + assert(TrainUtils.isImprovement(metric, 1.0, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, 0.75, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, 0.625, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, 0.375, bestScore, tolerance)) + } + } + + test("Zero improvement tolerance requires strict improvement") { + assert(TrainUtils.isImprovement("rmse", 0.9, 1.0, 0.0)) + assert(!TrainUtils.isImprovement("rmse", 1.0, 1.0, 0.0)) + assert(TrainUtils.isImprovement("auc", 0.6, 0.5, 0.0)) + assert(!TrainUtils.isImprovement("auc", 0.5, 0.5, 0.0)) + } +} From 20630db70b9132ae2573593c731a39e5011f7d2c Mon Sep 17 00:00:00 2001 From: Ranadeep Singh Date: Wed, 29 Jul 2026 15:26:55 -0700 Subject: [PATCH 2/2] fix: harden LightGBM early stopping parameters ## 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 #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: https://github.com/microsoft/SynapseML/issues/2565 - Pull request: https://github.com/microsoft/SynapseML/pull/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> --- .../synapse/ml/lightgbm/TrainUtils.scala | 9 +- .../ml/lightgbm/params/LightGBMParams.scala | 11 +- .../ml/lightgbm/split1/TrainUtilsSuite.scala | 101 ++++++++++++++---- 3 files changed, 95 insertions(+), 26 deletions(-) diff --git a/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala b/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala index 8540bc9c02b..dc2e631da46 100644 --- a/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala +++ b/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala @@ -26,6 +26,12 @@ private[lightgbm] object TrainUtils extends Serializable { } } + private[lightgbm] def shouldStopEarly(iteration: Int, + bestIteration: Int, + earlyStoppingRound: Int): Boolean = { + earlyStoppingRound > 0 && iteration - bestIteration >= earlyStoppingRound + } + def createBooster(trainParams: BaseTrainParams, trainDataset: LightGBMDataset, validDatasetOpt: Option[LightGBMDataset]): LightGBMBooster = { @@ -163,7 +169,8 @@ private[lightgbm] object TrainUtils extends Serializable { state.bestScore(index) = evalScore state.bestIteration(index) = state.iteration state.bestScores(index) = evalResults.map(_._2) - } else if (state.iteration - state.bestIteration(index) >= state.ctx.trainingCtx.earlyStoppingRound) { + } else if (shouldStopEarly(state.iteration, state.bestIteration(index), + state.ctx.trainingCtx.earlyStoppingRound)) { state.isFinished = true log.info("Early stopping, best iteration is " + state.bestIteration(index)) state.bestIterationResult = Some(state.bestIteration(index)) diff --git a/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala b/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala index 2b84e6e41ab..57d40e89c17 100644 --- a/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala +++ b/lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala @@ -185,13 +185,18 @@ trait LightGBMDatasetParams extends Wrappable { /** Defines common parameters across all LightGBM learners related to learning score evolution. */ trait LightGBMLearnerParams extends Wrappable { - val earlyStoppingRound = new IntParam(this, "earlyStoppingRound", "Early stopping round") + val earlyStoppingRound = new IntParam(this, "earlyStoppingRound", + "Number of rounds without sufficient improvement before stopping; zero disables early stopping", + ParamValidators.gtEq(0)) setDefault(earlyStoppingRound -> 0) def getEarlyStoppingRound: Int = $(earlyStoppingRound) def setEarlyStoppingRound(value: Int): this.type = set(earlyStoppingRound, value) - val improvementTolerance = new DoubleParam(this, "improvementTolerance", - "Metric improvement must exceed this value to reset the early stopping counter") + val improvementTolerance = new DoubleParam( + this, + "improvementTolerance", + "Metric improvement must exceed this value to reset the early stopping counter", + ParamValidators.gtEq(0.0)) setDefault(improvementTolerance -> 0.0) def getImprovementTolerance: Double = $(improvementTolerance) def setImprovementTolerance(value: Double): this.type = set(improvementTolerance, value) diff --git a/lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala b/lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala index 9f586cfef24..277725c9dfd 100644 --- a/lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala +++ b/lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala @@ -3,37 +3,94 @@ package com.microsoft.azure.synapse.ml.lightgbm.split1 -import com.microsoft.azure.synapse.ml.lightgbm.TrainUtils +import com.microsoft.azure.synapse.ml.lightgbm.{LightGBMRegressor, TrainUtils} import org.scalatest.funsuite.AnyFunSuite class TrainUtilsSuite extends AnyFunSuite { - test("Improvement tolerance requires sufficient improvement for lower-is-better metrics") { - val bestScore = 1.0 - val tolerance = 0.25 + private val lowerIsBetterMetrics = Seq( + "rmse", + "l1", + "mae", + "l2", + "mse", + "binary_logloss", + "binary_error", + "multi_logloss", + "multi_error", + "mape", + "quantile", + "huber", + "fair", + "poisson", + "gamma", + "gamma_deviance", + "tweedie", + "cross_entropy", + "kullback_leibler") - assert(TrainUtils.isImprovement("rmse", 0.5, bestScore, tolerance)) - assert(!TrainUtils.isImprovement("rmse", 0.75, bestScore, tolerance)) - assert(!TrainUtils.isImprovement("rmse", 0.875, bestScore, tolerance)) - assert(!TrainUtils.isImprovement("rmse", 1.125, bestScore, tolerance)) - } + private val higherIsBetterMetrics = Seq( + "auc", + "auc_mu", + "ndcg@1", + "ndcg@10", + "map@1", + "map@10", + "average_precision") + + private val tolerances = Seq(0.0, 0.25, 2.0, 25.0) - test("Improvement tolerance remains correct for higher-is-better metrics") { - val bestScore = 0.5 - val tolerance = 0.25 + test("Improvement tolerance is symmetric across metrics and tolerance values") { + val bestScore = 100.0 + val margin = 0.125 - Seq("auc", "ndcg@1", "map@1", "average_precision").foreach { metric => - assert(TrainUtils.isImprovement(metric, 1.0, bestScore, tolerance)) - assert(!TrainUtils.isImprovement(metric, 0.75, bestScore, tolerance)) - assert(!TrainUtils.isImprovement(metric, 0.625, bestScore, tolerance)) - assert(!TrainUtils.isImprovement(metric, 0.375, bestScore, tolerance)) + tolerances.foreach { tolerance => + lowerIsBetterMetrics.foreach { metric => + assert(TrainUtils.isImprovement(metric, bestScore - tolerance - margin, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, bestScore - tolerance, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, bestScore - tolerance / 2, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, bestScore + margin, bestScore, tolerance)) + } + + higherIsBetterMetrics.foreach { metric => + assert(TrainUtils.isImprovement(metric, bestScore + tolerance + margin, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, bestScore + tolerance, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, bestScore + tolerance / 2, bestScore, tolerance)) + assert(!TrainUtils.isImprovement(metric, bestScore - margin, bestScore, tolerance)) + } } } - test("Zero improvement tolerance requires strict improvement") { - assert(TrainUtils.isImprovement("rmse", 0.9, 1.0, 0.0)) - assert(!TrainUtils.isImprovement("rmse", 1.0, 1.0, 0.0)) - assert(TrainUtils.isImprovement("auc", 0.6, 0.5, 0.0)) - assert(!TrainUtils.isImprovement("auc", 0.5, 0.5, 0.0)) + test("Zero early stopping rounds disable wrapper early stopping") { + assert(!TrainUtils.shouldStopEarly( + iteration = 100, + bestIteration = 0, + earlyStoppingRound = 0)) + } + + test("Positive early stopping rounds stop only when the round boundary is reached") { + assert(!TrainUtils.shouldStopEarly(iteration = 4, bestIteration = 0, earlyStoppingRound = 5)) + assert(TrainUtils.shouldStopEarly(iteration = 5, bestIteration = 0, earlyStoppingRound = 5)) + assert(TrainUtils.shouldStopEarly(iteration = 10, bestIteration = 5, earlyStoppingRound = 5)) + } + + test("Early stopping parameters accept valid values and reject invalid values") { + val learner = new LightGBMRegressor() + + assert(learner.getEarlyStoppingRound == 0) + assert(learner.getImprovementTolerance == 0.0) + + Seq(0, 1, 100, Int.MaxValue).foreach { earlyStoppingRound => + assert(learner.setEarlyStoppingRound(earlyStoppingRound).getEarlyStoppingRound == earlyStoppingRound) + } + assertThrows[IllegalArgumentException](learner.setEarlyStoppingRound(-1)) + + Seq(0.0, 0.25, 25.0, Double.MaxValue).foreach { tolerance => + assert(learner.setImprovementTolerance(tolerance).getImprovementTolerance == tolerance) + } + + Seq(-0.25, Double.NegativeInfinity, Double.NaN).foreach { tolerance => + assertThrows[IllegalArgumentException](learner.setImprovementTolerance(tolerance)) + } } }