Skip to content
This repository was archived by the owner on Oct 26, 2022. It is now read-only.

Bugfix: multiprocessing queue doesn't pickle _size - #107

Open
kinland wants to merge 4 commits into
vterron:masterfrom
kinland:bugfix_multiprocessing_pickle
Open

Bugfix: multiprocessing queue doesn't pickle _size#107
kinland wants to merge 4 commits into
vterron:masterfrom
kinland:bugfix_multiprocessing_pickle

Conversation

@kinland

@kinland kinland commented Nov 15, 2019

Copy link
Copy Markdown

I needed to make these small changes in order to get queue working with my multiprocessing code in python 3.6+.

There were two issues:

  1. multiprocessing.queues.Queue.__init__ takes a ctx argument, which was not provided
  2. if you are using spawn rather than fork with multiprocessing (e.g. running on Windows), _size doesn't get pickled, and get/put/etc will throw an AttributeError about there being no such attribute

Comment thread util/queue.py Outdated

def __init__(self, *args, **kwargs):
super(Queue, self).__init__(*args, **kwargs)
super(Queue, self).__init__(*args, **kwargs, ctx=multiprocessing.get_context())

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please list this before **kwargs, so that it also work with Python 2 (see the failing Travis CI test).

Comment thread util/queue.py Outdated
Comment thread util/queue.py
@vterron

vterron commented Dec 1, 2019

Copy link
Copy Markdown
Owner

Thanks for contributing this! Please see my comments.

- fix legacy python compatibility
- use namedtuple to pass state information
@kinland

kinland commented Dec 27, 2019

Copy link
Copy Markdown
Author

Sorry about the delay! I've made the requested changes. (Edit: and fixed the missing import. Sorry about that.)

Comment thread util/queue.py
"""

def __init__(self, *args, **kwargs):
if sys.version_info >= (3, 4) and 'ctx' not in kwargs:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please add a comment here explaining why this is necessary (i.e. why we need multiprocessing.get_context() in Python 3.4+).

Comment thread util/queue.py


QueueState = namedtuple('QueueState', ['queue', 'size'])

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's make this class non-public: _QueueState.

Comment thread util/queue.py
super(Queue, self).__init__(*args, **kwargs)
self._size = SharedCounter(0)

# __getstate__ and __setstate__ are needed for pickling, otherwise _size won't be copied.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's drop this comment and instead add docstrings to the two methods, e.g. "Returns the contents to pickle for the instance" and "Sets the state of the instance upon unpickling".

@vterron vterron left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks again! Left some more comments.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants