Skip to content
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

Fixes to Processes Manager on MacOS #251

Merged

Conversation

jeremyleung521
Copy link
Member

@jeremyleung521 jeremyleung521 commented Jun 17, 2022

Issue Number. Is this pull request related to any outstanding issues? If so, list the issue number.
#114

Describe the changes made. A clear and concise description of what the problem is and what you did to fix it. E.g. [...] was happening and I've changed [...] to fix it.
Since Python >= 3.8, multiprocessing defaults to spawn on MacOS (link). With spawn, new processes no longer share memory with their parents, leading to cases like in #114 when WESTPA is used with the processes work manager.

The downside to swapping back to the fork method (which is the default on other Unix systems) is that might crash the whole subprocess. Currently, we suggest users to install python 3.7 on MacOS, which defaults to fork and puts us at the same risk anyways.

Added a few lines to swap to fork when it's on a mac since the alternative is either: 1) Rewrite processes so it runs well with spawn or 2) Not working. 1) seemed non-trivial and I think swapping back to fork is a nice compromise.

Goals and Outstanding Issues. A clear and concise list of goals (to be) accomplished.

  • Make Processes work manager work on MacOS + Python >=3.8

Major files changed.

  • src/westpa/work_managers/processes.py

Status.

  • Bug fix (non-breaking change which fixes an issue)
  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

Additional context. Add any other context or screenshots about the pull request here.

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2022

Codecov Report

Merging #251 (b79ab5f) into westpa-2.0-restruct (747daee) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@                   Coverage Diff                   @@
##           westpa-2.0-restruct     #251      +/-   ##
=======================================================
- Coverage                42.04%   42.02%   -0.02%     
=======================================================
  Files                      131      131              
  Lines                    16809    16834      +25     
=======================================================
+ Hits                      7067     7075       +8     
- Misses                    9742     9759      +17     
Flag Coverage Δ
unittests 42.02% <33.33%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/westpa/work_managers/processes.py 91.80% <33.33%> (-1.31%) ⬇️
src/westpa/core/binning/binless_driver.py 18.75% <0.00%> (-0.61%) ⬇️
src/westpa/core/binning/mab.py 4.23% <0.00%> (-0.27%) ⬇️
src/westpa/core/data_manager.py 68.82% <0.00%> (ø)
src/westpa/work_managers/zeromq/work_manager.py 84.98% <0.00%> (+0.68%) ⬆️
src/westpa/core/binning/binless_manager.py 12.50% <0.00%> (+1.15%) ⬆️

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 747daee...b79ab5f. Read the comment docs.

@jeremyleung521 jeremyleung521 merged commit 0576a50 into westpa:westpa-2.0-restruct Jun 22, 2022
@jeremyleung521 jeremyleung521 deleted the westpa2-mac-fork branch July 14, 2022 15:58
@jeremyleung521 jeremyleung521 mentioned this pull request Aug 3, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants