[blocked by refactor] [WIP] graceful shutdown signal handling

[awaelchli]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1999
Fixes #2913
maybe also fixes #2590 (need to check)
maybe also fixes #3275 (need to check)

• simplifies graceful shutdown (SIGINT is equivalent to KeyboardInterrupt)
• moved all signal handling to one place
• restored original signal handlers after training finishes

TODO:

• docs

[tullie]

Thanks for writing this it'll let me fix the logging finalize bug i'm looking into.

I think long term we need to put more thought into signal handling. Ideally, we'd have a global signal handling teardown function that closes everything nicely whether we're in the training loop, evaluate loop or in between.

[awaelchli]

I found out that in the kill signal handler, we need to call sys.exit(), otherwise the process hangs after tests completed. It seems the test suite sends one of these kill signals...
@Borda I merged your branch to check that CI works.

[awaelchli]

@justusschock I noticed that the try-catch block for the KeyboardInterrupt is around almost all the code in the train method. Do you think we could simply wrap the try catch around where we call train()?

[codecov]

Codecov Report

Merging #2165 into master will decrease coverage by 3%.
The diff coverage is 91%.

@@           Coverage Diff           @@
##           master   #2165    +/-   ##
=======================================
- Coverage      90%     87%    -3%     
=======================================
  Files          81      81            
  Lines        7644    7435   -209     
=======================================
- Hits         6878    6433   -445     
- Misses        766    1002   +236     

[williamFalcon]

@awaelchli i think this is WIP no?
let's make sure 100% that this is correct before merging so we don't add a major bug to 0.8.0? otherwise, we should hold off until 0.9.0

[awaelchli]

It's finished except I don't know how to test these signals in CI and slurm.

[pep8speaks]

Hello @awaelchli! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-16 16:15:27 UTC

[mergify]

This pull request is now in conflict... :(

[Borda]

LGTM 🐰

[Borda]

do not forget this one...

[Borda]

is still some extra docs missing?

[awaelchli]

@Borda this is not ready to go. this pr is completely destroyed now, there are too many conflicts. I have to start from scratch and send a new PR, I'll keep it open for now so I don't forget. I will work on it very soon, I promise.