Set start method to fork for MacOs ARM devices #6218

bchen93 · 2022-10-14T15:32:54Z

What is this PR about?
Fixing an issue with use of parallel sampling on M1 Macs where sampling with more than 1 core results in broken pipe error (#5339).

Changing multiprocessing context from forkserver to fork resolves this issue and does not seem to trigger issue (#3849) which was previously resolved by (#3919).

Checklist

Explain important implementation details 👆
Make sure that the pre-commit linting/style checks pass.
Link relevant issues (preferably in nice commit messages)
Are the changes covered by tests and docstrings?
Fill out the short summary sections 👇

Major / Breaking Changes

None Noted

Bugfixes / New features

Setting the default multiprocessing context to "fork" for ARM Macs, and "forkserver" for intel-based Macs.

Docs / Maintenance

...

ricardoV94 · 2022-10-14T18:23:26Z

Would it break stuff for older macOS?

bchen93 · 2022-10-14T18:32:09Z

Would it break stuff for older macOS?

I do not think so. I also tested on a 2016 Macbook Pro with i5 intel chip and it works with fork.

*edit. Not sure about older operating systems, if that's what you were asking.

bchen93 · 2022-10-17T14:15:49Z

After doing a little digging. Fork seemed to be the default start method for multiprocessing in MacOs (and still is for Unix) prior to python 3.8, after which they changed it to spawn (which does not work currently in pymc). There are some instances of fork crashing due to MacOs specific security features (after Os High Sierra (10.13). Which is resolved by setting export OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES. However, I haven't found it necessary to do this after changing the start method to fork in the pymc paralell_sampling.py script.

The main reason for using spawn over fork is that it is safer, and it is currently the only method available to windows.

I'm not sure if given the way that pymc uses multi-processing if the dangers of using fork really apply.

If not using fork, then I think there's a lot more work that needs to be done to make spawn/forkserver methods working on M1 Macs.

ghost · 2022-10-17T22:34:55Z

This seems like something users should be able to set to their liking. I'd be in favor of having a passable argument for sample that defaults to whatever is deemed to be safest or least likely to fail.

bchen93 · 2022-10-18T21:09:35Z

Funnily enough, that is already possible in the pm.sample() method. It accepts an arg mp_ctx, which should be a multiprocessing context object. e.g. using the example model in #5339

import numpy as np
import pymc as pm
import arviz as az
import multiprocessing as mp

def build_model():
    data = np.loadtxt(
        pm.get_data("efron-morris-75-data.tsv"), delimiter="\t", skiprows=1, usecols=(2, 3)
    )

    atbats = pm.floatX(data[:, 0])
    hits = pm.floatX(data[:, 1])

    N = len(hits)

    with pm.Model() as model:
        phi = pm.Uniform("phi", lower=0.0, upper=1.0)
        pareto_dist = pm.Pareto.dist(alpha=1.0001, m=1.5)
        kappa = pm.Bound("kappa", pareto_dist, lower=1.0)
        thetas = pm.Beta("thetas", alpha=phi * kappa, beta=(1.0 - phi) * kappa, shape=N)
        ys = pm.Binomial("ys", n=atbats, p=thetas, observed=hits)
    return model

# %%
mp_ctx = mp.get_context('fork') # <--- Get fork, spawn, or forkserver context object
model = build_model()
with model:
    trace = pm.sample(1000, target_accept=0.99, mp_ctx = mp_ctx) <---Tell sampler to use this context

However, as it stands, anything run on M1 mac will not work without having to define the start method as fork. Would it be better to set the default start method for ARM macs as fork and for intel macs as forkserver as before?

So essentially changing the code in parallel_sampling.py to:

        if mp_ctx is None or isinstance(mp_ctx, str):
            # Closes issue https://github.com/pymc-devs/pymc/issues/3849
            if platform.system() == "Darwin":
                if platform.processor() == 'arm':
                    mp_ctx = 'fork'
                else: 
                    mp_ctx = "forkserver"

ricardoV94 · 2022-10-19T14:26:02Z

However, as it stands, anything run on M1 mac will not work without having to define the start method as fork. Would it be better to set the default start method for ARM macs as fork and for intel macs as forkserver as before?

So essentially changing the code in parallel_sampling.py to:

That sounds reasonable, we can always revert if that starts causing problems. Can we get some more M1 users to confirm that the model in the original issue indeed fails for everyone?

twiecki · 2022-10-19T17:04:01Z

@drbenvincent confirmed this fixed the same issue for him on a different model. I think we can merge this.

ricardoV94 · 2022-10-19T17:08:03Z

We shouldn't merge this, but the default based on wether processor is arm or not mentioned in the last comment by the Op: #6218 (comment)

codecov · 2022-10-19T17:28:27Z

Codecov Report

Merging #6218 (4848422) into main (d47dac0) will decrease coverage by 3.80%.
The diff coverage is 92.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6218      +/-   ##
==========================================
- Coverage   93.58%   89.78%   -3.81%     
==========================================
  Files         101      101              
  Lines       22136    22131       -5     
==========================================
- Hits        20716    19870     -846     
- Misses       1420     2261     +841

Impacted Files	Coverage Δ
pymc/backends/ndarray.py	`79.27% <ø> (-0.19%)`	⬇️
pymc/parallel_sampling.py	`85.52% <66.66%> (-0.24%)`	⬇️
pymc/backends/base.py	`87.95% <100.00%> (+0.84%)`	⬆️
pymc/sampling.py	`82.62% <100.00%> (+0.14%)`	⬆️
pymc/tests/backends/fixtures.py	`78.30% <100.00%> (+0.31%)`	⬆️
pymc/tests/distributions/test_bound.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/distributions/test_censored.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/distributions/test_simulator.py	`0.00% <0.00%> (-99.51%)`	⬇️
pymc/tests/distributions/test_truncated.py	`0.00% <0.00%> (-99.49%)`	⬇️
pymc/distributions/truncated.py	`34.96% <0.00%> (-64.34%)`	⬇️
... and 7 more

twiecki · 2022-10-19T20:33:03Z

We shouldn't merge this

Why not?

Changing default multiprocessing start method to "fork" for ARM Macs and leaving default "forkserver" for intel macs.

bchen93 · 2022-10-19T22:17:23Z

We shouldn't merge this

Why not?

I believe @ricardoV94 wanted to change the then committed code s.t. the default for ARM Macs was specified as "fork" and and for intel Macs as "forkserver" (as it has been). I just committed the change to the code. So it can be checked/merged now if everyone is in agreement.

ricardoV94 · 2022-10-20T06:56:25Z

@bchen93 Can you clean up the changelist in the top comment? It should mention in bugfixes what you did (use different default when on MacOS ARM). You can leave the other sections empty

We use those for the automatically generated release notes.

bchen93 · 2022-10-20T13:06:54Z

@bchen93 Can you clean up the changelist in the top comment? It should mention in bugfixes what you did (use different default when on MacOS ARM). You can leave the other sections empty

We use those for the automatically generated release notes.

Done!

The parameterization uses the variable `mu` but the docstring uses `mu` in some cases and `theta` in other cases. The fix replaces all `theta` with `mu` to match the argument name for this distribution.

review-notebook-app · 2022-10-20T13:19:09Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

ricardoV94 · 2022-10-20T15:06:22Z

You seem to have caught unrelated changes in your PR

This reverts commit 726d8d0.

This reverts commit 631d3d4.

bchen93 · 2022-10-20T15:13:57Z

You seem to have caught unrelated changes in your PR

Sorry about that. I reverted the commits.

twiecki · 2022-10-20T21:39:00Z

/pre-commit-run

twiecki · 2022-10-21T07:51:22Z

Thanks @bchen93!

fixing paralell sampling issue on M1 Macs

67bb45d

bchen93 mentioned this pull request Oct 14, 2022

Strange Aesara warning and broken pipe error when fitting model with missing values #5339

Closed

twiecki marked this pull request as ready for review October 19, 2022 17:04

Changing set method defaults for arm vs intel Mac

b6caad8

Changing default multiprocessing start method to "fork" for ARM Macs and leaving default "forkserver" for intel macs.

ricardoV94 changed the title ~~Set start method to fork for MacOs~~ Set start method to fork for MacOs ARM devices Oct 20, 2022

cscheffler and others added 2 commits October 20, 2022 09:15

Fix docstring of the ZeroInflatedPoisson distribution.

631d3d4

The parameterization uses the variable `mu` but the docstring uses `mu` in some cases and `theta` in other cases. The fix replaces all `theta` with `mu` to match the argument name for this distribution.

Fix debug_print of wrong variable in notebook

726d8d0

bchen93 added 2 commits October 20, 2022 11:09

Revert "Fix debug_print of wrong variable in notebook"

723a9cb

This reverts commit 726d8d0.

Revert "Fix docstring of the ZeroInflatedPoisson distribution."

1f90837

This reverts commit 631d3d4.

twiecki approved these changes Oct 20, 2022

View reviewed changes

Run pre-commit

4848422

twiecki merged commit 7bad057 into pymc-devs:main Oct 21, 2022

bchen93 deleted the macOs-mp branch October 21, 2022 19:59

This was referenced Dec 1, 2022

New mp_ctx defaults in 4.3.0+ (#6218) for M1 Macs could break parallel sampling for M1 Mac users using JAX #6362

Closed

Fixed overriding user provided mp_ctx strings to pm.sample() on M1 MacOS #6363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set start method to fork for MacOs ARM devices #6218

Set start method to fork for MacOs ARM devices #6218

bchen93 commented Oct 14, 2022 •

edited

Loading

ricardoV94 commented Oct 14, 2022

bchen93 commented Oct 14, 2022 •

edited

Loading

bchen93 commented Oct 17, 2022

ghost commented Oct 17, 2022

bchen93 commented Oct 18, 2022 •

edited

Loading

ricardoV94 commented Oct 19, 2022

twiecki commented Oct 19, 2022

ricardoV94 commented Oct 19, 2022 •

edited

Loading

codecov bot commented Oct 19, 2022 •

edited

Loading

twiecki commented Oct 19, 2022

bchen93 commented Oct 19, 2022

ricardoV94 commented Oct 20, 2022 •

edited

Loading

bchen93 commented Oct 20, 2022

review-notebook-app bot commented Oct 20, 2022

ricardoV94 commented Oct 20, 2022

bchen93 commented Oct 20, 2022

twiecki commented Oct 20, 2022

twiecki commented Oct 21, 2022

Set start method to fork for MacOs ARM devices #6218

Set start method to fork for MacOs ARM devices #6218

Conversation

bchen93 commented Oct 14, 2022 • edited Loading

Major / Breaking Changes

Bugfixes / New features

Docs / Maintenance

ricardoV94 commented Oct 14, 2022

bchen93 commented Oct 14, 2022 • edited Loading

bchen93 commented Oct 17, 2022

ghost commented Oct 17, 2022

bchen93 commented Oct 18, 2022 • edited Loading

ricardoV94 commented Oct 19, 2022

twiecki commented Oct 19, 2022

ricardoV94 commented Oct 19, 2022 • edited Loading

codecov bot commented Oct 19, 2022 • edited Loading

Codecov Report

twiecki commented Oct 19, 2022

bchen93 commented Oct 19, 2022

ricardoV94 commented Oct 20, 2022 • edited Loading

bchen93 commented Oct 20, 2022

review-notebook-app bot commented Oct 20, 2022

ricardoV94 commented Oct 20, 2022

bchen93 commented Oct 20, 2022

twiecki commented Oct 20, 2022

twiecki commented Oct 21, 2022

bchen93 commented Oct 14, 2022 •

edited

Loading

bchen93 commented Oct 14, 2022 •

edited

Loading

bchen93 commented Oct 18, 2022 •

edited

Loading

ricardoV94 commented Oct 19, 2022 •

edited

Loading

codecov bot commented Oct 19, 2022 •

edited

Loading

ricardoV94 commented Oct 20, 2022 •

edited

Loading