[R-package] Disabled early stopping when using 'dart' boosting strategy

[jameslamb]

See discussion in #1893 for background. In this PR, I propose disabling early stopping when the user chooses 'dart' boosting. The same change was made in the Python package in #1895 .

This PR also adds the additional early stopping alias that was added in #2431 .

While working on this piece of code, I refactored it to make it a bit more obvious what is happening. I found that the same code is copied in lgb.cv() and lgb.train(). I think this duplication could lead to inconsistencies in the future, but I considered reducing that duplication out of scope.

[StrikerRUS]

@jameslamb Please see this my comment.

[jameslamb]

@jameslamb Please see this my comment.

@StrikerRUS apologies, I actually meant to address that comment in PR description. What do you think should be done? Since users are just passing in functions to callbacks, I don't know how to tell whether those functions are for early stopping or some other purpose.

Are you just suggesting that I invoke this warning WHENEVER someone uses dart boosting, so that people who passed early stopping custom callbacks will know?

[StrikerRUS]

@jameslamb In my comment from the original issue I meant that this fix covers the case when user passes boosting = 'dart' and early_stopping_rounds = x. However, it seems that the fix doesn't cover the case when user passes boosting = 'dart' and callbacks = list(cb.early.stop(x)). Am I wrong? The Python-package fix covers the both cases. I think it's possible just to mimic the Python code fix or

Since users are just passing in functions to callbacks, I don't know how to tell whether those functions are for early stopping or some other purpose.

LightGBM/R-package/R/callback.R

Line 386 in 1f88721

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

maybe somehow use this information?

[Laurae2]

@jameslamb @StrikerRUS From what I read, is it any(unlist(lapply(callbacks, function(x) {attr(x, "name")})) == "cb.early.stop") which should be also added as a condition?

[StrikerRUS]

@jameslamb @Laurae2 Maybe simply move the check into the callback function as it was done for Python-package? https://github.com/microsoft/LightGBM/pull/1895/files

[jameslamb]

@jameslamb @StrikerRUS From what I read, is it any(unlist(lapply(callbacks, function(x) {attr(x, "name")})) == "cb.early.stop") which should be also added as a condition?

to your @StrikerRUS 's comment...yes I could add that and am happy to, but cb.early.stop() is not exported from the package (https://github.com/microsoft/LightGBM/blob/master/R-package/NAMESPACE). So we can add that check (or something similar) to 100% prevent "any early stopping callbacks that are part of the LightGBM package", but I don't think there is anything we can do about the case where some user writes their own totally-custom thing and passing it to callbacks. I think that's fine, just want to clarify the difference.

As far as pushing the logic down into callback() like #1895 , I personally could not figure out how to do that in the R package. The structure of callback.R is quite a bit different from callback.py, as far as I can tell. Let me take another look at cb.early.stop() and see if I can mimic the behavior.

[StrikerRUS]

@jameslamb

... but cb.early.stop() is not exported from the package

Ah, I didn't know that!
So, I added known issue 6 in #1143 (comment) by a mistake, right?

but I don't think there is anything we can do about the case where some user writes their own totally-custom thing and passing it to callbacks.

I don't think that someone would write a custom callback for early stopping, since we already have early stopping callback. Even if one will, they will use our implementation as a reference and will see the prohibition of using early stopping with the dart mode.

[jameslamb]

@jameslamb

... but cb.early.stop() is not exported from the package

Ah, I didn't know that!
So, I added known issue 6 in #1143 (comment) by a mistake, right?

but I don't think there is anything we can do about the case where some user writes their own totally-custom thing and passing it to callbacks.

I don't think that someone would write a custom callback for early stopping, since we already have early stopping callback. Even if one will, they will use our implementation as a reference and will see the prohibition of using early stopping with the dart mode.

I don't think that known issue was a mistake, maybe you just meant something different than what I thought. We do have a keyword argument callbacks in lgb.train() and lgb.cv(), so I assumed your comment was about more thoroughly documenting the expect form of functions passed to that argument. Right now the documentation just says "A list of callback functions to evaluate at each iteration"

So our approach right now is to say "you can enable early stopping by passing early_stopping_rounds and providing a validation set to valids", not "add function cb.early_stop() to callbacks argument". callbacks is a generic mechanism the R package uses for "any code you want to run after each iteration". By the way this is the closest I've looked at this section of the code in a while so @Laurae2 may correct me, but I think that is how it works.

I think for now it would be sufficient to add the check using attr() to see if cb.early.stop has added to the list in callbacks.

[StrikerRUS]

@jameslamb Nope, I thought right about documenting functions in callback.R, because I was sure that users are encouraged take them and pass to callbacks argument.

[jameslamb]

@jameslamb Nope, I thought right about documenting functions in callback.R, because I was sure that users are encouraged take them and pass to callbacks argument.

Ok then yes, I misinterpreted what you meant. Documenting would also mean exporting. This is one of those things where R and Python are really different...in R you have to explicitly declare things as exported to make them usable. I'll update the language in #1143 to say "export callback functions" if that's alright.

[jameslamb]

Ok @Laurae2 @StrikerRUS whenever you have time, could you take another look? I've added protection against cb.early.stop() being passed directly via callbacks parameter.

I still left this in lgb.train() and lgb.cv() because I couldn't figure out how to do it all the way down in callback() like in the Python package.

[StrikerRUS]

@jameslamb

I'll update the language in #1143 to say "export callback functions" if that's alright.

I think that then this is another issue, even more feature request. But anyway, it's not related to pkgdown issue. I'll remove it from that list. Can you please create a separate issue for exporting callback functions?

[jameslamb]

@jameslamb

I'll update the language in #1143 to say "export callback functions" if that's alright.

I think that then this is another issue, even more feature request. But anyway, it's not related to pkgdown issue. I'll remove it from that list. Can you please create a separate issue for exporting callback functions?

Ok makes sense. I've created #2479 and attached it to #2302

[StrikerRUS]

@jameslamb I'm sorry, but could you please explain what is wrong with the following try to mimic Python solution on [pseudo?]R? https://www.diffchecker.com/LylbJ8EG

[jameslamb]

@jameslamb I'm sorry, but could you please explain what is wrong with the following try to mimic Python solution on [pseudo?]R? https://www.diffchecker.com/LylbJ8EG

I have never seen diffchecker.com before, super cool!

In my opinion, this code proposed in your link is going to be difficult to debug and reason about.

It's relying on a flag called enabled being present one level up the call stack environment up (since it is not passed into init() and params being available an unknown number of levels up the call stack environments up (since it is not passed in to init() or to cb.early.stop().

So in my opinion while my proposal in this PR feels more tedious, it is also more obvious and easier to debug / change.

I made that choice partially because I just feel it is a better design for the R package, but also partially because I am not so familiar with all of the R code package code that I could know the full implications of doing something like you proposed.

I will defer to @Laurae2 (who understand the internals of the R package more thoroughly) though. If @Laurae2 prefers something like the way this was handled in the Python package and pseudocode in your in link here, I'll happily change the approach from this PR and push the logic down into cb.early.stop().

edit: fixed incorrect use of "call stack" in my original comment

[StrikerRUS]

@jameslamb Thanks a lot for your explanation!

[jameslamb]

I just rebased this to current master. ping @Laurae2 for a review

[jameslamb]

I just rebased this to latest master to catch up with changes over the last two weeks. It is ready for review.

[Laurae2]

@jameslamb As you pointed out there's a lot of things in common in lgb.train and lgb.cv which are still separated, you did the right things at the moment to duplicate the code.

Ideally in the future we should be able to merge all the duplicate code in one, but it requires rewriting the R package.

[StrikerRUS]

Left some minor suggestions.

[jameslamb]

I've rebased to master and implemented requested changes to handling of aliases.

[jameslamb]

Ok this has been rebased to master and I've fixed the n_trees vs. n_rounds thing

[StrikerRUS]

LGTM, except two actually unresolved comments about grammar from #2443 (review).

[jameslamb]

LGTM, except two actually unresolved comments about grammar from #2443 (review).

ah! ok fixing

[jameslamb]

Thank you for the reviews everyone! I am going to merge this once CI is done.

[jameslamb]

was getting this on check-docs task on Travis:

429: too many requests
http://www.ozone3d.net/gpu_caps_viewer/

Restarting that task after waiting a few minutes seemed to solve it, but something to keep an eye on I guess.

[StrikerRUS]

@jameslamb

Restarting that task after waiting a few minutes seemed to solve it, but something to keep an eye on I guess.

Yeah, I see this error very often. And docs job is needed to be restarted quite often, that is why we keep it only at Travis where each collaborator has enough rights to re-run it (#1497 (comment)).