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

gh-103134: Update multiprocessing.managers.ListProxy and multiprocessing.managers.DictProxy #103133

Merged
merged 34 commits into from
May 20, 2024

Conversation

invisibleroads
Copy link
Contributor

@invisibleroads invisibleroads commented Mar 30, 2023

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

For now, we can use the workaround del xs[:]

```
from multiprocessing import Manager
with Manager() as manager:
    xs = manager.list()
    xs.clear()
```

For now, we can use the workaround `del xs[:]`
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Mar 30, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@arhadthedev arhadthedev added stdlib Python modules in the Lib dir topic-multiprocessing labels Mar 30, 2023
@invisibleroads invisibleroads changed the title Add support for manager.list().clear() gh-103134: Add support for manager.list().clear() Mar 30, 2023
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

Are there any tests for BaseList Proxy or derivatives that are not automatically expanded by expanding this list? And which should be manually augmented?

Lib/multiprocessing/managers.py Outdated Show resolved Hide resolved
@@ -1148,7 +1148,7 @@ def set(self, value):
BaseListProxy = MakeProxyType('BaseListProxy', (
'__add__', '__contains__', '__delitem__', '__getitem__', '__len__',
'__mul__', '__reversed__', '__rmul__', '__setitem__',
'append', 'count', 'extend', 'index', 'insert', 'pop', 'remove',
'append', 'clear', 'count', 'extend', 'index', 'insert', 'pop', 'remove',
'reverse', 'sort', '__imul__'
Copy link
Member

Choose a reason for hiding this comment

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

Given the definition of method __iadd__ in the definition of ListProxy below, it would seem that it should be in the list above. On the other hand, adding xs += [2] to your example works as is. Maybe '_imul' does not need to be listed. Does it hurt to list 'iadd'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like ListProxy.__iadd__ uses BaseListProxy.extend and ListProxy.__imul__ calls BaseListProxy.__imul__. I'm not sure of the reasoning behind why ListProxy defines __iadd__ and __imul__ separately instead of including them directly in BaseListProxy. Testing xs += [2] and xs *= 2 works when tested manually.

I'm afraid of replacing BaseListProxy with ListProxy for fear of breaking something. Looking at git blame, it looks like this part of the code was last changed 15 years ago? @benjaminp would it be safe to replace BaseListProxy with ListProxy and just include __iadd__ and __imul__?

@terryjreedy
Copy link
Member

As mentioned on issue, DictProxy and maybe others need updating.

invisibleroads and others added 3 commits March 31, 2023 01:15
```
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
…Hrn91.rst

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
@invisibleroads
Copy link
Contributor Author

As mentioned on issue, DictProxy and maybe others need updating.

Added a few more bits to DictProxy to support the cases you mentioned, including fromkeys, d | {}, {} | d, reversed(d).

After your comments, I'm tempted to replace BaseListProxy with ListProxy but would like to hear back from @benjaminp if @benjaminp remembers why BaseListProxy was defined separately.

@invisibleroads
Copy link
Contributor Author

invisibleroads commented Mar 31, 2023

As an update, I did try removing BaseListProxy and just adding __iadd__ and __imul__ directly to ListProxy and 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 -- works with this pull request

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

The pull request passes the above manual test.

@invisibleroads invisibleroads changed the title gh-103134: Add support for manager.list().clear() gh-103134: Update multiprocessing.managers.ListProxy and multiprocessing.managers.DictProxy Mar 31, 2023
@invisibleroads
Copy link
Contributor Author

invisibleroads commented Mar 31, 2023

We could write a test that uses a process to modify a manager.list or manager.list, then check that the list or dict has actually been modified.

I found the multiprocessing tests here
https://github.com/python/cpython/blob/main/Lib/test/_test_multiprocessing.py#L5703

will try to add tests over the weekend

@invisibleroads
Copy link
Contributor Author

This pull request is ready for review. Thanks to @arhadthedev for accepting this issue, @terryjreedy for the suggestions and @sunmy2019 for the very important hints.

@invisibleroads
Copy link
Contributor Author

@Yhg1s
Is there any chance we could get these multiprocessing manager updates into Python 3.12?
The code changes are relatively straightforward and add missing functionality to multiprocessing.managers.ListProxy and multiprocessing.managers.DictProxy.
Thanks for managing these releases.

@invisibleroads
Copy link
Contributor Author

This minor enhancement backports recently added list and dictionary functionality into multiprocessing.managers.ListProxy and multiprocessing.managers.DictProxy so that scripts that use multiprocessing.Manager can use the same list and dictionary functionality. Would it be possible for someone to review these proposed enhancements?

@kumaraditya303
@serhiy-storchaka
@pablogsal
@iritkatriel
@gvanrossum

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Please test the type and the value of results.

Lib/multiprocessing/managers.py Show resolved Hide resolved
Lib/test/_test_multiprocessing.py Show resolved Hide resolved
Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved
Lib/test/_test_multiprocessing.py Show resolved Hide resolved
Lib/multiprocessing/managers.py Show resolved Hide resolved
@invisibleroads
Copy link
Contributor Author

It's been almost a year, but I finally got around to making the changes requested by @serhiy-storchaka.

Would it be possible for someone to review these changes?
@pablogsal
@iritkatriel
@gvanrossum
@kumaraditya303

@invisibleroads
Copy link
Contributor Author

Thanks to @encukou for taking the time to review this during the PyCon 2024 sprints!!

@encukou encukou enabled auto-merge (squash) May 20, 2024 14:08
@encukou encukou merged commit bbb4988 into python:main May 20, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir topic-multiprocessing
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants