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

Bugfix: multiprocessing queue doesn't pickle _size #107

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kinland
Copy link

@kinland kinland commented Nov 15, 2019

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

util/queue.py Outdated
@@ -66,7 +66,7 @@ class Queue(multiprocessing.queues.Queue):
"""

def __init__(self, *args, **kwargs):
super(Queue, self).__init__(*args, **kwargs)
super(Queue, self).__init__(*args, **kwargs, ctx=multiprocessing.get_context())
Copy link
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).

util/queue.py Outdated Show resolved Hide resolved
util/queue.py Show resolved Hide resolved
@vterron
Copy link
Owner

vterron commented Dec 1, 2019

Thanks for contributing this! Please see my comments.

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

kinland commented Dec 27, 2019

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

@@ -66,9 +71,20 @@ class Queue(multiprocessing.queues.Queue):
"""

def __init__(self, *args, **kwargs):
if sys.version_info >= (3, 4) and 'ctx' not in kwargs:
Copy link
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+).

@@ -51,6 +53,9 @@ def value(self):
return self.count.value


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

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

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

Copy link
Owner

@vterron vterron left a comment

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.

None yet

2 participants