Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[python-package] reorganize early stopping callback #6114

Merged
merged 8 commits into from Oct 5, 2023

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Sep 27, 2023

Contributes to #3756.
Contributes to #3867.

Fixes the following errors from mypy:

callback.py:232: error: Too few arguments  [call-arg]
callback.py:308: error: Argument "train_name" to "_is_train_set" of "_EarlyStoppingCallback" has incompatible type "str | Callable[[Any, Any], list[Any]]"; expected "str"  [arg-type]
callback.py:398: error: Argument 3 to "_is_train_set" of "_EarlyStoppingCallback" has incompatible type "str | Callable[[Any, Any], list[Any]]"; expected "str"  [arg-type]

Those all come from the fact that when env.model in CallbackEnv is a CVBooster, any attributes not explicitly defined on the CVBooster class return a method that is called on each of the Booster objects in .boosters

def __getattr__(self, name: str) -> Callable[[Any, Any], List[Any]]:
"""Redirect methods call of CVBooster."""
def handler_function(*args: Any, **kwargs: Any) -> List[Any]:
"""Call methods with each booster, and concatenate their results."""
ret = []
for booster in self.boosters:
ret.append(getattr(booster, name)(*args, **kwargs))
return ret
return handler_function

For example:

import lightgbm as lgb
from sklearn.datasets import make_regression

X, y = make_regression(n_samples=10_000)
dtrain = lgb.Dataset(X, y)
results = lgb.cv(
    train_set=dtrain,
    params={"objective": "regression"},
    num_boost_round=7,
    nfold=3,
    stratified=False,
    return_cvbooster=True
)

cv_booster = results["cvbooster"]

cv_booster.num_trees()
# [7, 7, 7]

mypy is rightly complaining that given that, it isn't safe to treat env.model._train_data_name as if it was a string... since for a CVBooster, it won't be.

cv_booster._train_data_name
# <function CVBooster.__getattr__.<locals>.handler_function at 0x120c11bc0>

This PR proposes some changes to avoid those cases in the early stopping callback.

It also proposes some other changes to make the code in that callback a bit easier to understand (for example, calling _EarlyStoppingCallback._is_train_set() with keyword arguments).

@jameslamb jameslamb changed the title WIP: [python-package] reorganize early stopping callback [python-package] reorganize early stopping callback Sep 27, 2023
@jameslamb jameslamb marked this pull request as ready for review September 27, 2023 04:20
python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
Comment on lines 319 to 320
if self.stopping_rounds <= 0:
raise ValueError(f"stopping_rounds should be greater than zero. got: {self.stopping_rounds}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this doesn't need the env we could move it to __init__, WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! Having it be a loud error right when the callback is created, instead of deferred all the way til the first iteration of training, seems useful. And I'd be surprised to learn that there are other libraries or user code depending on initializing lgb.early_stopping() with a negative value of this and then somehow updating the value before the first time it's called.

Moved into __init__() in bd3366a.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this broke this test:

def test_train_raises_informative_error_for_params_of_wrong_type():
X, y = make_synthetic_regression()
params = {"early_stopping_round": "too-many"}
dtrain = lgb.Dataset(X, label=y)
with pytest.raises(lgb.basic.LightGBMError, match="Parameter early_stopping_round should be of type int, got \"too-many\""):
lgb.train(params, dtrain)

Now the error from the early stopping callback gets thrown before this one from the C++ side:

Log::Fatal("Parameter %s should be of type int, got \"%s\"", key.c_str(), candidate);

So I pushed 7a98d82, which:

  • switches that test to use a different parameter, to keep covering that C++-side validation
  • adds a test in test_callback.py on this specific error from lgb.early_stopping()
  • adds an isinstance() check in the condition guarding that error in lgb.early_stopping(), so you can an informative error instead of something like TypeError: '<=' not supported between instances of 'str' and 'int'

Given all those changes, @jmoralez could you re-review? I don't want to sneak those in on your previous approval.

python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
lgb.early_stopping(stopping_rounds=-1)

with pytest.raises(ValueError, match="stopping_rounds should be an integer and greater than 0. got: neverrrr"):
lgb.early_stopping(stopping_rounds="neverrrr")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha thank you, thank you 😂

@jameslamb jameslamb merged commit d45dca7 into master Oct 5, 2023
41 checks passed
@jameslamb jameslamb deleted the python/mypy-early-stopping branch October 5, 2023 17:45
Ten0 pushed a commit to Ten0/LightGBM that referenced this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants