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

Update multiprocessing.managers.ListProxy and multiprocessing.managers.DictProxy #103134

Closed
invisibleroads opened this issue Mar 30, 2023 · 7 comments
Labels
3.13 bugs and security fixes topic-multiprocessing type-feature A feature request or enhancement

Comments

@invisibleroads
Copy link
Contributor

invisibleroads commented Mar 30, 2023

Feature or enhancement

Standard lists accept clear as a shortcut for del xs[:]

xs = ['a']
xs.clear()

However, multiprocessing.ListProxy omitted support for clear().

from multiprocessing import Manager

with Manager() as manager:
    xs = manager.list()
    xs.clear()

raises the following exception

AttributeError: 'ListProxy' object has no attribute 'clear'

Pitch

If clear is supported in a standard list, it should be supported in a multiprocessing list!

Previous discussion

This issue was not previously discussed.

Linked PRs

@terryjreedy
Copy link
Member

terryjreedy commented Mar 30, 2023

ListProxy was written before list.clear and list.copy were added in 3.3. Since manager.list has all the other list methods, it seems straightforward to add the new methods.

@terryjreedy
Copy link
Member

Dicts got reversed in 3.8 and | and |= in 3.9. I think we should expand this to updating all proxy types.

invisibleroads added a commit to invisibleroads/cpython that referenced this issue Mar 31, 2023
```
from multiprocessing import Manager
with Manager() as manager:
    xs = manager.list()
    xs.clear
    xs.copy
    d = manager.dict()
    d | {}  # __or__
    {} | d  # __ror__
    reversed(d)  # __reversed__
    d.fromkeys
```

suggested by @terryjreedy
tested manually in Python 3.10.8

python#103134
@invisibleroads
Copy link
Contributor Author

invisibleroads commented Mar 31, 2023

Updated the pull request to add xs.copy and d.fromkeys, d | {}, {} | d, reversed(d), d |= {} as you recommended.

Waiting to hear back from @benjaminp to see whether it is safe to replace BaseListProxy with ListProxy and just bring __iadd__ and __imul__ directly as you suggested.

Also, is there a place to put unit tests? I guess to test this properly, we would need to launch two processes and check that changes in one process are reflected in changes in the other process... Not sure how we would write a unit test for that, although we can certainly test it manually.

@invisibleroads invisibleroads changed the title Update ListProxy to add support for multiprocessing.Manager().list().clear() Update multiprocessing.managers.ListProxy and multiprocessing.managers.DictProxy Mar 31, 2023
@invisibleroads
Copy link
Contributor Author

invisibleroads commented Mar 31, 2023

As an update, I did try removing BaseListProxy and adding __iadd__ and __imul__ directly to ListProxy but it didn't work. I'm not sure why. Here is the code that I used to test this functionality manually.

from multiprocessing import Manager, Process

def f(x):
    x += [1]
    x *= 2
    print(x)

def g(d):
    d |= {'b': 2}
    print(d)

with Manager() as manager:

    x = manager.list()
    x.append(0)
    p = Process(target=f, args=(x,))
    p.start()
    p.join()
    q = Process(target=print, args=(x,))
    q.start()
    q.join()

    d = manager.dict()
    d['a'] = 1
    p = Process(target=g, args=(d,))
    p.start()
    p.join()
    q = Process(target=print, args=(d,))
    q.start()
    q.join()

RESULTS

ACTUAL after replacing BaseListProxy with ListProxy (the __imul__ doesn't seem to work).
ACTUAL when DictProxy is missing __ior__

[0, 1, 0, 1]
[0, 1]
{'a': 1, 'b': 2}
{'a': 1}

EXPECTED and ACTUAL with current commits for this pull request (keep BaseListProxy and bring DictProxy.__ior__) -- works with current pull request

[0, 1, 0, 1]
[0, 1, 0, 1]
{'a': 1, 'b': 2}
{'a': 1, 'b': 2}

@sunmy2019
Copy link
Member

sunmy2019 commented Apr 2, 2023

RESULTS

ACTUAL after replacing BaseListProxy with ListProxy (the __imul__ doesn't seem to work). ACTUAL when DictProxy is missing __ior__

[0, 1, 0, 1]
[0, 1]
{'a': 1, 'b': 2}
{'a': 1}

EXPECTED and ACTUAL with current commits for this pull request (keep BaseListProxy and bring DictProxy.__ior__) -- works with current pull request

[0, 1, 0, 1]
[0, 1, 0, 1]
{'a': 1, 'b': 2}
{'a': 1, 'b': 2}

Digging for one hour, I found it actually pretty simple.

Try this one out,

def f(x):
    x += [1]
    print(type(x))
    x *= 2

When you find x has type list but not <class 'multiprocessing.managers.ListProxy'>, you know you screwed up.

class ListProxy(BaseListProxy):
    def __iadd__(self, value):
        self._callmethod('extend', (value,))
        return self
    def __imul__(self, value):
        self._callmethod('__imul__', (value,))
        return self

This return self plays key role here. I understand the whole thing after seeing this.

def _callmethod(self, methodname, args=(), kwds={}):
'''
Try to call a method of the referent and return a copy of the result
'''

invisibleroads added a commit to invisibleroads/cpython that referenced this issue Apr 30, 2023
@invisibleroads
Copy link
Contributor Author

invisibleroads commented Apr 30, 2023

When you find x has type list but not <class 'multiprocessing.managers.ListProxy'>, you know you screwed up.

def f(x):
    x += [1]
    print(type(x))
    x *= 2

@sunmy2019 thanks so much for the important hint. I was able to make obj |= {'bar': 6} work properly after overriding __ior__ using the same technique used for ListProxy.

BaseDictProxy = MakeProxyType('DictProxy', (
    '__contains__', '__delitem__', '__getitem__', '__ior__', '__iter__',
    '__len__', '__or__', '__reversed__', '__ror__', '__setitem__', 'clear',
    'copy', 'fromkeys', 'get', 'items', 'keys', 'pop', 'popitem',
    'setdefault', 'update', 'values',
    ))
class DictProxy(BaseDictProxy):
    _method_to_typeid_ = {
        '__iter__': 'Iterator',
    }
    def __ior__(self, value):
        self._callmethod('__ior__', (value,))
        return self

I finished adding the tests and the pull request is ready for review.

@gpshead
Copy link
Member

gpshead commented May 20, 2024

thanks for taking this on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes topic-multiprocessing type-feature A feature request or enhancement
Projects
Development

No branches or pull requests

6 participants