[python-package] Add type hints to the callback file

[deddyjobson]

What I did

• Added type hints for callback.py as per [python-package] type hints in python package #3756.
• Verified that there were no additional errors when running mypy.

Note

• I ran into a problem from mypy where function attributes raise errors. I implemented a workaround as suggested in the discussion thread (Declare and use attributes of function objects python/mypy#2087).

[ghost]

All CLA requirements met.

[deddyjobson]

@jameslamb Can you please confirm if I need to resolve these failed tests or if they're not caused by my patch?

[jameslamb]

Some of the failures are unrelated to this PR's changes and should be fixed by merging the most recent master (#4095).

However, I think the check-docs failure is probably related to this pull request.

[autosummary] generating autosummary for: Python-API.rst

Warning, treated as error:
[autosummary] failed to import 'lightgbm.Booster': no module named lightgbm.Booster

That error from Sphinx is not very informative but it usually is caused by a legitimate underlying problem, for example #3192 (comment).

Can you please provide the specific mypy error(s) that this PR attempts to fix with CallbackWithAttributes?

[deddyjobson]

Thanks for the tips. Here is an example of a mypy error that CallbackWithAttributes resolves:

python-package\lightgbm\callback.py:86: error: "Callable[[CallbackEnv], None]" has no attribute "order"

I did import an additional package (typing_extensions) which I guess is what caused the problem. Removing the package and one associated type hint resulted in no errors.

[jameslamb]

Thanks for doing this @deddyjobson ! This one is tricky, really appreciate you working through it.

I left some initial comments, but I'd like @StrikerRUS to comment as well. I'm ok with the approach you've proposed of introducing a decorator to document the fact that we add attributes to the callback functions, I think that's a nice, non-invasive approach to improve the value we get from mypy.

[jameslamb]

Looks good to me! I can see in the lint job's logs that there are no mypy warnings for callback.py, and I think you've covered everything that needs a hint.

https://github.com/microsoft/LightGBM/pull/4093/checks?check_run_id=2207644984

Thanks very much! I won't merge until @StrikerRUS has a chance to look as well.

[StrikerRUS]

I'm afraid decorator will hurt performance as callback functions are called on each iteration.

[deddyjobson]

Removing the decorator results in 5 additional errors from mypy:

python-package\lightgbm\callback.py:71: error: "Callable[[CallbackEnv], None]" has no attribute "order"
python-package\lightgbm\callback.py:102: error: "Callable[[CallbackEnv], None]" has no attribute "order"
python-package\lightgbm\callback.py:142: error: "Callable[[CallbackEnv], None]" has no attribute "before_iteration"
python-package\lightgbm\callback.py:143: error: "Callable[[CallbackEnv], None]" has no attribute "order"
python-package\lightgbm\callback.py:240: error: "Callable[[CallbackEnv], None]" has no attribute "order"

[jameslamb]

Thanks! I agree with @StrikerRUS and I didn't think about the overhead before. In this first round of adding type hints to the library, I think it's desirable to only make changes that don't negatively affect the performance of lightgbm.

Can you please add # type: ignore comments for the lines you've mentioned in #4093 (comment)?

[deddyjobson]

Cool, didn't know I could do that.

[jameslamb]

Cool, didn't know I could do that.

oh, yep! This is super useful. Python is a really flexible language, so there are a lot of situations that are hard for mypy to understand. It's just a static analyzer and doesn't run your code.

By the way, I think there needs to be two spaces between the end of the line and the comment

x = something()  # type: ignore

[jameslamb]

Thanks very much for the changes! This looks good to me. I can see in https://github.com/microsoft/LightGBM/pull/4093/checks?check_run_id=2242197161 that there are no mypy warnings for callback.py and I agree with the choices of type hints you made.

[jameslamb]

We resolved a few blocking issues in continuous integration today, so I've updated this with the changes from the latest master. Will merge once it builds. Thanks again for the help!

[github-actions]

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.