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] fix mypy errors related to eval result parsing in callbacks #6096

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Sep 13, 2023

Contributes to #3756.
Contributes to #3867.

Resolves the following errors from mypy:

python-package/lightgbm/callback.py:128: error: Item "None" of "Optional[List[Tuple[str, str, float, bool]]]" has no attribute "__iter__" (not iterable)  [union-attr]
python-package/lightgbm/callback.py:143: error: Item "None" of "Optional[List[Tuple[str, str, float, bool]]]" has no attribute "__iter__" (not iterable)  [union-attr]
python-package/lightgbm/callback.py:283: error: Argument 1 to "len" has incompatible type "Optional[List[Tuple[str, str, float, bool]]]"; expected "Sized"  [arg-type]
python-package/lightgbm/callback.py:285: error: Value of type "Optional[List[Tuple[str, str, float, bool]]]" is not indexable  [index]
python-package/lightgbm/callback.py:286: error: Value of type "Optional[List[Tuple[str, str, float, bool]]]" is not indexable  [index]
python-package/lightgbm/callback.py:362: error: Argument 1 to "len" has incompatible type "Optional[List[Tuple[str, str, float, bool]]]"; expected "Sized"  [arg-type]
python-package/lightgbm/callback.py:363: error: Value of type "Optional[List[Tuple[str, str, float, bool]]]" is not indexable  [index]
python-package/lightgbm/callback.py:368: error: Argument 1 to "append" of "list" has incompatible type "Optional[List[Tuple[str, str, float, bool]]]"; expected "Union[List[Tuple[str, str, float, bool]], List[Tuple[str, str, float, bool, float]]]"  [arg-type]
python-package/lightgbm/callback.py:370: error: Incompatible types in assignment (expression has type "Optional[List[Tuple[str, str, float, bool]]]", target has type "Union[List[Tuple[str, str, float, bool]], List[Tuple[str, str, float, bool, float]]]")  [assignment]
python-package/lightgbm/callback.py:372: error: Value of type "Optional[List[Tuple[str, str, float, bool]]]" is not indexable  [index]
python-package/lightgbm/callback.py:375: error: Value of type "Optional[List[Tuple[str, str, float, bool]]]" is not indexable  [index]

These are all mypy rightly identifying places in the callbacks code where CallbackEnv.evaluation_result_list is used unconditionally as if it contains a list...

def _init(self, env: CallbackEnv) -> None:
self.eval_result.clear()
for item in env.evaluation_result_list:

... even though it's possible for it to be None.

cb(callback.CallbackEnv(model=booster,
params=params,
iteration=i,
begin_iteration=init_iteration,
end_iteration=init_iteration + num_boost_round,
evaluation_result_list=None))

This PR proposes defensively raising an informative error if env.evaluation_result_list is still None when any of those codepaths are reached. These should never be encountered by user code, but if they are then they're replacing cryptic errors like TypeError: 'NoneType' object is not subscriptable with something clearer.

Why not just enforce that CallbackEnv.evaluation_result_list is always a list and never None?

I'm afraid that that would break other integrations with lightgbm.

evaluation_result_list has been nullable since the earliest days of the Python package, 7+ years ago: #97. I think taking on these errors messages is preferable to risking that.

Especially since I've learned tonight that other projects are relying on the callbacks mechanism to integration with lightgbmm, like:

@jameslamb jameslamb changed the title WIP: [python-package] fix mypy errors related to eval result parsing in callbacks [python-package] fix mypy errors related to eval result parsing in callbacks Sep 13, 2023
@jameslamb jameslamb marked this pull request as ready for review September 13, 2023 04:34
@jameslamb jameslamb merged commit 163416d into master Sep 13, 2023
39 checks passed
@jameslamb jameslamb deleted the python/callback-env-eval-list branch September 13, 2023 20:38
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 Dec 13, 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.

2 participants