[python-package] support sub-classing scikit-learn estimators

[jameslamb]

I recently saw a Stack Overflow post ("Why can't I wrap LGBM?") expressing the same concerns from #4426 ... it's difficult to sub-class lightgbm's scikit-learn estimators.

It doesn't have to be! Look how minimal the code is for XGBRFRegressor:

https://github.com/dmlc/xgboost/blob/45009413ce9f0d2bdfcd0c9ea8af1e71e3c0a191/python-package/xgboost/sklearn.py#L1869

This PR proposes borrowing some patterns I learned while working on xgboost's scikit-learn estimators to make it easier to sub-class lightgbm estimators. This also has the nice side effect of simplifying the lightgbm.dask code 😁

Notes for Reviewers

Why make the breaking change of requiring keyword args?

As part of this PR, I'm proposing immediately switching the constructors for scikit-learn estimators here (including those in lightgbm.dask) to only supporting keyword arguments.

Why I'm proposing this instead of a deprecation cycle:

• scikit-learn itself does this (HistGradientBoostingClassifier example)
  • so all of its machinery passing parameters around as keyword arguments
  • keyword arguments are recommended throughout https://scikit-learn.org/stable/developers/develop.html
• I strongly suspect that using positional arguments for these constructors is rare
• anyone relying on positional arguments will get a loud and easy-to-diagnose-and-fix error, so the effort to adjust should be minimal

import lightgbm as lgb
lgb.LGBMClassifier("gbdt")
# Traceback (most recent call last):
#   File "<stdin>", line 1, in <module>
# TypeError: LGBMClassifier.__init__() takes 1 positional argument but 2 were given

I posted a related answer to that Stack Overflow question

https://stackoverflow.com/a/79344862/3986677

[StrikerRUS]

Could you please setup an RTD build for this branch? I'd like to see how init signature will be rendered there.

[jameslamb]

Sure, here's a first build: https://readthedocs.org/projects/lightgbm/builds/26983170/

[StrikerRUS]

Great simplification, thanks for working on it!

I don't have any serious comments, just want to get some answers before approving.

[StrikerRUS]

Do you think it's OK to have just one client argument in the signature, but describe all parent args in the docstring?..

[jameslamb]

I think it's a little better for users to see all the parameters right here, instead of having to click over to another page.

This is what XGBoost is doing too: https://xgboost.readthedocs.io/en/stable/python/python_api.html#xgboost.XGBRFRegressor

But I do also appreciate that it could look confusing.

If we don't do it this way, then I'd recommend we add a link in the docs for `**kwargs`` in these estimators, like this:

**kwargs Other parameters for the model. These can be any of the keyword arguments for LGBMModel or any other LightGBM parameters documented at https://lightgbm.readthedocs.io/en/latest/Parameters.html.

I have a weak preference for keeping it as-is (the signature in docs not having all parameters, but docstring having all parameters), but happy to change it if you think that's confusing.

[StrikerRUS]

Thanks for clarifying your opinion!
I love your suggestion for **kwargs description. But my preference is also weak 🙂
I think we need a third judge opinion for this question.

Either way, I'm approving this PR!

[jameslamb]

@jmoralez or @borchero could one of you comment on this thread and help us break the tie?

To make progress on the release, if we don't hear back in the next 2 days I'll merge this PR as-is and we can come back and change the docs later.

[borchero]

Sorry, I only saw this now! My personal preference would actually be to keep all of the parameters (similar to the previous state) and simply make them keyword arguments. While this results in more code and some duplication of defaults, I think that this is the clearest interface for users. If you think this is undesirable @jameslamb, I'd at least opt for documenting all of the "transitive" parameters, just like in the XGBoost docs.

[borchero]

Going over the code again, I think the number of times the args are repeated, it's a very practical consideration to use **kwargs.

[jameslamb]

Thanks!

Haha so actually, I think your previous comment + @StrikerRUS 's question has convinced me that we should put all the keyword arguments back!

• reduces confusion in the docs or running help() interactively ("the clearest interface for users" like you said)
• we have a unit test to ensure that they all stay in sync
• it's ok for that to be slightly more development effort because:
  • adding/removing keyword arguments from the class constructors is not that common here
  • adding new classes inheriting from these estimators here is very rare

[jameslamb]

I'll make that change tomorrow. Put up WIP: in the title to indicate it's not ready to merge yet.

[jameslamb]

I just pushed a commit restoring all the keyword arguments to the Dask signatures. I also updated the test checking the signatures, including adding a check on the equality of .get_params().

That triggered this docs build as well: https://readthedocs.org/projects/lightgbm/builds/27106240/

@borchero @StrikerRUS when you have time, could you take another look?

[StrikerRUS]

Thank you very much!

[borchero]

Thanks!

[borchero]

Going over the code again, I think the number of times the args are repeated, it's a very practical consideration to use **kwargs.