Make autodE compatible with Windows and change parallel process handling #202

shoubhikraj · 2022-11-20T10:58:22Z

Major change: multiprocessing library replaced with concurrent.futures for Linux/macOS and loky for Windows

concurrent.futures has ProcessPoolExecutor with slightly different argument names, but uses multiprocessing backend, so there should be no difference in behaviour
Multiprocessing runs poorly on Windows due to issues with pickling, importing __main__ etc.; loky solves many of those issues

Minor change: timeout decorator in autode/utils.py is reimplemented with a switch for Windows

Without switch, there is no timeout as process creation is time-consuming
Setting Config.use_experimental_timeout = True will use a reusable process pool from loky to implement timeout

Other changes

Context manager added for temporary config

Tests on Windows: all success

TODO:

Some tests need to be modified so that they work on windows
Some tests are failling because of Windows file permission quirks - add extra code to deal with file permissions
Add continuous-integration on Windows
Add name to https://github.com/duartegroup/autodE#contributors
Pass the module configs to worker processes

…lib and loky

Fix tests so that they work on windows

t-young31 · 2022-11-21T20:05:01Z

it looks like the CI test where XTB calculations are run (pytest_codecov) are failing because there isn't sufficient memory on the runner. Don't understand why 16 GB is being required because Config.n_cores should be set to 1 before each test 😕 (tests/conftest.py). Nevertheless, I think the memory check should be on the total rather than the available, and let the OS deal with the consequences!

t-young31 · 2022-11-21T20:07:06Z

also looks like the linting is failing because black hasn't been run. if you follow the dev install instructions and install pre-commit it'll do it without having to think about it!

t-young31

Looking great – have added some thoughts

autode/hessians.py

autode/pes/relaxed.py

autode/pes/unrelaxed.py

autode/utils.py

requirements.txt

tests/test_hessian.py

tests/test_wrappers/test_gaussian.py

shoubhikraj · 2022-11-22T15:49:30Z

it looks like the CI test where XTB calculations are run (pytest_codecov) are failing because there isn't sufficient memory on the runner. Don't understand why 16 GB is being required because Config.n_cores should be set to 1 before each test 😕 (tests/conftest.py). Nevertheless, I think the memory check should be on the total rather than the available, and let the OS deal with the consequences!

Thanks for the review! Is there anything I could locally check about the XTB test? I haven't modified anything in the conftest.py file, so not sure why it is not working. I used available memory because the OS is bound to consume some amount of memory at all times, so requesting the total amount of memory (by Orca or any other software) would increase the chances of crash. Is there any harm in using available memory, if it is available from psutil?

Co-authored-by: Tom Young <39765193+t-young31@users.noreply.github.com>

t-young31 · 2022-11-27T11:23:07Z

Is there anything I could locally check about the XTB test? I haven't modified anything in the conftest.py file, so not sure why it is not working.

I imagine it was broken before this PR and I just didn't spot it. If you find out why that would be awesome!

I used available memory because the OS is bound to consume some amount of memory at all times, so requesting the total amount of memory (by Orca or any other software) would increase the chances of crash. Is there any harm in using available memory, if it is available from psutil?

I don't think either the total or available memory is the right number because what you really want is the maximum amount that can be allocated to the calculation without kicking crucial stuff out of memory. I'd however be pretty strongly in favour of having it as the total because the available memory makes the chances that a calculation raising that runtime error non-deterministic... which would be super annoying. Say, If the OS could re-jig things and make it possible to run the calc. e.g. if I happen to have a load of tabs open in my browser and try to run something locally I'd rather they were kicked out than my calculation failing.

.github/workflows/pytest_cov.yml

autode/config.py

autode/utils.py

requirements.txt

tests/test_utils.py

…imeout for windows

shoubhikraj · 2023-01-09T09:32:25Z

The config should be picklable now, and the original format has been left unchanged. The pytest-codecov for patch is failing because of the parts of code that do not run on the main process. Not sure if it can be fixed.

t-young31

I've got more comments – sorry. only suggestions really though. looks awesome overall 🚀

autode/utils.py

install_xtb_ci.sh

tests/test_calculation.py

tests/test_graphs.py

tests/test_calculation.py

…ing (#202) * make autode compatible with windows: replace multiprocessing with joblib and loky * remove comment autode/utils.py * check total memory in utils * use loky for parallel processing * loky context is used for better handling of parallelisation * fixes for timeout wrapper in POSIX * use class instances instead of class attributes for Config * pass parent Config state into child processes * keep default timeout wrapper, also add new wrapper * separate parallelisation scheme for windows, restore default for linux/mac * fix test_config_in_worker_proc for posix * codecov on windows * add tests for config copy * change config implementation for updating in worker process * add context manager for temporary config, minor fix in experimental timeout for windows * add tests for context manager * use bash for installing xtb in CI runner * add tests for cleanup after timeout * exclude setup.py from codecov * make config picklable and easily readable * add test and minor fixes * move xtb install script to .github/scripts * use temporary_config for test_utils.py * update changelog and contributor list * resolve conflicts with v1.4.0 Co-authored-by: Tom Young <39765193+t-young31@users.noreply.github.com>

* Ignore doc paths for code CI (duartegroup#235) * Ignore doc paths for code CI * Update PR template * Add "ERROR" level to logging (duartegroup#231) * add ERROR level to logging * update docs for logging * update tests for logging * update changelog * Fix parsing Qchem v6 output (duartegroup#234) * Fix parsing Qchem v6 output * Fix units for values div mul (duartegroup#242) * Fix units for div mul * Update changelog * Update workflow (duartegroup#244) * Fix xtb opt with cartesian coordinates (duartegroup#247) * Fix xtb opt with cartesian coordinates * Update changelog [skip actions] * Remove hessian conversion on normal mode calculation (duartegroup#243) * Ensure no unit conversion in hessian * Use a copy for value and valuearray * Raise exception on value `to` with inplace * Add missed changelog updates * Template for v1.4.0 (duartegroup#203) * Make autodE compatible with Windows and change parallel process handling (duartegroup#202) * make autode compatible with windows: replace multiprocessing with joblib and loky * remove comment autode/utils.py * check total memory in utils * use loky for parallel processing * loky context is used for better handling of parallelisation * fixes for timeout wrapper in POSIX * use class instances instead of class attributes for Config * pass parent Config state into child processes * keep default timeout wrapper, also add new wrapper * separate parallelisation scheme for windows, restore default for linux/mac * fix test_config_in_worker_proc for posix * codecov on windows * add tests for config copy * change config implementation for updating in worker process * add context manager for temporary config, minor fix in experimental timeout for windows * add tests for context manager * use bash for installing xtb in CI runner * add tests for cleanup after timeout * exclude setup.py from codecov * make config picklable and easily readable * add test and minor fixes * move xtb install script to .github/scripts * use temporary_config for test_utils.py * update changelog and contributor list * resolve conflicts with v1.4.0 Co-authored-by: Tom Young <39765193+t-young31@users.noreply.github.com> * update docs (duartegroup#229) * update docs for windows installation and temporary_config * update docs for windows installation and temporary_config * minor edit Co-authored-by: Tom Young <39765193+t-young31@users.noreply.github.com> * Autouse tempoary_config (duartegroup#236) * minor fixes --------- Co-authored-by: Tom Young <39765193+t-young31@users.noreply.github.com>

shoubhikraj added 3 commits November 19, 2022 20:18

make autode compatible with windows: replace multiprocessing with job…

a58776d

…lib and loky

minor changes to parallel process handling

d974b47

minor change to timeout decorator

dde009d

t-young31 added this to the v1.4.0 milestone Nov 20, 2022

shoubhikraj and others added 3 commits November 20, 2022 18:16

fix tests to work on windows

d338b87

Merge pull request #1 from shoubhikraj/tests-mp-to-joblib

75a58f0

Fix tests so that they work on windows

CI on windows

35a1b0b

t-young31 changed the base branch from master to v1.4.0 November 21, 2022 08:48

shoubhikraj marked this pull request as ready for review November 21, 2022 11:04

shoubhikraj requested a review from t-young31 November 21, 2022 11:04

t-young31 reviewed Nov 21, 2022

View reviewed changes

t-young31 added the enhancement New feature or request label Nov 21, 2022

shoubhikraj and others added 9 commits November 22, 2022 20:14

remove unnecessary dependencies

7832bea

Co-authored-by: Tom Young <39765193+t-young31@users.noreply.github.com>

fix test to work on Windows

d16c550

Merge remote-tracking branch 'origin/master'

3f93d87

use list comprehension in unrelaxed.py

561a16a

fix syntax error in test_numerical_hessian_in_daemon

e16157e

fix syntax error in test_numerical_hessian_in_daemon

3eba6a8

remove comment autode/utils.py

27edb7c

Co-authored-by: Tom Young <39765193+t-young31@users.noreply.github.com>

fix formatting

9594f09

Merge remote-tracking branch 'origin/master'

0b89276

shoubhikraj and others added 5 commits November 30, 2022 10:06

Merge branch 'duartegroup:master' into master

76a5deb

check total memory in utils

d532aa9

Merge remote-tracking branch 'origin/master'

de0e782

use loky for parallel processing

a48a263

reimplement timeout with Loky

a65f483

shoubhikraj added 2 commits December 19, 2022 11:11

add tests for config copy

d10eca2

change config implementation for updating in worker process

c26be3a

t-young31 reviewed Dec 30, 2022

View reviewed changes

shoubhikraj added 12 commits December 31, 2022 15:32

add context manager for temporary config, minor fix in experimental t…

8bc055a

…imeout for windows

add tests for context manager

ae786b6

use bash for installing xtb in CI runner

733339c

minor fix for bash ci helper script

30ac6c4

minor fix

e7e6a7d

minor fix

aea9b7b

add tests for cleanup after timeout

d9f8d66

exclude setup.py from codecov

870f564

minor change

1d03d08

make config picklable and easily readable

1da3530

make config picklable and easily readable

0e753c3

add test and minor fixes

4d2cf77

shoubhikraj requested a review from t-young31 January 9, 2023 09:32

t-young31 reviewed Jan 10, 2023

View reviewed changes

autode/utils.py Show resolved Hide resolved

install_xtb_ci.sh Outdated Show resolved Hide resolved

tests/test_calculation.py Outdated Show resolved Hide resolved

tests/test_graphs.py Show resolved Hide resolved

shoubhikraj added 2 commits January 13, 2023 16:05

move xtb install script to .github/scripts

34fa886

use temporary_config for test_utils.py

b177e3a

t-young31 approved these changes Jan 14, 2023

View reviewed changes

t-young31 force-pushed the v1.4.0 branch from 5fc5d3f to a8c2b8f Compare January 15, 2023 11:47

shoubhikraj added 3 commits January 15, 2023 14:18

update changelog and contributor list

f019b8d

resolve conflicts with v1.4.0

b469ad3

Merge branch 'v1.4.0' into master

4b333c3

t-young31 reviewed Jan 16, 2023

View reviewed changes

tests/test_calculation.py Show resolved Hide resolved

t-young31 merged commit 23af779 into duartegroup:v1.4.0 Jan 16, 2023

shoubhikraj mentioned this pull request Jun 8, 2023

macOS multiprocessing bugfix #278

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make autodE compatible with Windows and change parallel process handling #202

Make autodE compatible with Windows and change parallel process handling #202

shoubhikraj commented Nov 20, 2022 •

edited

Loading

t-young31 commented Nov 21, 2022

t-young31 commented Nov 21, 2022

t-young31 left a comment

shoubhikraj commented Nov 22, 2022

t-young31 commented Nov 27, 2022

shoubhikraj commented Jan 9, 2023

t-young31 left a comment

Make autodE compatible with Windows and change parallel process handling #202

Make autodE compatible with Windows and change parallel process handling #202

Conversation

shoubhikraj commented Nov 20, 2022 • edited Loading

t-young31 commented Nov 21, 2022

t-young31 commented Nov 21, 2022

t-young31 left a comment

Choose a reason for hiding this comment

shoubhikraj commented Nov 22, 2022

t-young31 commented Nov 27, 2022

shoubhikraj commented Jan 9, 2023

t-young31 left a comment

Choose a reason for hiding this comment

shoubhikraj commented Nov 20, 2022 •

edited

Loading