bpo-39244: multiprocessing get all start methods #18625

idomic · 2020-02-23T15:07:49Z

https://bugs.python.org/issue39244

idomic · 2020-02-23T15:11:34Z

codecov · 2020-02-23T15:36:42Z

Codecov Report

Merging #18625 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18625      +/-   ##
==========================================
- Coverage   82.11%   82.06%   -0.05%     
==========================================
  Files        1956     1955       -1     
  Lines      589428   584089    -5339     
  Branches    44459    44461       +2     
==========================================
- Hits       484010   479356    -4654     
+ Misses      95762    95105     -657     
+ Partials     9656     9628      -28

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Modules/_decimal/libmpdec/umodarith.h	`80.76% <0.00%> (-19.24%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-7.15%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.77%)`	⬇️
... and 321 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b76518d...33db325. Read the comment docs.

idomic · 2020-03-01T17:25:04Z

Adding @taleinat , do I have to add tests for this?

idomic · 2020-03-15T14:14:34Z

ping @taleinat

taleinat · 2020-03-16T15:31:52Z

Lib/multiprocessing/context.py

        if sys.platform == 'win32':
            return ['spawn']
+        elif sys.platform == 'darwin':
+            return ['spawn', 'fork', 'forkserver']


Does this not require the if reduction.HAVE_SEND_HANDLE: check as below?

Moved the platform check to be included in the HAVE_SEND_HANDLE if block

taleinat · 2020-03-16T15:33:04Z

Adding @taleinat , do I have to add tests for this?

The test already included seems sufficient.

taleinat · 2020-03-16T15:33:30Z

ping @taleinat

Sorry, I'm extremely busy these days...

idomic · 2020-03-22T21:20:32Z

ping @taleinat

Sorry, I'm extremely busy these days...

That's alright, don't worry about it and stay safe!

…bpo-39244_multiprocessing

taleinat · 2020-04-24T20:58:13Z

Lib/multiprocessing/context.py

        else:
            if reduction.HAVE_SEND_HANDLE:
-                return ['fork', 'spawn', 'forkserver']
+                if sys.platform == 'darwin':


Are you certain reduction.HAVE_SEND_HANDLE will always be true on macOS? If not 100% this is true on older and future versions, then a similar change should be done in the else clause below.

taleinat · 2020-04-24T20:58:17Z

Lib/test/_test_multiprocessing.py

        if sys.platform == 'win32':
            self.assertEqual(methods, ['spawn'])
+        elif sys.platform == 'darwin':
+            self.assertEqual(methods, ['spawn', 'fork', 'forkserver'])


ISTM this should allow 'forkserver' to be missing, as done by the assertion two lines below.

Dropped this test, I agree it's redundant + fixed the code to have the return value in the else clause

bedevere-bot · 2020-04-24T20:58:30Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

idomic · 2020-05-24T23:07:07Z

I have made the requested changes; please review again

bedevere-bot · 2020-05-24T23:07:10Z

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

taleinat · 2020-05-25T06:59:24Z

Lib/multiprocessing/context.py

            return ['spawn']
        else:
+            if sys.platform == 'darwin':
+                return ['spawn', 'fork', 'forkserver']


This breaks the intention of the existing code: The purpose of the HAVE_SEND_HANDLE check is to decide whether to include 'forkserver' as an option. It should be done regardless of platform.

Perhaps just add a conditional after this code the brings 'spawn' to the beginning of the list on macOS.

taleinat · 2020-05-25T07:01:27Z

Misc/NEWS.d/next/Library/2020-02-23-15-09-47.bpo-39244.aBK5IM.rst

@@ -0,0 +1,2 @@
+Fixed :class:`multiprocessing.context.get_all_start_methods`
+If the context is macOS, return the default methods by order.


This wording is imprecise; this should always return the default method first, which was not done on the macOS platform.

bedevere-bot · 2020-05-25T07:01:37Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

taleinat · 2020-05-25T14:41:05Z

Lib/multiprocessing/context.py

            if reduction.HAVE_SEND_HANDLE:
                return ['fork', 'spawn', 'forkserver']
-            else:
-                return ['fork', 'spawn']
+            if sys.platform == 'darwin':
+                return ['spawn', 'fork', 'forkserver']


This still needs to return a list without 'forkserver' when HAVE_SEND_HANDLE is false.

Suggested change

if reduction.HAVE_SEND_HANDLE:

return ['fork', 'spawn', 'forkserver']

else:

return ['fork', 'spawn']

if sys.platform == 'darwin':

return ['spawn', 'fork', 'forkserver']

methods = ['spawn', 'fork'] if sys.platform == 'darwin' else ['fork', 'spawn']

if reduction.HAVE_SEND_HANDLE:

methods.append('forkserver')

return methods

taleinat

LGTM

miss-islington · 2020-05-26T14:54:24Z

Thanks @idomic for the PR, and @taleinat for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

bedevere-bot · 2020-05-26T14:54:37Z

GH-20431 is a backport of this pull request to the 3.9 branch.

…pythonGH-18625) (cherry picked from commit db098bc) Co-authored-by: idomic <michael.ido@gmail.com>

bedevere-bot · 2020-05-26T14:54:46Z

GH-20432 is a backport of this pull request to the 3.8 branch.

…GH-18625) (cherry picked from commit db098bc) Co-authored-by: idomic <michael.ido@gmail.com>

Added macos case + test

725c58a

idomic requested a review from 1st1 as a code owner February 23, 2020 15:07

the-knights-who-say-ni added the CLA signed label Feb 23, 2020

bedevere-bot added the awaiting review label Feb 23, 2020

📜🤖 Added by blurb_it.

33db325

idomic mentioned this pull request Feb 23, 2020

bpo-39244: multiprocessing get all start methods #18529

Closed

idomic added 2 commits March 1, 2020 11:55

Update 2020-02-23-15-09-47.bpo-39244.aBK5IM.rst

bfa8032

Update 2020-02-23-15-09-47.bpo-39244.aBK5IM.rst

c43f6c4

taleinat reviewed Mar 16, 2020

View reviewed changes

Merge branch 'master' into bpo-39244_multiprocessing

d59d2dd

idomic added 2 commits March 22, 2020 17:20

Moved statement and pulled master

7618700

Merge remote-tracking branch 'origin/bpo-39244_multiprocessing' into …

73a1e3d

…bpo-39244_multiprocessing

taleinat requested changes Apr 24, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Apr 24, 2020

idomic added 4 commits May 17, 2020 16:13

Merge branch 'master' into bpo-39244_multiprocessing

f9687db

Fixed comments for code and tests

872545c

Fixing tests

da6f504

Fixing tests

95cd489

bedevere-bot added awaiting change review and removed awaiting changes labels May 24, 2020

bedevere-bot requested a review from taleinat May 24, 2020 23:07

taleinat requested changes May 25, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels May 25, 2020

Fixing tests

9bf71dd

taleinat reviewed May 25, 2020

View reviewed changes

Added Tal's commit

7db96d5

taleinat added the needs backport to 3.8 label May 26, 2020

Update 2020-02-23-15-09-47.bpo-39244.aBK5IM.rst

341fd79

taleinat approved these changes May 26, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels May 26, 2020

taleinat added the needs backport to 3.9 label May 26, 2020

taleinat merged commit db098bc into python:master May 26, 2020

bedevere-bot removed the awaiting merge label May 26, 2020

bedevere-bot removed needs backport to 3.9 labels May 26, 2020

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 26, 2020

bpo-39244: multiprocessing return default start method first on macOS (…

9e3559b

…pythonGH-18625) (cherry picked from commit db098bc) Co-authored-by: idomic <michael.ido@gmail.com>

miss-islington added a commit that referenced this pull request May 26, 2020

bpo-39244: multiprocessing return default start method first on macOS (…

1c88bf8

…GH-18625) (cherry picked from commit db098bc) Co-authored-by: idomic <michael.ido@gmail.com>

miss-islington added a commit that referenced this pull request May 26, 2020

bpo-39244: multiprocessing return default start method first on macOS (…

285ff63

…GH-18625) (cherry picked from commit db098bc) Co-authored-by: idomic <michael.ido@gmail.com>

		@@ -0,0 +1,2 @@
		Fixed :class:`multiprocessing.context.get_all_start_methods`
		If the context is macOS, return the default methods by order.

Uh oh!

bpo-39244: multiprocessing get all start methods #18625

bpo-39244: multiprocessing get all start methods #18625

Uh oh!

Conversation

idomic commented Feb 23, 2020 • edited by taleinat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

idomic commented Feb 23, 2020

Uh oh!

codecov bot commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

idomic commented Mar 1, 2020

Uh oh!

idomic commented Mar 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taleinat commented Mar 16, 2020

Uh oh!

taleinat commented Mar 16, 2020

Uh oh!

idomic commented Mar 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idomic May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Apr 24, 2020

Uh oh!

idomic commented May 24, 2020

Uh oh!

bedevere-bot commented May 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented May 26, 2020

Uh oh!

bedevere-bot commented May 26, 2020

Uh oh!

bedevere-bot commented May 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

idomic commented Feb 23, 2020 •

edited by taleinat

Loading

codecov bot commented Feb 23, 2020 •

edited

Loading

idomic May 17, 2020 •

edited

Loading