WIP (do not merge yet): Export callback functions in R package (fixes #2479)

[mayer79]

This PR fixes #2479, i.e., it exports and documents the callback functions in R.

@jameslamb Currently, passing a list of callback functions to lgb.train() etc. does not yet work properly because the "old" parameters early_stopping_rounds, eval_freq etc. override the callback list. How should we deal with this?

1. Fully ignore the old parameters and only use the callback list with a deprecation warning.
2. Keep the old parameters and override them with the functions passed by callbacks. This will need a couple of changes in lgb.train(), lgb.cv(), lightgbm().

[jameslamb]

Thanks very much for working on this @mayer79 !

First, I think that before exporting these callbacks, we should change the names. I don't think R packages should use names with dots for anything other than S3 methods. There are many things in this package that violate that rule (e.g. lgb.train(), lgb.cv()) and they've been left unchanged for historical reasons, but I'd like to take this opportunity where new functions are being exported to try to avoid introducing more such names.

What do you think about this?

• cb.early.stop() --> cb_early_stop()
• cb.print.evaluation() --> cb_print_evaluation()
• cb.record.evaluation() --> cb_record_evaluation()

If you agree with that, would you be willing to open a separate PR just changing those names?

---

passing a list of callback functions to lgb.train() etc. does not yet work properly because the "old" parameters early_stopping_rounds, eval_freq etc. override the callback list. How should we deal with this?

Thanks for pointing this out! Over in the Python package, we've recently removed these special keyword arguments to lgb.train() and lgb.cv() in favor of encouraging the use of callbacks (see #4574 and #4908).

I think the R package should do something similar. That would mean:

1. For now, raise deprecation warnings in lgb.train() and lgb.cv() if keyword argument early_stopping_rounds, eval_freq, etc. are non-NULL, and in those deprecation warnings encourage the use of the corresponding callbacks.
2. Add an issue to the backlog to remove those warnings and arguments after the release of LightGBM 4.0.

HOWEVER...we also should continue to support the case where parameters like early_stopping_rounds is passed via params.

In the Python package, the possibility of passing both params={"early_stopping_rounds": 10} and callbacks=[lgb.early_stopping(15)] isn't handled explicitly. If the parameter early_stopping_rounds is passed in params, an lgb.early_stopping() is added to the callbacks list.

LightGBM/python-package/lightgbm/engine.py

Lines 198 to 209 in f185695

	if "early_stopping_round" in params:
	callbacks_set.add(
	callback.early_stopping(
	stopping_rounds=params["early_stopping_round"],
	first_metric_only=first_metric_only,
	verbose=_choose_param_value(
	main_param_name="verbosity",
	params=params,
	default_value=1
	).pop("verbosity") > 0
	)
	)

I think that what will happen in those places is that both callbacks will be evaluated on each iteration, and early stopping will happen if either of them thinks early stopping has been met (I'd have to test that to be sure).

I think that in the R package, since we're just newly exporting the callbacks, we should be stricter and raise an error prior to training beginning if early_stopping_rounds from params and cb_early_stopping() are both provided.

The name and all arguments passed in are stored in the callback function's attributes...

LightGBM/R-package/R/callback.R

Lines 318 to 319 in f185695

	attr(callback, "call") <- match.call()
	attr(callback, "name") <- "cb.early.stop"

...so it should be possible to check whether or not any of the functions in callbacks have name "cb.early.stop" AND whether the values passed to that function conflict with early_stopping_rounds passed through params.

[mayer79]

@jameslamb Thanks a lot for your detailed feedback and instructions. I am a bit short in time but will start to change the callback functions as you proposed in a separate PR. It definitively makes sense to get rid of the dots in those function names.

[jameslamb]

Thanks very much @mayer79 !

Take your time. I don't think exposing callbacks is something that needs to be done urgently...this effort will go as fast as you are able to put effort into it, and can be done at any time.

[jameslamb]

@mayer79 are you still interested in working on this feature?

If not, then we'll close this pull request so that others (or you in the future!) will know when they see #2479 that it's available to work on.

[mayer79]

@jameslamb : I think we can close the PR for now, thanks for the remainder. We can pick up the ideas later.

[jameslamb]

Ok no problem. Thanks as always for your help with LightGBM!

[github-actions]

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.