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

set the multiprocessing distribution method from getgoing #786

Closed
wants to merge 2 commits into from

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Sep 16, 2020

This is needed because on OSX on Python 3.8 the start method is either None or is set to spawn (see saltstack/salt#55847) - and tests are failing (well, one can not actually run with more than one process) see https://github.com/ESMValGroup/ESMValCore/runs/1116923215?check_suite_focus=true

Before you start, please read our contribution guidelines.

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes #issue_number

@valeriupredoi
Copy link
Contributor Author

cheers for merging the vmpfrof -> vprof PR @bouweandela - let's now get this one in as well so we can have a fully working environment on OSX with Python 3.8 - see https://github.com/ESMValGroup/ESMValCore/runs/1175425522?check_suite_focus=true 🍺

@bouweandela
Copy link
Member

I can see the failing tests, but I do not quite understand what the problem is here. Why is it a problem that the default multiprocessing start method is 'spawn' on Mac OS? I don't think we are using the salt package that you linked an issue from above?

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Sep 28, 2020

you are right, I should have edited my comment when I found out the real cause - the method is not set on OSX (returns None) for Python 3.8 - I don't know why but I see this both on the GA machine and on the Mac I was using for tests myself 🍺

ie mp.get_start_method(allow_none=True) is None on OSX-Py3.8 - maybe a missing setting in the mpi config? Either way, you can check it yourself if you have a Mac somewhere, it's pretty annoying

@bouweandela
Copy link
Member

I would rather like to know what the actual bug is, but if you cannot find an issue, we'll just have to try and work around it.

Could you try what happens if you just insert

multiprocessing.get_start_method()

here (just before creating the Pool instance)

with Pool(processes=max_parallel_tasks) as pool:

instead of the changes in this pull request?
According to the get_start_method documentation that should fix the start method if it hasn't been fixed yet.

@valeriupredoi
Copy link
Contributor Author

yep I agree with you, but sadly I don't have access to the Mac no more - I also believe I tried that and it didn't work, maybe you can try plugging that into github-actions2 branch and see if the test runs to completion? 🍺

@valeriupredoi
Copy link
Contributor Author

@valeriupredoi
Copy link
Contributor Author

@bouweandela if you don't plan on allowing this through we will know for sure the tool doesn't work on OSX with Python 3.8 (possibly 3.9+ too - not 100% sure of that yet, 3.9 is still too young for OSX) (see repeated failed tests) so we either allow the patch through or we tell users about the OS+Python version restrictions 👍

@bouweandela
Copy link
Member

I'm not convinced this 'solution' is the right approach, they changed the default way of starting new processes on Mac OS for a reason, i.e. fork didn't work: see https://bugs.python.org/issue33725. Just forcing the old behaviour is unlikely to solve the problem.

We have the following note on Mac OS X in the tutorial installation instructions:

On MacOSX, installation only works fine for esmvaltool-python and esmvaltool-ncl with Python 3.7.

Maybe we could add something similar in the common installation issues section I added in ESMValGroup/ESMValTool#1971, but until at least one active developer of @ESMValGroup/esmvaltool-coreteam gets a Mac and is willing to invest the time to support Mac OS I think the experience of using ESMValTool on Mac OS will remain quite bumpy.

@jvegreg
Copy link
Contributor

jvegreg commented Jan 11, 2021

until at least one active developer of @ESMValGroup/esmvaltool-coreteam gets a Mac and is willing to invest the time to support Mac OS I think the experience of using ESMValTool on Mac OS will remain quite bumpy.

It was usually quite smooth. Maybe I can borrow the old one I was using and take a look at this

@valeriupredoi
Copy link
Contributor Author

OK sounds good - Javi, make sure you are using Python 3.8, older versions work fine. Maybe we can setup a docker container or a VM somewhere where we have OSX latest installed?

@bvreede
Copy link
Contributor

bvreede commented Jan 26, 2021

FWIW I ran tests on my machine (OSX with Python 3.8) using this branch, and all was fine. This fixes the compatibility of ESMValTool with Python 3.8, so it could be worth merging this PR. I discussed this with @stefsmeets and he agrees :)

@valeriupredoi
Copy link
Contributor Author

cheers @bvreede - testing much appreciated! You'll have to convince @bouweandela to have this merged, he's quite suspicious of its functionality, I have given up trying to convince him 😆

@stefsmeets
Copy link
Contributor

stefsmeets commented Jan 26, 2021

I'm not convinced this 'solution' is the right approach, they changed the default way of starting new processes on Mac OS for a reason, i.e. fork didn't work: see https://bugs.python.org/issue33725. Just forcing the old behaviour is unlikely to solve the problem.

Seems to me like this PR is a nice fix to get it to work. If you go through the discussion on bpo, you will see that 'fork' has the same issues on 3.7 and 3.8 on OSX 10.13 and above. It is that on 3.8 they recognized this and marked the behaviour as unsafe by defaulting to 'spawn'. The cause is unexpected behaviour from code calling into Apple system frameworks (which we don't use). There is no reason to believe that this patch from @valeriupredoi will cause any new issues in esmvaltool.

I suggest we apply this patch, or make it very clear that we do not support esmvaltool on Mac.

@bouweandela
Copy link
Member

The failing tests were fixed in #1003.

@bouweandela bouweandela closed this Mar 2, 2021
@bouweandela bouweandela deleted the set_mpi_start_method branch March 2, 2021 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants