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] make _InnerPredictor construction stricter #5961

Merged
merged 6 commits into from Jul 14, 2023

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jul 8, 2023

Proposes some changes to make the construction of an _InnerPredictor:

  • replacing constructor arguments booster_handle and model_file (and the corresponding if-else block) with class methods .from_booster() and .from_model_file()
  • removing Booster._to_predictor(), in favor of direct use of those class methods
  • avoiding initializing Booster._handle to None in Booster.__init__() (so that mypy can confirm that it's never possible for it to be None)
  • removing _InnnerPredictor.num_total_iteration, in favor of just calling _InnerPredictor.current_iteration() in the one place that was using _InnnerPredictor.num_total_iteration

These changes reduce unnecessary duplication, make it easier to understand the relationship between _InnerPredictor and Booster, and reduce the risk of "silently fell back to a default value" types of bugs by making these internal APIs stricter (no default values and mostly don't accept None).

Notes for Reviewers

Now that v4.0.0 is almost out (#5952), next I'm looking to spend some time on making it easier for outside contributors to contribute to LightGBM. This is the first in a series of PRs I'm planning to make LightGBM's internals stricter and easier to understand.

How I tested this

In addition to the unit tests, confirmed that this does not introduce any new errors from mypy by running the following on both master and this branch.

sh ./.ci/lint-python.sh

Comment on lines -3909 to +3956
if self._handle is not None:
_safe_call(_LIB.LGBM_BoosterFree(self._handle))
# ensure that existing Booster is freed before replacing it
# with a new one createdfrom file
_safe_call(_LIB.LGBM_BoosterFree(self._handle))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As of #5947, there is no path through lightgbm's public API which would allow a user to overwrite Booster._handle with None.

Making these changes to ensure it's never None avoids the need to do if {booster}._handle is not None types of checks in _InnerPredictor.from_booster().

@jameslamb jameslamb marked this pull request as ready for review July 8, 2023 05:36
@jameslamb jameslamb requested a review from guolinke July 8, 2023 05:36
python-package/lightgbm/basic.py Outdated Show resolved Hide resolved
python-package/lightgbm/basic.py Outdated Show resolved Hide resolved
Co-authored-by: José Morales <jmoralz92@gmail.com>
@jameslamb jameslamb requested a review from jmoralez July 12, 2023 16:09
@jameslamb jameslamb requested a review from jmoralez July 12, 2023 19:03
@jameslamb jameslamb merged commit 7d4d897 into master Jul 14, 2023
41 checks passed
@jameslamb jameslamb deleted the simplify-inner-predictor branch July 14, 2023 21:29
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants