[python] make early_stopping callback pickleable

[Yard1]

Hey, I'm a maintainer of LightGBM-Ray. I was working on early stopping support for that library, however the early stopping callback on LightGBM's master branch raises exceptions in our tests (see test_linux_cutting_edge (3.7)).

I assumed this is due to the nonlocal variables present in the callback and them not interacting well with Ray serialization using cloudpickle. I would imagine there would be similar issues with other distributed computing or serialization libraries.

In order to solve that issue, I took the liberty of rewriting that callback into a callable class, with the nonlocal variables becoming attributes. This solved the issues with serialization. No functionality has been affected.

Please let me know if this approach is suitable, and if not, I would love to discuss other potential solutions to the problem I am facing.

[StrikerRUS]

@Yard1 Hey, thank you very much for the interest in LightGBM and this contribution!

As an early opinion I would say I support conversion complex functions with closures into classes. I think this will make callbacks usage more user friendly and LightGBM codebase simpler.

Just a note: we have received a feedback about the need in flexibility of callbacks serializability from Neptune maintainers recently #4719. Also, XGBoost migrated to callback classes: dmlc/xgboost#6199. So I believe this is the right direction.

[jameslamb]

I agree with @StrikerRUS , I definitely support the proposed direction. I'll mark this PR as "in-progress" for now. Please get the unit tests working again, and then we'll be able to provide a review. Let us know if you need help with the tests.

[jameslamb]

@Yard1 since you're a new contributor, we'll have to manually approve CI runs on each commit

This is something GitHub added last year (I think?) to combat abuse of GitHub Actions.

Sorry for the inconvenience!

[Yard1]

@jameslamb All's good, was on the approving side myself many times 😂

The tests pass locally for me now, so it should be good!

[Yard1]

@jameslamb @StrikerRUS The CI is all green now!

[jameslamb]

Thanks for this! The fact that tests are passing without needing any modification is a very good sign 🙌

Please see my first round of suggestions. I think it would be easier to review this PR if some of the style changes you've propose were reverted.

[Yard1]

@jameslamb I've applied your feedback, please take a look. Thanks!

[jmoralez]

Thank you for this! LGTM

[Yard1]

The tests are failing due to a scheduled Windows brownout in CI.

[jameslamb]

Great test! Thanks very much for adding that, and for moving the pickling / unpickling helper functions into utils.py.

Please see one question I have about related changes in callback.py.

[Yard1]

@jameslamb Had to merge with master, please rerun tests

[StrikerRUS]

Thank you very much for your work! Please consider checking some very minor suggestions below from me.

[StrikerRUS]

Awesome work! Thank you very much for this PR!

[Yard1]

@jameslamb Gentle ping :)

[Yard1]

Let me merge master again, maybe that will fix the CI

[Yard1]

@StrikerRUS @jameslamb CI is green now 🙏

[StrikerRUS]

@jameslamb Could you please take another look before merging?

[jameslamb]

Looks great!

Thanks so much for your work on this, and for adding tests. 🚀

[jameslamb]

Thanks again for this!

I changed the title to "make early_stopping callback pickleable" because PR titles in this project automatically become items in the release notes, and I think the fact that lgb.early_stopping is now pickleable will be more important to users than the particular mechanism used to achieve that.

I've also added #5080 documenting the work to ensure that all other callbacks are pickleable. Thanks for bringing the issue to our attention.

Good luck with lightgbm-ray, and we hope we'll see more contributions from you in the future 👋

[Yard1]

Thanks!

[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.