Graceful shutdown on python interpreter exit #1631

justusschock · 2020-04-27T08:09:51Z

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?

With this pr we can register functions to be run on interpreter exit
Partial fix to #1228

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2020-04-27T08:09:55Z

Hello @justusschock! Thanks for updating this PR.

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

Comment last updated at 2020-05-26 20:52:30 UTC

CHANGELOG.md

pytorch_lightning/trainer/training_loop.py

mergify · 2020-04-27T11:40:49Z

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

justusschock · 2020-04-27T15:23:14Z

unfortunately this seems to be impossible for SIGKILL: https://mail.python.org/pipermail/python-list/2003-June/206903.html

But for SIGTERM, SIGSEGV and SIGINT it should work. If a cluster decides to kill a process, would it use SIGKILL or SIGTERM?

Edit: for SLURM: They try with a SIGTERM first, followed by a SIGKILL if necessary: https://slurm.schedmd.com/scancel.html

mergify · 2020-04-27T21:41:56Z

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

jeremyjordan · 2020-04-28T01:25:58Z

have you tested this in ddp mode to see if it resolves #965 ?

justusschock · 2020-04-28T07:07:31Z

@jeremyjordan not explicitly, but I thought that our gpu tests run on ddp?

justusschock · 2020-04-28T07:25:32Z

@Borda Any Idea, why tests are permanently auto-cancelled here?

justusschock · 2020-04-28T07:34:28Z

Somehow tests are taking a really long time when they are already finished....

Borda · 2020-04-28T08:13:08Z

Any Idea, why tests are permanently auto-cancelled here?

I had something similar when I implemented __del__ in the logger and then it started hanging out like the pytest are not able to properly release resources but running the same locally was fine...
I was contacting support about this issue but not much advice...

justusschock · 2020-04-28T08:55:58Z

What did you do in the end? Delete it?

pytorch_lightning/trainer/training_loop.py

williamFalcon · 2020-05-02T12:58:33Z

@justusschock can we rebase master and restart these checks?

Borda · 2020-05-02T15:37:56Z

/rebase

williamFalcon · 2020-05-10T21:06:52Z

pytorch_lightning/trainer/training_loop.py

@@ -299,6 +303,15 @@ def has_arg(self, *args):
        """Warning: this is just empty shell for code implemented in other class."""

    def train(self):
+        # add signal handlers for process kills
+        def _signal_kill_handler(*args):


won't this interfere with the HPC auto-save signal?

I don't know. I think it shouldn't, because this will be triggered only on python interpreter exit (with SIGTERM).

Borda · 2020-05-11T19:13:01Z

@justusschock do you think we can finish it this week?

mergify · 2020-05-12T01:59:43Z

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

williamFalcon · 2020-05-17T14:06:14Z

@Borda @awaelchli these tests are timing out. did we change something?

awaelchli · 2020-05-17T22:43:02Z

Could it be that the shutdown code added here has side effects on the test suite? Maybe tests are waiting on some processes to finish that are stuck?

justusschock · 2020-05-18T05:42:15Z

@williamFalcon they timedout straight from the beginning :D

@awaelchli That's what I'm afrai of. That way we can't really test this and we also cannot use our CI anymore with this feature...

Borda · 2020-05-18T11:56:24Z

Could it be that the shutdown code added here has side effects on the test suite? Maybe tests are waiting on some processes to finish that are stuck?

yes, it is this case but it is very hard to reproduce locally, I had something similar when I implemented simple __del__ in the logger and it started hanging on CI but passing locally

Co-Authored-By: Jirka Borovec <[email protected]>

codecov · 2020-05-26T21:48:31Z

Codecov Report

Merging #1631 into master will increase coverage by 0%.
The diff coverage is 94%.

@@          Coverage Diff           @@
##           master   #1631   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          74      74           
  Lines        4650    4663   +13     
======================================
+ Hits         4070    4082   +12     
- Misses        580     581    +1

awaelchli

This is great :)

awaelchli · 2020-05-30T01:30:42Z

When building the docs I see this now:

Error in atexit._run_exitfuncs:
TypeError: run_training_teardown() missing 1 required positional argument: 'self'

Any idea if this is serious or can it be ignored in docs?

Borda · 2020-05-30T05:45:15Z

It is the same as #1999
This happens when you call run_training_teardown() on a class not instance...

* Fraceful shutdown on python interpreter exit * Update CHANGELOG.md * Update training_loop.py * Update training_loop.py * Update CHANGELOG.md Co-Authored-By: Jirka Borovec <[email protected]> * pep8, move to constant * Update training_loop.py * Update training_loop.py * Update training_loop.py * pep8, move to constant * pep8 * timeout Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Jirka <[email protected]>

justusschock requested review from williamFalcon, awaelchli, Borda, jeremyjordan and a team April 27, 2020 08:09

Borda approved these changes Apr 27, 2020

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team April 27, 2020 09:23

justusschock mentioned this pull request Apr 27, 2020

Remove warning #1634

Merged

5 tasks

awaelchli reviewed Apr 29, 2020

View reviewed changes

pytorch_lightning/trainer/training_loop.py Show resolved Hide resolved

mergify bot requested a review from a team April 29, 2020 00:22

Borda added the feature Is an improvement or enhancement label Apr 30, 2020

Borda added this to the 0.7.6 milestone Apr 30, 2020

justusschock force-pushed the at_exit branch from a032094 to fcdadcc Compare May 2, 2020 17:56

Borda force-pushed the at_exit branch from fcdadcc to eab8be7 Compare May 9, 2020 08:48

williamFalcon reviewed May 10, 2020

View reviewed changes

mergify bot requested a review from a team May 10, 2020 21:07

williamFalcon approved these changes May 17, 2020

View reviewed changes

mergify bot requested a review from a team May 17, 2020 12:38

Borda modified the milestones: 0.7.7, 0.8.0 May 26, 2020

justusschock and others added 12 commits May 26, 2020 22:52

Fraceful shutdown on python interpreter exit

bc8c5ee

Update CHANGELOG.md

ebfb51e

Update training_loop.py

d6e6db6

Update training_loop.py

adc6c7c

Update CHANGELOG.md

5584370

Co-Authored-By: Jirka Borovec <[email protected]>

pep8, move to constant

e799f16

Update training_loop.py

c075610

Update training_loop.py

388bd12

Update training_loop.py

3d3f5f3

pep8, move to constant

705d903

pep8

8ff10c8

timeout

27b0277

Borda force-pushed the at_exit branch from 7046e22 to 27b0277 Compare May 26, 2020 20:52

awaelchli mentioned this pull request May 28, 2020

Stopping the code along with a graceful shutdown. #1988

Closed

Borda requested a review from awaelchli May 29, 2020 13:16

awaelchli approved these changes May 29, 2020

View reviewed changes

Borda merged commit ceecf1c into master May 29, 2020

Borda deleted the at_exit branch May 29, 2020 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graceful shutdown on python interpreter exit #1631

Graceful shutdown on python interpreter exit #1631

justusschock commented Apr 27, 2020

pep8speaks commented Apr 27, 2020 •

edited

Loading

mergify bot commented Apr 27, 2020

justusschock commented Apr 27, 2020 •

edited

Loading

mergify bot commented Apr 27, 2020

jeremyjordan commented Apr 28, 2020

justusschock commented Apr 28, 2020

justusschock commented Apr 28, 2020

justusschock commented Apr 28, 2020

Borda commented Apr 28, 2020 •

edited

Loading

justusschock commented Apr 28, 2020

williamFalcon commented May 2, 2020

Borda commented May 2, 2020

williamFalcon May 10, 2020

justusschock May 12, 2020

Borda commented May 11, 2020

mergify bot commented May 12, 2020

williamFalcon commented May 17, 2020

awaelchli commented May 17, 2020 •

edited

Loading

justusschock commented May 18, 2020

Borda commented May 18, 2020

codecov bot commented May 26, 2020

awaelchli left a comment

awaelchli commented May 30, 2020

Borda commented May 30, 2020

Graceful shutdown on python interpreter exit #1631

Graceful shutdown on python interpreter exit #1631

Conversation

justusschock commented Apr 27, 2020

Before submitting

What does this PR do?

PR review

Did you have fun?

pep8speaks commented Apr 27, 2020 • edited Loading

Comment last updated at 2020-05-26 20:52:30 UTC

mergify bot commented Apr 27, 2020

justusschock commented Apr 27, 2020 • edited Loading

mergify bot commented Apr 27, 2020

jeremyjordan commented Apr 28, 2020

justusschock commented Apr 28, 2020

justusschock commented Apr 28, 2020

justusschock commented Apr 28, 2020

Borda commented Apr 28, 2020 • edited Loading

justusschock commented Apr 28, 2020

williamFalcon commented May 2, 2020

Borda commented May 2, 2020

williamFalcon May 10, 2020

Choose a reason for hiding this comment

justusschock May 12, 2020

Choose a reason for hiding this comment

Borda commented May 11, 2020

mergify bot commented May 12, 2020

williamFalcon commented May 17, 2020

awaelchli commented May 17, 2020 • edited Loading

justusschock commented May 18, 2020

Borda commented May 18, 2020

codecov bot commented May 26, 2020

Codecov Report

awaelchli left a comment

Choose a reason for hiding this comment

awaelchli commented May 30, 2020

Borda commented May 30, 2020

pep8speaks commented Apr 27, 2020 •

edited

Loading

justusschock commented Apr 27, 2020 •

edited

Loading

Borda commented Apr 28, 2020 •

edited

Loading

awaelchli commented May 17, 2020 •

edited

Loading