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: the PR isn't complete, it lacks docs and tests. It also didn't pass any review (this was pointed by Ronald), even though it adds 1300 lines of code. No programmer is perfect, so it's statistically likely that the PR is defective. With git, forks and branches, we definitely don't need to commit unfinished PRs to the main repo. It's perfectly fine to maintain some non-trivial piece of work in a separate fork. People do it on a regular basis (for example I have currently two such long-lived branches: one for PEP 556 and one for PEP 574). Also, this is not the first time this happened. Another multiprocessing PR was merged some years ago without any docs or tests: https://bugs.python.org/issue28053 Today that work /still/ lacks docs or tests, and there is a suspicion that it doesn't work as intended (see issue comments). It's probably too late to revert it, but it's definitely a slippery slope. Regards Antoine.
On Feb 3, 2019, at 13:03, Antoine Pitrou <solipsis@pitrou.net> wrote:
I'd like to ask for the reversion of the changes done in https://github.com/python/cpython/pull/11664
The reason is simple: the PR isn't complete, it lacks docs and tests. It also didn't pass any review (this was pointed by Ronald), even though it adds 1300 lines of code. No programmer is perfect, so it's statistically likely that the PR is defective.
I concur. I actually think CI shouldn’t even pass without sufficiently covering tests and docs (sans a “trivial” or other short circuiting label), but that might be unpopular. -Barry
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
I think this is now up to the 3.8 release manager. On Sun, Feb 3, 2019 at 4:34 PM Terry Reedy <tjreedy@udel.edu> 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)
Also, did anyone ask Davin directly to roll it back? On Sun, Feb 3, 2019 at 4:49 PM Guido van Rossum <guido@python.org> wrote:
I think this is now up to the 3.8 release manager.
On Sun, Feb 3, 2019 at 4:34 PM Terry Reedy <tjreedy@udel.edu> 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 <tjreedy@udel.edu> 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
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 <tjreedy@udel.edu> 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 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 <tjreedy@udel.edu> 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 4 Feb 2019, at 04:25, Davin Potts <python+python_dev@discontinuity.net> 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.
I asked a question about the commit yesterday night in the tracker and was waiting for a response (which I fully expected to take some time due to timezone differences and this being a volunteer driven project). Ronald
On Sun, 3 Feb 2019 21:25:27 -0600 Davin Potts <python+python_dev@discontinuity.net> 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 Sun, 3 Feb 2019 21:12:38 -0600 Davin Potts <python+python_dev@discontinuity.net> wrote:
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.
For the record: submitting a PR without tests or docs is perfectly fine, and a reasonable way to ask for feedback. Merging that PR is not, usually (especially as you didn't wait for feedback). So there might have been a misunderstanding between you and Lukasz, Yury and the "others". Or perhaps this is another instance of taking a disruptive decision in private... 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
On 2019-02-05, 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.
New or changed APIs are my major concern as well. Localized problems can be fixed later without much trouble. However, APIs "lock" us in and make it harder to change things later. Also, will new APIs need to be eventually supported by other Python implementations? I would imagine that doing zero-copy mixed with alternative garbage collection strategies could be complicated. Could we somehow mark these APIs as experimental in 3.8? My gut reaction is that we shouldn't revert. However, looking at the changes, it seems 'multiprocessing.shared_memory' could be an external extension package that lives in PyPI. It doesn't require changes to other interpreter internals. It doesn't seem to require internal Python header files. Regards, Neil
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 Wed, 6 Feb 2019 at 05:17, Neil Schemenauer <nas-python@arctrix.com> wrote:
My gut reaction is that we shouldn't revert. However, looking at the changes, it seems 'multiprocessing.shared_memory' could be an external extension package that lives in PyPI. It doesn't require changes to other interpreter internals. It doesn't seem to require internal Python header files.
The desired dependency in this case goes the other way: we'd like this in the standard library so that other standard library components can use it, and it can eventually become part of the "assumed baseline" that the reference Python interpreter offers to projects building on top of it. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Feb 5, 2019, at 9:52 AM, Giampaolo Rodola' <g.rodola@gmail.com> 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
On 02/05/2019 11:35 AM, Raymond Hettinger wrote:
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).
Thank you for the reminder.
Right now, he could really use some help and support from everyone on the team.
I am really looking forward to this enhancement. Thank you, Davin, and everyone else who has, and will, work on it. -- ~Ethan~
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
On Wed, Feb 6, 2019 at 12:51 PM Giampaolo Rodola' <g.rodola@gmail.com> 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
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' <g.rodola@gmail.com> wrote:
On Wed, Feb 6, 2019 at 12:51 PM Giampaolo Rodola' <g.rodola@gmail.com> 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 Sun, Feb 24, 2019 at 5:09 AM Davin Potts < python+python_dev@discontinuity.net> 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.
Nice job! It wasn't easy to abstract such a low level interface. -- Giampaolo - http://grodola.blogspot.com
On Sat, 23 Feb 2019 22:09:03 -0600 Davin Potts <python+python_dev@discontinuity.net> 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 Feb 3, 2019, at 18:10, Raymond Hettinger <raymond.hettinger@gmail.com> wrote:
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).
I think that should generally be the exception, but if it does happen, there ought to be a release blocker issue for the tests and docs. The problem then is if those things *don’t* happen and we get too late in the release cycle to roll the change back. -Barry
On 4 Feb 2019, at 03:10, Raymond Hettinger <raymond.hettinger@gmail.com> wrote:
On Feb 3, 2019, at 5:40 PM, Terry Reedy <tjreedy@udel.edu> 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 Sun, 3 Feb 2019 18:10:43 -0800 Raymond Hettinger <raymond.hettinger@gmail.com> wrote:
On Feb 3, 2019, at 5:40 PM, Terry Reedy <tjreedy@udel.edu> 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,
According to Github, it was opened 11 days ago. The first commit itself is 12 days old (again according to Github): https://github.com/python/cpython/pull/11664/commits/90f4a6cb2da8e187fa38b05... Now, perhaps the work itself is much older. But regardless, you cannot expect someone to take notice of a PR or issue if they are not put in CC. It is very much against our usual conventions to check in large pieces of code without asking anyone for review. Regards Antoine.
On 4 Feb 2019, at 01:49, Guido van Rossum <guido@python.org> 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? - Ł
The main problem here seems to be a shortage of communication. :/ Also, I agree on the exceptional nature of merging incomplete PRs. -eric On Mon, Feb 4, 2019 at 3:37 AM Łukasz Langa <lukasz@langa.pl> wrote:
On 4 Feb 2019, at 01:49, Guido van Rossum <guido@python.org> 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?
- Ł _______________________________________________ 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/ericsnowcurrently%40gmail...
On Feb 4, 2019, at 2:36 AM, Łukasz Langa <lukasz@langa.pl> 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 Feb 3, 2019, at 1:03 PM, Antoine Pitrou <solipsis@pitrou.net> 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. It was only recently that you edited his name out of the list of 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'd like to see more team work here rather than applying social pressures via python-dev (which is a *very* public list). Raymond
On Sun, 3 Feb 2019 17:52:55 -0800 Raymond Hettinger <raymond.hettinger@gmail.com> wrote:
On Feb 3, 2019, at 1:03 PM, Antoine Pitrou <solipsis@pitrou.net> 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.
participants (13)
-
Antoine Pitrou -
Barry Warsaw -
Davin Potts -
Eric Snow -
Ethan Furman -
Giampaolo Rodola' -
Guido van Rossum -
Neil Schemenauer -
Nick Coghlan -
Raymond Hettinger -
Ronald Oussoren -
Terry Reedy -
Łukasz Langa