Fix tests for multi-processing with spawn method (i.e. macOSX with Python>3.8)

[bvreede]

Description

Fixed a test that caused problems with MacOSX when using multiprocessing set to spawn (standard in Python 3.8).

• Closes Error message for 'spawn' multiprocessing method #1002
• Link to documentation:

---

Before you get started

• ☝ Create an issue to discuss what you are going to do

Checklist

• PR has a descriptive title for the changelog
• Labels are assigned so they can be used in the changelog
• Code follows the style guide
• Documentation is available for new functionality
• YAML files pass pre-commit or yamllint checks
• Circle/CI tests pass
• Codacy code quality checks pass
• Documentation builds successfully on readthedocs
• Unit tests are available

If you make backwards incompatible changes to the recipe format:

• Update ESMValTool and link the pull request(s) in the description

---

To help with the number pull requests:

• 🙏 We kindly ask you to review two other open pull requests in this repository

[valeriupredoi]

nice one @bvreede - but the important question is if an actual task (not a mock task) runs on OSX without setting the multiproc method at the start of the script? Since that's what was getting raised by the failing test 👍

[bvreede]

Thanks @valeriupredoi! As far as I understand, the task itself runs with either spawn or fork; the issue we zeroed in on was with the test. Fwiw running recipes on mac osx with python >3.8 works well as far as I've seen — have yet to encounter an error. Are there any issues outside of this failing test that we know of, that were fixed by setting multiprocessing to fork?

The method that was used in this test is a monkeypatched _run method that is still used, except now called in a different way; i.e. MockBaseTask._run.

[valeriupredoi]

great! if you confirm that you are able to run a few example recipes on OSX with Python>=3.8 and max_parallel_tasks: set to 2 or more in the config-user then it means we good to go and in business 👍 🍺

[bvreede]

I can confirm this!

[bvreede]

Is that enough? :)

[valeriupredoi]

excellent work 🥳

[valeriupredoi]

Is that enough? :)

absolutely! I feel like a blind man trying to figure all this stuff only relying on the remote GA machine. Good stuff! If I am not too much of a nuisance, would you be able to have a look at the GA tests next, include OSX fully - for tests and builds etc 🍺

[bvreede]

If you can explain to me what the GA tests are 😅 absolutely.
[Apologies; new to this!]

[valeriupredoi]

sorry - Github Actions, you can find them in .github/workflows and let me know if you have questions about them, I'd be very happy to assist!

[stefsmeets]

To ensure that this is working with spawn, can you add a test with a context manager:
i.e. something like this:

with multiprocessing.get_context('spawn').Pool() as pool:
    pool.map(function, args)

[bvreede]

@valeriupredoi

The failing test in GA is the same as the one that's failing on my machine. The latest run on master shows the following failure summary:

FAILED tests/integration/test_task.py::test_run_tasks[2] - TypeError: 'NoneTy...
FAILED tests/integration/test_task.py::test_run_tasks[3] - TypeError: 'NoneTy...
FAILED tests/integration/test_task.py::test_run_tasks[4] - TypeError: 'NoneTy...
FAILED tests/integration/test_task.py::test_run_tasks[None] - TypeError: 'Non...
FAILED tests/integration/test_task.py::test_run_tasks[16] - TypeError: 'NoneT...
FAILED tests/integration/cmor/_fixes/cmip6/test_cesm2.py::test_cl_fix_file

The last one is new to me, however; and it does not fail locally with python 3.8 and 3.9.
Can we add 3.9 to GA tests for OSX? There's a comment that ESMValTool does not work well for 3.9, but it's not clear to me why. It would be informative to see whether this test also fails for 3.9, for one.

[valeriupredoi]

sure - go for it! That's my old comment before we've done all the good stuff with Python 3.9. Pls reinstate all the tests I commented out and add whatever you think it's needed so we start understanding what's going on in OSX land (well, more like you start understanding coz I am OSX-less 😁 ). But let's first merge this and continue work in a new PR, I reckon

[bvreede]

Thanks for the review @stefsmeets! I agree it's a good idea to make sure that the tests (and the software) work with both multiprocessing methods — but given that we have now explicitly removed the dependency on spawn/fork from this test, it doesn't make sense to me to add this condition. Or am I missing something?
It does make sense to test the task function itself under both methods — would this do that?
Appreciate your clarification, I'm learning tons!

[stefsmeets]

Thanks for the review @stefsmeets! I agree it's a good idea to make sure that the tests (and the software) work with both multiprocessing methods — but given that we have now explicitly removed the dependency on spawn/fork from this test, it doesn't make sense to me to add this condition. Or am I missing something?
It does make sense to test the task function itself under both methods — would this do that?
Appreciate your clarification, I'm learning tons!

Hi @bvreede , good question! This is to make sure that we do not silently introduce a bug that affects the multiprocessing behaviour. i.e. if we want to transition to using 'spawn' eventually / support osx, we might as well add a test right now to ensure any change in behaviour of esmvalcore will cause the test to fail for both methods.

[bouweandela]

@bvreede Could you please merge the master branch into this pull request, to see if all tests now pass?

[bvreede]

Not sure if I did that in the most elegant way, but all tests pass now! Merci bien.

[stefsmeets]

Hi @bvreede , nice work! Looks fine to me. I'm just wondering why this works, because it seems like it is almost the same as before. I'm not entirely happy how the second test (test_runner_uses_priority) now monkeypatches MockBaseTask, but I guess it is the way it is for now. I hope it won't cause issues for the next person debugging this in the future.

Just a small comment on the code and then it is good to go. 🚀

[stefsmeets]

All good, I have not strong enough opinions whether this should be mp or multiprocessing, so this can be merged @valeriupredoi 👍