Also, did anyone ask Davin directly to roll it back? On Sun, Feb 3, 2019 at 4:49 PM Guido van Rossum wrote:

I think this is now up to the 3.8 release manager.

On Sun, Feb 3, 2019 at 4:34 PM Terry Reedy wrote:

...

On 2/3/2019 4:03 PM, Antoine Pitrou wrote:

...

Hello,

I'd like to ask for the reversion of the changes done in https://github.com/python/cpython/pull/11664

The reason is simple: [over 1000 lines not reviewed, no tests, no docs]

Aside from the technical reasons Antoine gave, which I agree with, I think the merge was legally questionable, as a non-contributor is listed as a copyright holder. Message 334805. https://bugs.python.org/issue35813

-- Terry Jan Reedy

_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/guido%40python.org

-- --Guido van Rossum (python.org/~guido)

-- --Guido van Rossum (python.org/~guido)

On Feb 3, 2019, at 5:40 PM, Terry Reedy wrote:

On 2/3/2019 7:55 PM, Guido van Rossum wrote:

...

Also, did anyone ask Davin directly to roll it back?

Antoine posted on the issue, along with Robert O. Robert reviewed and make several suggestions.

I think the PR sat in a stable state for many months, and it looks like RO's review comments came *after* the commit. FWIW, with dataclasses we decided to get the PR committed early, long before most of the tests and all of the docs. The principle was that bigger changes needed to go in as early as possible in the release cycle so that we could thoroughly exercise it (something that almost never happens while something is in the PR stage). It would be great if the same came happen here. IIRC, shared memory has long been the holy grail for multiprocessing, helping to mitigate its principle disadvantage (the cost of moving data between processes). It's something we really want. But let's see what the 3.8 release manager has to say. Raymond

On 2/3/2019 7:55 PM, Guido van Rossum wrote:

Also, did anyone ask Davin directly to roll it back?

It was only recently that you edited his name out of the list of

Simply put: no. There have been a number of reactionary comments in the last 16 hours but no attempt to reach out to me directly during that time. On Sun, Feb 3, 2019 at 8:12 PM Raymond Hettinger < raymond.hettinger@gmail.com> wrote: maintainers for multiprocessing

even though that is what he's been working on for the last two years and at the last two sprints.

I think it would be best to discuss Antoine's decision to take this particular action without first consulting me, elsewhere and not part of this thread. As I said, I am happy to do the most constructive thing possible and I sought the advice of those I highly respect first before doing as I have. Davin On Sun, Feb 3, 2019 at 9:12 PM Davin Potts < python+python_dev@discontinuity.net> wrote:

I am attempting to do the right thing and am following the advice of other core devs in what I have done thus far.

Borrowing heavily from what I've added to issue35813 just now:

This work is the result of ~1.5 years of development effort, much of it accomplished at the last two core dev sprints. The code behind it has been stable since September 2018 and tested as an independently installable package by multiple people.

I was encouraged by Lukasz, Yury, and others to check in this code early, not waiting for tests and docs, in order to both solicit more feedback and provide for broader testing. I understand that doing such a thing is not at all a novelty. Thankfully it is doing that -- I hope that feedback remains constructive and supportive.

There are some tests to be found in a branch (enh-tests-shmem) of github.com/applio/cpython which I think should become more comprehensive before inclusion. Temporarily deferring and not including them as part of the first alpha should reduce the complexity of that release.

Regarding the BSD license on the C code being adopted, my conversations with Brett and subsequently Van have not raised concerns, far from it -- there is a process which is being followed to the letter. If there are other reasons to object to the thoughtful adoption of code licensed like this one, that deserves a decoupled and larger discussion first.

Davin

On Sun, Feb 3, 2019 at 8:12 PM Raymond Hettinger < raymond.hettinger@gmail.com> wrote:

...

...

On Feb 3, 2019, at 5:40 PM, Terry Reedy wrote:

On 2/3/2019 7:55 PM, Guido van Rossum wrote:

...

Also, did anyone ask Davin directly to roll it back?

Antoine posted on the issue, along with Robert O. Robert reviewed and make several suggestions.

I think the PR sat in a stable state for many months, and it looks like RO's review comments came *after* the commit.

FWIW, with dataclasses we decided to get the PR committed early, long before most of the tests and all of the docs. The principle was that bigger changes needed to go in as early as possible in the release cycle so that we could thoroughly exercise it (something that almost never happens while something is in the PR stage). It would be great if the same came happen here. IIRC, shared memory has long been the holy grail for multiprocessing, helping to mitigate its principle disadvantage (the cost of moving data between processes). It's something we really want.

But let's see what the 3.8 release manager has to say.

Raymond

_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/python%2Bpython_dev%40dis...

On Sun, 3 Feb 2019 21:25:27 -0600 Davin Potts wrote:

On 2/3/2019 7:55 PM, Guido van Rossum wrote:

...

Also, did anyone ask Davin directly to roll it back?

Simply put: no. There have been a number of reactionary comments in the last 16 hours but no attempt to reach out to me directly during that time.

By construction, if I post a comment on an issue you opened yourself on the bug tracker, you are receiving those comments. I'm not sure why a private message would be necessary. Generally, we refrain from doing things in private except if there are personal issues. Regards Antoine.

On Mon, Feb 4, 2019 at 4:21 AM Davin Potts < python+python_dev@discontinuity.net> wrote:

I am attempting to do the right thing and am following the advice of other core devs in what I have done thus far.

Borrowing heavily from what I've added to issue35813 just now:

This work is the result of ~1.5 years of development effort, much of it accomplished at the last two core dev sprints. The code behind it has been stable since September 2018 and tested as an independently installable package by multiple people.

I was encouraged by Lukasz, Yury, and others to check in this code early, not waiting for tests and docs, in order to both solicit more feedback and provide for broader testing. I understand that doing such a thing is not at all a novelty.

Actually it is a novelty (you should wait for review and approval). The main problem I have with this PR is that it seems to introduce 8 brand new APIs, but since there is no doc, docstrings or tests it's unclear which ones are supposed to be used, how or whether they are supposed to supersede or deprecate older (slower) ones involving inter process communication. The introduction of new APIs in the stdlib is a sensitive topic because once they get in they stay in, so a discussion should occur early on, definitively not at alphaX stage. Don't mean to point fingers here, the goal in itself (zero-copy, a topic I recently contributed to myself for the shutil module) is certainly valuable, but I concur and think this change should be reverted and post-poned for 3.9. -- Giampaolo - http://grodola.blogspot.com -- Giampaolo - http://grodola.blogspot.com

I wrote:

Could we somehow mark these APIs as experimental in 3.8?

It seems the change "e5ef45b8f519a9be9965590e1a0a587ff584c180" the one we are discussing. It adds two new files: Lib/multiprocessing/shared_memory.py Modules/_multiprocessing/posixshmem.c It doesn't introduce new C APIs. So, only multiprocessing.shared_memory seems public. I see we have PEP 411 that should cover this case: https://www.python.org/dev/peps/pep-0411/ The setup.py code could be more defensive. Maybe only build on platforms that have supported word sizes etc? For 3.8, could it be activated by uncommenting a line in Modules/Setup, rather than by setup.py? What happens in shared_memory if the _posixshmem module is not available? On Windows it seems like an import error is raised. Otherwise, _PosixSharedMemory becomes 'object'. Does that mean the API still works but you lose the zero-copy speed? Regards, Neil

On Feb 5, 2019, at 9:52 AM, Giampaolo Rodola' wrote:

The main problem I have with this PR is that it seems to introduce 8 brand new APIs, but since there is no doc, docstrings or tests it's unclear which ones are supposed to be used, how or whether they are supposed to supersede or deprecate older (slower) ones involving inter process communication.

The release manger already opined that if tests and docs get finished for the second alpha, he prefers not to have a reversion and would rather on build on top of what already shipped in the first alpha. FWIW, the absence of docs isn't desirable but it isn't atypical. PEP 572 code landed without the docs. Docs for dataclasses arrived much after the code. The same was true for the decimal module. Hopefully, everyone will team up with Davin and help him get the ball over the goal line. BTW, this is a feature we really want. Our multicore story for Python isn't a good one. Due to the GIL, threading usually can't exploit multiple cores for better performance. Async has lower overhead than threading but achieves its gains by keeping all the data in a single process. That leaves us with multiprocessing where the primary obstacle has been the heavy cost of moving data between processes. If that cost can be reduced, we've got a winning story for multicore. This patch is one of the better things that is happening to Python. Aside from last week's procedural missteps and communication issues surrounding the commit, the many months of prior work on this have been stellar. How about we stop using a highly public forum to pile up on Davin (being the subject of a thread like this can be a soul crushing experience). Right now, he could really use some help and support from everyone on the team. Raymond

Davin, I am not familiar with the multiprocessing module, so take the following with a big grain of salt. I took a look at the PR, then I got an idea of how multiprocessing module is organized by reading the doc. Here's some food for thought in terms of API reorganization. SharedMemoryManager, SharedMemoryServer --------------------------------------- It appears to me these are the 2 main public classes, and after reading the doc it seems they really belong to "managers https://docs.python.org/3/library/multiprocessing.html#multiprocessing-manag..." (multiprocessing.managers namespace). Also: * SharedMemoryManager is a subclass of multiprocessing.managers.SyncManager * SharedMemoryServer is a subclass of multiprocessing.managers.Server shared_memory.py could be renamed to _shared_memory.py and managers.py could import and expose these 2 classes only. Support APIs ------------ These are objects which seem to be used in support of the 2 classes above, but apparently are not meant to be public. As such they could simply live in _shared_memory.py and not be exposed: - shareable_wrap(): used only in SharedMemoryTracker.wrap() - SharedMemoryTracker: used only by SharedMemoryServer - SharedMemory, WindowsNamedSharedMemory, PosixSharedMemory: used by shareable_wrap() and SharedMemoryTracker - ShareableList: it appears this is not used, but by reading here https://github.com/python/cpython/blob/e0e5065daef36dafe10a46eaa8b7800274d73... I have a doubt: shouldn't it be register()ed against SharedMemoryManager? C extension module ------------------ - ExistentialError, Error - it appears these are not used - PermissionsException, ExistentialException - I concur with Ronald Oussoren's review: you could simply use PyErr_SetFromErrno() and let the original OSError exception bubble up. Same for O_CREAT, O_EXCL, O_CREX, O_TRUNC which are already exposed in the os module. I have a couple of other minor nitpicks re. the code but I will comment on the PR. Compatibility ------------- I'm not sure if SyncManager and SharedMemoryManager are fully interchangeable so I think the doc should clarify this. SyncManager handles a certain set of types https://github.com/python/cpython/blob/e0e5065daef36dafe10a46eaa8b7800274d73.... It appears SharedMemoryManager is supposedly able to do the same except for lists https://github.com/applio/cpython/blob/516cf4ac14af257913f46216973c09d58eb25.... Is my assumption correct? Also, multiprocessing.Manager() by default returns a SyncManager. If we'll get to a point where SyncManager and SharedMemoryManager are able to handle the same types it'd be good to return SharedMemoryManager as the default, but it's probably safer to leave it for later. Unless they are already there (I don't know) it would be good to have a full set of unit-tests for all the register()ed types and test them against SyncManager and SharedMemoryManager. That would give an idea on the real interchangeability of these 2 classes and would also help writing a comprehensive doc. Hope this helps. -- Giampaolo - http://grodola.blogspot.com

I have done what I was asked to do: I added tests and docs in a new PR (GH-11816) as of Feb 10. Since that time, the API has matured thanks to thoughtful feedback from a number of active reviewers. At present, we appear to have stabilized around an API and code that deserves to be exercised further. To get that exercise and because there are currently no outstanding objections, I am merging the PR to get it into the second alpha. There will undoubtedly be further revisions and adjustments. During this effort, I have received a surprising number of personal emails expressing support and encouragement from people, most of whom I have never met. Your kindness has been wonderful. Davin On Wed, Feb 6, 2019 at 10:58 AM Giampaolo Rodola' wrote:

On Wed, Feb 6, 2019 at 12:51 PM Giampaolo Rodola' wrote:

...

Unless they are already there (I don't know) it would be good to have a full set of unit-tests for all the register()ed types and test them against SyncManager and SharedMemoryManager. That would give an idea on the real interchangeability of these 2 classes and would also help writing a comprehensive doc.

In order to speed up the alpha2 inclusion process I created a PR which implements what said above: https://github.com/python/cpython/pull/11772 https://bugs.python.org/issue35917 Apparently SharedMemoryManager works out of the box and presents no differences with SyncManager, but the list type is not using ShareableList. When I attempted to register it with "SharedMemoryManager.register('list', list, ShareableList)" I got the following error:

Traceback (most recent call last): File "foo.py", line 137, in test_list o = self.manager.list() File "/home/giampaolo/svn/cpython/Lib/multiprocessing/managers.py", line 702, in temp proxy = proxytype( TypeError: __init__() got an unexpected keyword argument 'manager'

I am not sure how to fix that (I'll leave it up to Davin). The tests as-is are independent from PR-11772 so I suppose they can be reviewed/checked-in regardless of the changes which will affect shared_memory.py.

-- Giampaolo - http://grodola.blogspot.com

On Sat, 23 Feb 2019 22:09:03 -0600 Davin Potts wrote:

I have done what I was asked to do: I added tests and docs in a new PR (GH-11816) as of Feb 10.

Since that time, the API has matured thanks to thoughtful feedback from a number of active reviewers. At present, we appear to have stabilized around an API and code that deserves to be exercised further. To get that exercise and because there are currently no outstanding objections, I am merging the PR to get it into the second alpha. There will undoubtedly be further revisions and adjustments.

I agree the overall API looks reasonable. Thanks for doing this in time. Regards Antoine.

On 4 Feb 2019, at 03:10, Raymond Hettinger wrote:

...

On Feb 3, 2019, at 5:40 PM, Terry Reedy wrote:

On 2/3/2019 7:55 PM, Guido van Rossum wrote:

...

Also, did anyone ask Davin directly to roll it back?

Antoine posted on the issue, along with Robert O. Robert reviewed and make several suggestions.

@Terry: Robert is usually called Ronald :-)

I think the PR sat in a stable state for many months, and it looks like RO's review comments came *after* the commit.

That’s because I only noticed the PR after commit: The PR was merged within an hour of creating the BPO issue.

FWIW, with dataclasses we decided to get the PR committed early, long before most of the tests and all of the docs. The principle was that bigger changes needed to go in as early as possible in the release cycle so that we could thoroughly exercise it (something that almost never happens while something is in the PR stage). It would be great if the same came happen here. IIRC, shared memory has long been the holy grail for multiprocessing, helping to mitigate its principle disadvantage (the cost of moving data between processes). It's something we really want.

But with dataclasses there was public discussion on the API. This is a new API with no documentation in a part of the library that is known to be complex in nature. Ronald -- Twitter: @ronaldoussoren Blog: https://blog.ronaldoussoren.net/

On 4 Feb 2019, at 01:49, Guido van Rossum wrote:

I think this is now up to the 3.8 release manager.

I responded on the tracker: https://bugs.python.org/issue35813#msg334817 I wrote:

@Davin, in what time can you fill in the missing tests and documentation? If this is something you finish do before alpha2, I'm inclined to leave the change in.

As it stands, I missed the controversy yesterday as I was busy making my first release. So the merge *got released* in alpha1. I would prefer to fix the missing pieces forward instead of reverting and re-submitting which will only thrash blame and history at this point.

FTR, I do agree with Antoine, Ronald and others that in the future such big changes should be as close to their ready state at merge time.

@Raymond, would you be willing to work with Davin on finishing this work in time for alpha2? - Ł

On Feb 4, 2019, at 2:36 AM, Łukasz Langa wrote:

@Raymond, would you be willing to work with Davin on finishing this work in time for alpha2?

I would be happy to help, but this is beyond my technical ability. The people who are qualified to work on this have already chimed in on the discussion. Fortunately, I think this is a feature that everyone wants. So it just a matter of getting the experts on the subject to team-up and help get it done. Raymond

On Sun, 3 Feb 2019 17:52:55 -0800 Raymond Hettinger wrote:

...

On Feb 3, 2019, at 1:03 PM, Antoine Pitrou wrote:

I'd like to ask for the reversion of the changes done in https://github.com/python/cpython/pull/11664

Please work *with* Davin on this one.

You know, Raymond, I'm a volunteer and I dedicate my time to whatever I want. If someone pushes some unfinished work, it is perfectly normal to ask for reversion instead of feeling obliged to finish the work myself. Regards Antoine.