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 log_evaluation callback pickleable #5101

Merged
merged 2 commits into from
Mar 30, 2022
Merged

Conversation

StrikerRUS
Copy link
Collaborator

Contributes to #5080.

In master, test_log_evaluation_callback_is_picklable() test fails with

============================================================================== FAILURES ==============================================================================
_________________________________________________________ test_log_evaluation_callback_is_picklable[pickle] __________________________________________________________

serializer = 'pickle', tmp_path = WindowsPath('C:/Users/nekit/AppData/Local/Temp/pytest-of-nekit/pytest-26/test_log_evaluation_callback_i0')

    @pytest.mark.parametrize('serializer', ["pickle", "joblib", "cloudpickle"])
    def test_log_evaluation_callback_is_picklable(serializer, tmp_path):
        callback = lgb.log_evaluation(period=42)
        tmp_file = tmp_path / "log_evaluation.pkl"
>       pickle_obj(
            obj=callback,
            filepath=tmp_file,
            serializer=serializer
        )

test_callback.py:29:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

obj = <function log_evaluation.<locals>._callback at 0x000001E31C0E7C10>
filepath = WindowsPath('C:/Users/nekit/AppData/Local/Temp/pytest-of-nekit/pytest-26/test_log_evaluation_callback_i0/log_evaluation.pkl'), serializer = 'pickle'

    def pickle_obj(obj, filepath, serializer):
        if serializer == 'pickle':
            with open(filepath, 'wb') as f:
>               pickle.dump(obj, f)
E               AttributeError: Can't pickle local object 'log_evaluation.<locals>._callback'

utils.py:142: AttributeError
_________________________________________________________ test_log_evaluation_callback_is_picklable[joblib] __________________________________________________________

serializer = 'joblib', tmp_path = WindowsPath('C:/Users/nekit/AppData/Local/Temp/pytest-of-nekit/pytest-26/test_log_evaluation_callback_i1')

    @pytest.mark.parametrize('serializer', ["pickle", "joblib", "cloudpickle"])
    def test_log_evaluation_callback_is_picklable(serializer, tmp_path):
        callback = lgb.log_evaluation(period=42)
        tmp_file = tmp_path / "log_evaluation.pkl"
>       pickle_obj(
            obj=callback,
            filepath=tmp_file,
            serializer=serializer
        )

test_callback.py:29:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
utils.py:144: in pickle_obj
    joblib.dump(obj, filepath)
d:\miniconda3\lib\site-packages\joblib\numpy_pickle.py:480: in dump
    NumpyPickler(f, protocol=protocol).dump(value)
d:\miniconda3\lib\pickle.py:487: in dump
    self.save(obj)
d:\miniconda3\lib\site-packages\joblib\numpy_pickle.py:282: in save
    return Pickler.save(self, obj)
d:\miniconda3\lib\pickle.py:560: in save
    f(self, obj)  # Call unbound method with explicit self
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <joblib.numpy_pickle.NumpyPickler object at 0x000001E31C239490>, obj = <function log_evaluation.<locals>._callback at 0x000001E31C24E670>
name = 'log_evaluation.<locals>._callback'

    def save_global(self, obj, name=None):
        write = self.write
        memo = self.memo

        if name is None:
            name = getattr(obj, '__qualname__', None)
        if name is None:
            name = obj.__name__

        module_name = whichmodule(obj, name)
        try:
            __import__(module_name, level=0)
            module = sys.modules[module_name]
            obj2, parent = _getattribute(module, name)
        except (ImportError, KeyError, AttributeError):
>           raise PicklingError(
                "Can't pickle %r: it's not found as %s.%s" %
                (obj, module_name, name)) from None
E           _pickle.PicklingError: Can't pickle <function log_evaluation.<locals>._callback at 0x000001E31C24E670>: it's not found as lightgbm.callback.log_evaluation.<locals>._callback

d:\miniconda3\lib\pickle.py:1070: PicklingError

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 very much for starting work on this issue, I think it'll be great to have better compatibility with frameworks like Ray and Dask.

I just left two minor comments.

def __init__(self, period: int = 1, show_stdv: bool = True) -> None:
self.order = 10
self.before_iteration = False

Copy link
Collaborator

Choose a reason for hiding this comment

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

very very minor comment, but could this empty line be removed, to group all assignments to self.* properties?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe it would be better for code readability to split properties that are common for all callbacks (order and before_iteration) and specific properties for a particular callback. WDYT?
Refer to

self.order = 30
self.before_iteration = False
self.stopping_rounds = stopping_rounds
self.first_metric_only = first_metric_only
self.verbose = verbose
self.min_delta = min_delta

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah I see, that makes sense. Ok yeah I'm fine with that too! I don't feel strongly either way.

tests/python_package_test/test_callback.py Show resolved Hide resolved
@StrikerRUS StrikerRUS merged commit 8b33e77 into master Mar 30, 2022
@StrikerRUS StrikerRUS deleted the log_eval_callback branch March 30, 2022 18:52
@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.

2 participants