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

Allow concat_arrays to be pickable #8549

Merged
merged 2 commits into from
Mar 9, 2020

Conversation

emcastillo
Copy link
Member

This tries to fix a bug that happens when using MultiprocessParallelUpdater with forkserver or spawn as the start mode.

There is an error because the multiprocessing module tries to pickle the converter function and this one has lost its complete import route from the func_name or __name__ attribute due being decorated with converter.

Essentially I avoid the name classing in the module by not using the decorator.

I tried to solve this using functools.wrap but the case its trickier than it seems at first.

@emcastillo
Copy link
Member Author

emcastillo commented Feb 28, 2020

This is an snippet reproducing the issue

import pickle


class A:
    def __init__(self, func):
        self.func = func


def decorator():
    def wrap(func):
        return A(func)
    return wrap


@decorator()
def test():
    pass


class ThingWithBoundedFunc:
    def __init__(self, func=test):
        self.func = test


if __name__ == '__main__':
    obj = ThingWithBoundedFunc()
    pickle.dumps(obj)  # Fails
    pickle.dumps(test)  # Fails

either pickling obj or just the test function fails
seems like test is missing the import route on it's __name__ attribute when it is wrapped in the A class. Resulting in the following error:

_pickle.PicklingError: Can't pickle <function test at 0x7f7eabe32488>: it's not the same object as __main__.test

functools.wraps couldn't be used to solve this, as it is not a function wrapping a function but, an object.

Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

The current approach LGTM.

I tried to solve this using functools.wrap but the case its trickier than it seems at first.

Maybe because #7489 introduced the check if isinstance(converter, Converter).

if isinstance(converter, Converter):

@@ -106,6 +106,11 @@ def converter():
If the batch were requested to be converted to ChainerX with such
converters, :class:`RuntimeError` will be raised.

.. notes:: Converters using this decorator can't be pickled,
Copy link
Member

Choose a reason for hiding this comment

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

Could you fix this error?

Warning, treated as error:
/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/chainer/dataset/convert.py:docstring of chainer.dataset.converter:41:Unknown directive type "notes".

.. notes:: Converters using this decorator can't be pickled,

@emcastillo emcastillo force-pushed the pickable-converter branch 2 times, most recently from c1c3804 to cf70171 Compare March 2, 2020 06:04
@toslunar
Copy link
Member

toslunar commented Mar 2, 2020

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit cf70171, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member Author

Finally travis passed! Sorry about it

chainer/dataset/convert.py Outdated Show resolved Hide resolved
@emcastillo
Copy link
Member Author

PTAL, Thanks!!

@toslunar
Copy link
Member

toslunar commented Mar 3, 2020

Jenkins, test this please.

Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM.

@toslunar toslunar added cat:enhancement Implementation that does not break interfaces. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. labels Mar 3, 2020
@toslunar toslunar added this to the v7.3.0 milestone Mar 3, 2020
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 807cb5f, target branch master) succeeded!

@emcastillo
Copy link
Member Author

I think failures are not related. thanks

chainer/dataset/convert.py Outdated Show resolved Hide resolved
Co-Authored-By: Toshiki Kataoka <tos.lunar@gmail.com>
@toslunar
Copy link
Member

toslunar commented Mar 5, 2020

TravisCI: tests/chainermn_tests/extensions_tests/test_multi_node_snapshot.py failed on MNIST download (urllib.error.HTTPError: HTTP Error 403: Forbidden).
Seems unrelated to the PR.

@toslunar
Copy link
Member

toslunar commented Mar 5, 2020

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit c6a9a9c, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member Author

Same 403,
I can access the url from my local env though 😥

@chainer-ci
Copy link
Member

@toslunar This pull-request is marked as st:test-and-merge, but there were no activities for the last 3 days. Could you check?

@toslunar
Copy link
Member

toslunar commented Mar 9, 2020

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit c6a9a9c, target branch master) succeeded!

@mergify mergify bot merged commit 3726438 into chainer:master Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement Implementation that does not break interfaces. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants