Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-103134: Update multiprocessing.managers.ListProxy and multiprocessing.managers.DictProxy #103133
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
invisibleroads commented Mar 30, 2023 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
``` from multiprocessing import Manager with Manager() as manager: xs = manager.list() xs.clear() ``` For now, we can use the workaround `del xs[:]`
bedevere-bot commented Mar 30, 2023
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
ghost commented Mar 30, 2023 • edited by ghost
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by ghost
Uh oh!
There was an error while loading. Please reload this page.
terryjreedy left a comment
There was a problem hiding this 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?
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2023-03-30-18-19-53.gh-issue-103134.bHrn91.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Lib/multiprocessing/managers.py Outdated
| '__mul__', '__reversed__', '__rmul__', '__setitem__', | ||
| 'append', 'count', 'extend', 'index', 'insert', 'pop', 'remove', | ||
| 'append', 'clear', 'count', 'extend', 'index', 'insert', 'pop', 'remove', | ||
| 'reverse', 'sort', '__imul__' |
There was a problem hiding this comment.
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'?
There was a problem hiding this comment.
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 commented Mar 30, 2023
As mentioned on issue, DictProxy and maybe others need updating. |
``` 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 <[email protected]>
invisibleroads commented Mar 31, 2023
Added a few more bits to DictProxy to support the cases you mentioned, including After your comments, I'm tempted to replace |
invisibleroads commented Mar 31, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
As an update, I did try removing RESULTSACTUAL after replacing EXPECTED and ACTUAL with current commits for this pull request -- works with this pull request The pull request passes the above manual test. |
invisibleroads commented Mar 31, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 will try to add tests over the weekend |
invisibleroads commented Apr 30, 2023
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 commented May 20, 2023
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 left a comment
There was a problem hiding this 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
invisibleroads commented Apr 27, 2024
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? |
Check that the dictionary returned by dict_proxy.from_keys is a dict as defined in dict.from_keys as requested by @encukou
invisibleroads commented May 20, 2024
Thanks to @encukou for taking the time to review this during the PyCon 2024 sprints!! |
For now, we can use the workaround
del xs[:]