Closed dotsdl closed 2 years ago
One note: for the case of optimizations, which can be long-running tasks that we sought to address here, a gradient calculation with e.g. psi4
will not immediately terminate upon exit of the manager. This is due to the fact that killing the processes in the pool does not kill any child processes, such as those started via subprocess.Popen
.
I don't see a clean way to propagate the SIGTERM that qcengine.compute_procedure
receives in the process pool down through Psi4Harness.compute
and into the subprocess wait block to terminate the child process. However, the current solution at least eliminates the manager immediately so that file cleanup can occur shortly after. The remaining child process is orphaned and will be handled by PID 1 on the host.
Edit: I do have a solution that can cleanly terminate the underlying subprocess.Popen
. Will formulate as a separate PR here and to QCEngine.
This pull request introduces 1 alert when merging 4d196603cd534adc0b8c8f7579aaee24abf46bf7 into 00d989919419b8ad7893631e7b5224f605310aab - view on LGTM.com
new alerts:
From live testing with a local qcfractal-manager
, we only need the changes introduced in this PR to propagate KeyboardInterrupt
into the process pool workers that execute calls to QCEngine compute
and compute_procedure
. Sending a SIGINT, SIGHUP, or SIGTERM to the manager cleanly kills the manager and child psi4
calls down through QCEngine with a process pool, and all scratch files are cleaned up.
@bennybp we are getting CI failures from psql
with all environments. It is not a fundamental issue with the conda-forge
postgresql
package; installing it alone with e.g.:
conda create -n postgres -c conda-forge postgresql python=3.7
gives a psql --version
call that works. Is there a dependency in the CI envs that could be causing the error we're seeing in the CI (reproduced locally):
psql: symbol lookup error: /home/david/.conda/envs/qcfractal-base/bin/../lib/libreadline.so.8: undefined symbol: tputs
?
Or alternatively, should we be restricting the postgresql
version in these environments to some max version?
This pull request introduces 1 alert when merging 3162eb764abaf5da918ea4437ad3668e4d1f57cf into 00d989919419b8ad7893631e7b5224f605310aab - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 13f8878d0b5e823a11665050a843494e97f6011a into 00d989919419b8ad7893631e7b5224f605310aab - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 5fe7d544025f7fbac9c2619317796a725d1d607f into 00d989919419b8ad7893631e7b5224f605310aab - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging d24cc4e9ee28f67b221d80af574fc56dc7f4356f into 00d989919419b8ad7893631e7b5224f605310aab - view on LGTM.com
new alerts:
Not sure what the root cause of the CI failures are now. Superficially they appear unrelated to manager execution, but not certain.
This pull request introduces 1 alert when merging 17a3bcfb2a0bd6b0e2774393aa07ba60aef5d294 into 00d989919419b8ad7893631e7b5224f605310aab - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 94739c168f3ec4820f642a5d73835cf4e3232b7a into 00d989919419b8ad7893631e7b5224f605310aab - view on LGTM.com
new alerts:
@bennybp looks like we're good to go here! Thanks again for all your help!
@bennybp can you review and merge? If we get this and #701 in we should be able to cut a release that we can roll out to production managers.
This pull request introduces 1 alert when merging 6e774536404da7258ecf26c921338f42a89713fa into 8b96203ce0e28cf6322a599af0ef87c5b534659a - view on LGTM.com
new alerts:
Looks like all tests still pass with both QCElemental 0.24 and QCEngine 0.22!
Awesome, thank you @bennybp!
Closes #699.
Swaps out
concurrent.futures.ProcessPoolExecutor
formultiprocessing.Pool
.We needed a way to hard-terminate processes without waiting for them to complete.
multiprocessing
gives us this.Changelog description
ExecutorAdaptor
now usesmultiprocessing.Pool
instead ofconcurrent.futures.ProcessPoolExecutor
, and hard-terminates tasks on manager shutdown.Status