[python-package] Allow to pass early stopping min delta in params

[borchero]

Motivation

Currently, setting min_delta in the early stopping callback requires specifying the early stopping callback manually. It would be nice to continue passing all early-stopping-related parameters in params.

[borchero]

Finally ready now @jameslamb 😄

[jameslamb]

Looking into this, I just realized... early_stopping_min_delta is specific to the Python interface! I'm sorry for not noticing that earlier.

It isn't listed at https://lightgbm.readthedocs.io/en/latest/Parameters.html and isn't a valid property of the LightGBM::Config object on the C++ side (https://github.com/microsoft/LightGBM/blob/master/include/LightGBM/config.h).

I guess because when that was added, it was only added on the Python side and not in this C++ code:

LightGBM/src/boosting/gbdt.cpp

Lines 456 to 473 in 6a1ec44

	bool GBDT::EvalAndCheckEarlyStopping() {
	bool is_met_early_stopping = false;
	// print message for metric
	auto best_msg = OutputMetric(iter_);
	is_met_early_stopping = !best_msg.empty();
	if (is_met_early_stopping) {
	Log::Info("Early stopping at iteration %d, the best iteration round is %d",
	iter_, iter_ - early_stopping_round_);
	Log::Info("Output of best iteration round:\n%s", best_msg.c_str());
	// pop last early_stopping_round_ models
	for (int i = 0; i < early_stopping_round_ * num_tree_per_iteration_; ++i) {
	models_.pop_back();
	}
	}
	return is_met_early_stopping;
	}

Given that, if it's going to show up in the params dictionary (which I get is valuable because it's the type of parameter you'd like to search a space of values for during hyperparam tuning), then I I think it should be popped off very early in train() / cv(), like this:

early_stopping_min_delta = params.pop("early_stopping_min_delta", 0.0)

I haven't pulled your branch and tested, but strongly suspect LightGBM is raising this warning about an unrecognized parameter:

LightGBM/src/io/config.cpp

Line 28 in 6a1ec44

Log::Warning("Unknown parameter %s", kv);

ALTERNATIVELY... this PR could be expanded by adding early_stopping_min_delta to the Config object.

That'd involve:

• adding an entry in https://github.com/microsoft/LightGBM/blob/master/include/LightGBM/config.h
• running python ./helpers/parameter_generator.py
• modifying the tests from [python-package] [R-package] include more params in model text representation (fixes #6010) #6077 to be sure that parameter is making it through to model files and back:

  • LightGBM/tests/python_package_test/test_engine.py

    Line 1454 in 6a1ec44

    def test_all_expected_params_are_written_out_to_model_text(tmp_path):

  • LightGBM/R-package/tests/testthat/test_lgb.Booster.R

    Line 828 in 6a1ec44

    # entries with default values of params

---

@borchero @jmoralez what do you think?

• Option 1: add early_stopping_min_delta as a LightGBM-wide parameter (even if only the Python package currently uses it)
• Option 2: .pop() it off params in train() / cv() in the Python package

[jmoralez]

We didn't mark #2526 as done in #2302 because the min_delta should be implemented on the cpp side (ref), so it'd definitely be great to have it as a general LightGBM parameter.

[jameslamb]

Ah right! Thank you for those references.

Alright I think we should make adding it to the Config on the C++ side part of the scope here @borchero . So that LightGBM won't raise a warning and so it'll survive the round-trip to/from model files.

[borchero]

Makes sense! I'll take care, hopefully soon 😄

[borchero]

Modifying the cpp config seemed to be straightforward enough 😄 I guess this is ready for review again (in case CI passes)

[borchero]

Tangentially related to this PR: do you think, we can do another release of LightGBM after this is merged @jameslamb? I'd really like to get my hands on #6314 and #6353 😄 👀

[jameslamb]

we can do another release of LightGBM after this is merged @jameslamb?

Yeah I think it's a good time! It's been 3 months since the last one.

I'd love to get those fixes you mentioned out and #6019. I'll open a release PR where we can talk about what else we want to see get in before doing a release, and talk about whether it should be 4.3.1 or 4.4.0.

[jameslamb]

I guess this is ready for review again

Could you please modify the tests I mentioned in #6274 (review)? If this parameter is expected to make it into model files, we should test that to be sure that isn't accidentally broken in the future.

[borchero]

I think this should do it. I also adjusted the C++ early stopping logic to make use of the min delta.

[jameslamb]

Looks great! I added 2 more very very minor comments on the tests... do what you want with those, and I don't need to review again. You can merge this after you've addressed them.

This is a really nice improvement, thanks for working on it!

Are you interested in doing a follow-up PR to add this same support to the R package? It'd be very similar.

LightGBM/R-package/R/lgb.train.R

Lines 258 to 268 in 1443548

	# If user supplied early_stopping_rounds, add the early stopping callback
	if (using_early_stopping) {
	callbacks <- .add_cb(
	cb_list = callbacks
	, cb = cb_early_stop(
	stopping_rounds = early_stopping_rounds
	, first_metric_only = isTRUE(params[["first_metric_only"]])
	, verbose = params[["verbosity"]] > 0L
	)
	)
	}

LightGBM/R-package/R/callback.R

Line 187 in 1443548

cb_early_stop <- function(stopping_rounds, first_metric_only, verbose) {

If not, maybe @mayer79 would be interested (saw that he 👍🏻 'd some comments on this PR).

[borchero]

@jameslamb unfortunately, our AppVeyor build seems to be broken (also on main) which might be related to a new build of pyarrow on conda-forge (cf. https://anaconda.org/conda-forge/pyarrow/files: v15.0.2 build 5 was published right before our CI started failing).

Based on my what-I-thought-is-a-bug-report, I opened #6425 to try something out to fix it.

Unfortunately, I cannot replicate the issue "locally", hence, I'm a little stuck debugging 👀

[jameslamb]

@borchero I just applied these 2 small suggestions:

• [python-package] Allow to pass early stopping min delta in params #6274 (comment)
• [python-package] Allow to pass early stopping min delta in params #6274 (comment)

And I'll merge this PR once CI passes. If you disagree with either of them we can revert them, but I think they're non-controversial.

[borchero]

@borchero I just applied these 2 small suggestions:

Sure! 😄 I would have taken care once CI was functional again 😬

[jameslamb]

I would have taken care once CI

yeah sorry, I was too eager 😬

[jameslamb]

I'm so glad to see this one finally merged! Thanks for all your hard work, @borchero