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] make early_stopping callback pickleable #5012

Merged
merged 25 commits into from
Mar 17, 2022

Conversation

Yard1
Copy link
Contributor

@Yard1 Yard1 commented Feb 16, 2022

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
Copy link
Collaborator

StrikerRUS commented Feb 16, 2022

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

@StrikerRUS StrikerRUS requested a review from jmoralez February 16, 2022 23:11
@jameslamb
Copy link
Collaborator

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 jameslamb changed the title [python] Turn early_stopping into a Callable class WIP: [python] Turn early_stopping into a Callable class Feb 17, 2022
@jameslamb
Copy link
Collaborator

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

image

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

Sorry for the inconvenience!

@Yard1
Copy link
Contributor Author

Yard1 commented Feb 17, 2022

@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
Copy link
Contributor Author

Yard1 commented Feb 17, 2022

@jameslamb @StrikerRUS The CI is all green now!

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

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.

python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
@Yard1 Yard1 requested a review from jameslamb February 18, 2022 11:58
@Yard1 Yard1 changed the title WIP: [python] Turn early_stopping into a Callable class [python] Turn early_stopping into a Callable class Feb 18, 2022
@Yard1
Copy link
Contributor Author

Yard1 commented Feb 22, 2022

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

Copy link
Collaborator

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

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

Thank you for this! LGTM

@Yard1
Copy link
Contributor Author

Yard1 commented Feb 24, 2022

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

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

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.

python-package/lightgbm/callback.py Show resolved Hide resolved
@Yard1
Copy link
Contributor Author

Yard1 commented Mar 2, 2022

@jameslamb Had to merge with master, please rerun tests

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

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

python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
@Yard1 Yard1 requested review from StrikerRUS and jameslamb March 3, 2022 17:56
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Awesome work! Thank you very much for this PR!

@Yard1
Copy link
Contributor Author

Yard1 commented Mar 7, 2022

@jameslamb Gentle ping :)

@Yard1
Copy link
Contributor Author

Yard1 commented Mar 12, 2022

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

@Yard1
Copy link
Contributor Author

Yard1 commented Mar 15, 2022

@StrikerRUS @jameslamb CI is green now 🙏

@StrikerRUS
Copy link
Collaborator

@jameslamb Could you please take another look before merging?

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Looks great!

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

@jameslamb jameslamb changed the title [python] Turn early_stopping into a Callable class [python] make early_stopping callback pickleable Mar 17, 2022
@jameslamb jameslamb merged commit f77e0ad into microsoft:master Mar 17, 2022
@jameslamb
Copy link
Collaborator

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
Copy link
Contributor Author

Yard1 commented Mar 17, 2022

Thanks!

@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@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 Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants