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] support saving and loading CVBooster (fixes #3556) #5160

Merged
merged 24 commits into from
Aug 16, 2022

Conversation

nyanp
Copy link
Contributor

@nyanp nyanp commented Apr 20, 2022

This PR resolves #3556 by implementing the following method in CVBooster:

  • save_model
  • model_from_string
  • model_to_string

@ghost
Copy link

ghost commented Apr 20, 2022

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ nyanp sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

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 very much for taking this on!

I will provide a more thorough review in the next few days, for now please see my initial round of suggestions.

Also note that this does not totally resolve #3556. As mentioned in the discussion there, it should also be possible to pickle a CVBooster and load one from a pickle file. I think that work should be picked up in a follow-up PR after this one.

python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
@nyanp
Copy link
Contributor Author

nyanp commented Apr 22, 2022

Also note that this does not totally resolve #3556. As mentioned in the discussion there, it should also be possible to pickle a CVBooster and load one from a pickle file. I think that work should be picked up in a follow-up PR after this one.

This can be solved by simply defining __getstate__ and __setstate__ in CVBooster, which I will try to solve together in this PR.

@nyanp nyanp requested a review from jameslamb April 25, 2022 10:40
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 the test changes! I'd originally suggested splitting pickling into a separate PR to keep this one small, but now that I see how small the changes are I think it's ok to leave all in this PR.

Please see a few more recommendations on the tests. I'll provide a more thorough review once I have a chance to run this code myself and inspect the outputs.

tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
@jameslamb jameslamb changed the title [python-package] support saving and loading CVBooster [python-package] support saving and loading CVBooster (fixes #3556) Apr 26, 2022
@nyanp nyanp force-pushed the save-cvboosters branch from d87aff5 to 3ec7fd0 Compare April 26, 2022 23:12
@nyanp
Copy link
Contributor Author

nyanp commented May 3, 2022

I signed the CLA and once the status was OK, but at some point, the bot status changed again to "not signed yet".
When I access the CLA page (https://cla.opensource.microsoft.com/microsoft/LightGBM?pullRequest=5160) again, it says "You have signed the CLA for multiple repositories or organizations" and I cannot re-sign.
Can anyone help me?

@StrikerRUS
Copy link
Collaborator

@nyanp Sorry about this inconvenience!
I believe we can "fix" it via closing-reopenning this PR.

@StrikerRUS StrikerRUS closed this May 3, 2022
@nyanp nyanp requested a review from jameslamb May 30, 2022 12:12
@nyanp
Copy link
Contributor Author

nyanp commented Jun 3, 2022

@jameslamb
Hi, could you please review it again? I believe all review points have been resolved. Thanks :)

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.

Thanks a lot for this PR!
Please consider checking some my comments below.

python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Show resolved Hide resolved
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 very much for this! I agree with all of @StrikerRUS 's suggestions, so please address those before we can merge this.

I have no further comments right now.

@StrikerRUS
Copy link
Collaborator

Kindly ping @nyanp

@jameslamb
Copy link
Collaborator

@nyanp are you interested in continuing this pull request? This is a really useful addition to the library and we're very grateful for your work so far. I'd love to see this completed.

@nyanp
Copy link
Contributor Author

nyanp commented Jul 18, 2022

@jameslamb
Sorry for the late reply, I have been off for a while. I will resume today!

@nyanp nyanp requested a review from jameslamb July 30, 2022 09:41
@nyanp
Copy link
Contributor Author

nyanp commented Jul 30, 2022

I believe all review content has now been resolved.
Please re-review @StrikerRUS @jameslamb

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.

Great job, thank you!

LGTM!

Just some minor comments below.

python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Show resolved Hide resolved
tests/python_package_test/test_engine.py Show resolved Hide resolved
@StrikerRUS StrikerRUS requested review from jameslamb and removed request for jameslamb August 14, 2022 17:08
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.

I just re-reviewed. All of my and other reviewers' comments have been addressed, and I don't have any other suggestions.

EXCELLENT work @nyanp 🎉 🎉 🎉

We really appreciate your hard work on this very impactful feature, and your patience with the review process. I hope you'll consider contributing to LightGBM more in the future.

@jameslamb jameslamb merged commit 4a9b08e into microsoft:master Aug 16, 2022
@nyanp
Copy link
Contributor Author

nyanp commented Aug 23, 2022

I just re-reviewed. All of my and other reviewers' comments have been addressed, and I don't have any other suggestions.

EXCELLENT work @nyanp 🎉 🎉 🎉

We really appreciate your hard work on this very impactful feature, and your patience with the review process. I hope you'll consider contributing to LightGBM more in the future.

@jameslamb
Thank you very much for the kind and polite review! I am very happy to contribute to my favorite library. I have enjoyed this long journey ;)

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

[python] support saving and loading CVBooster
4 participants