Skip to content

Train End Error Handling Fix #6864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 52 commits into from
Apr 14, 2021
Merged

Train End Error Handling Fix #6864

merged 52 commits into from
Apr 14, 2021

Conversation

@shuyingsunshine21
Copy link
Contributor

@shuyingsunshine21 shuyingsunshine21 commented Apr 7, 2021 •
edited by carmocca
Loading

What does this PR do?

This PR changes the logic so we NO LONGER try to save on KeyboardInterrupt or call callback's on_train_end

Fixes #6842
Fixes #6807
Fixes #5766

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

Shuying Sun and others added 30 commits March 23, 2021 12:06
Fix some test errors
89f284d 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Merge branch 'master' of https://github.com/PyTorchLightning/pytorch-…
80cfbff 
…lightning

pull latest code
checkpoint consolidation
536c132
@shuyingsunshine21
Update ddp_spawn.py
f172101
@shuyingsunshine21
Update test_metric_result_integration.py
bf70e43
@shuyingsunshine21
Update test_results.py
ea74906
@shuyingsunshine21
Update utils.py
a9aae99
@shuyingsunshine21
Update utils.py
70fe5da
@shuyingsunshine21
Update test_all_gather_grad.py
0d23d75
@shuyingsunshine21
Update test_all_gather_grad.py
ca6f98b
@shuyingsunshine21
Merge pull request #1 from shuyingsunshine21/shuyingsunshine21-checkp…
c5053da 
…oint_consolidate

Update test_all_gather_grad.py
@shuyingsunshine21
Update test_results.py
9d4a2b8
@shuyingsunshine21
Revert "Update test_results.py"
7635b4f 
This reverts commit 9d4a2b8.
@shuyingsunshine21
Revert "Merge pull request #1 from shuyingsunshine21/shuyingsunshine2…
d64f90c 
…1-checkpoint_consolidate"

This reverts commit c5053da, reversing
changes made to 0d23d75.
@shuyingsunshine21
Revert "Update test_all_gather_grad.py"
dcdcd29 
This reverts commit 0d23d75.
@shuyingsunshine21
Revert "Update utils.py"
8651d54 
This reverts commit 70fe5da.
@shuyingsunshine21
Revert "Update utils.py"
15f4b9e 
This reverts commit a9aae99.
@shuyingsunshine21
Revert "Update test_results.py"
250d0aa 
This reverts commit ea74906.
@shuyingsunshine21
Revert "Update test_metric_result_integration.py"
6c095b2 
This reverts commit bf70e43.
@shuyingsunshine21
Revert "Update ddp_spawn.py"
8222dc9 
This reverts commit f172101.
@shuyingsunshine21
Revert "checkpoint consolidation"
3a9fde9 
This reverts commit 536c132.
@shuyingsunshine21
Revert "Revert "checkpoint consolidation""
7a369f4 
This reverts commit 3a9fde9.
@shuyingsunshine21
Revert "Revert "Revert "checkpoint consolidation"""
b4a0b9e 
This reverts commit 7a369f4.
Merge branch 'master' of https://github.com/PyTorchLightning/pytorch-…
5cf1db1 
…lightning
@shuyingsunshine21
Revert "Revert "Update ddp_spawn.py""
0ce7e05 
This reverts commit 8222dc9.
@shuyingsunshine21
Revert "Revert "Update test_metric_result_integration.py""
fe9736d 
This reverts commit 6c095b2.
@shuyingsunshine21
Revert "Revert "Update test_results.py""
c314ef6 
This reverts commit 250d0aa.
@shuyingsunshine21
Revert "Revert "Update utils.py""
c3feda0 
This reverts commit 8651d54.
@shuyingsunshine21
Revert "Revert "Update test_all_gather_grad.py""
c759477 
This reverts commit dcdcd29.
Merge branch 'master' of https://github.com/shuyingsunshine21/pytorch…
7a8e540 
…-lightning
@carmocca
Copy link
Contributor

carmocca commented Apr 9, 2021

Labeling as API change since this removes support for saving checkpoints on Ctrl+C/exception and it might not be okay for some users. I'd like to have @williamFalcon's approval first.

@carmocca carmocca added _Will design Includes a design discussion labels Apr 9, 2021
@carmocca
Copy link
Contributor

carmocca commented Apr 9, 2021 •
edited
Loading

Also, given the current changes, this should be updated since callback's on_train_end won't be called on Ctrl+C

https://github.com/PyTorchLightning/pytorch-lightning/blame/master/docs/source/common/trainer.rst#L145-L149

@awaelchli awaelchli linked an issue Apr 13, 2021 that may be closed by this pull request
DDP does not clean up the processes it makes #6994
Closed
@Borda Borda requested a review from carmocca April 13, 2021 20:42
Borda
Borda reviewed Apr 13, 2021
View reviewed changes
Copy link
Collaborator

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a Traner argument that would enable all these emergency savings, I this was quite a useful feature in case you run training for a while and then timeout or any other kill came...

@shuyingsunshine21
Copy link
Contributor Author

shuyingsunshine21 commented Apr 13, 2021

can we have a Traner argument that would enable all these emergency savings, I this was quite a useful feature in case you run training for a while and then timeout or any other kill came...

wonder if setting "frequent" checkpointing in training would help.

@awaelchli awaelchli mentioned this pull request Apr 13, 2021
Closed
@shuyingsunshine21
Copy link
Contributor Author

shuyingsunshine21 commented Apr 14, 2021

looks like the test fail is related to a recent test: https://github.com/PyTorchLightning/pytorch-lightning/pull/6969/files

because of the current error handling logic, it does not catch this assertion error

https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/trainer/test_training_loop.py#L140-L143

shuyingsunshine21
shuyingsunshine21 commented Apr 14, 2021
View reviewed changes
@tchaton
Copy link
Contributor

tchaton commented Apr 14, 2021

Dear @shuyingsunshine21,

You have 2 failing test on Azure. Do you need help to solve this ?

Best,
T.C

@tchaton tchaton removed the _Will label Apr 14, 2021
@shuyingsunshine21
Copy link
Contributor Author

shuyingsunshine21 commented Apr 14, 2021

Dear @shuyingsunshine21,

You have 2 failing test on Azure. Do you need help to solve this ?

Best,
T.C

Oh, thanks, did not notice that. Let me take a look.

@carmocca
Copy link
Contributor

carmocca commented Apr 14, 2021

Oh, thanks, did not notice that. Let me take a look.

@shuyingsunshine21 The issue is on our end. Seems like there was a fairscale update that is breaking our CI. We are taking a look.

@carmocca carmocca merged commit 03a73b3 into Lightning-AI:master Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Borda Borda Borda left review comments

@ethanwharris ethanwharris ethanwharris left review comments

@williamFalcon williamFalcon williamFalcon approved these changes

@tchaton tchaton tchaton approved these changes

@justusschock justusschock justusschock approved these changes

@awaelchli awaelchli Awaiting requested review from awaelchli

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@edenlightning edenlightning Awaiting requested review from edenlightning

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@carmocca carmocca Awaiting requested review from carmocca

+1 more reviewer

@ananthsub ananthsub ananthsub approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working design Includes a design discussion distributed Generic distributed-related topic

Projects

None yet

Milestone

pl:1.2.x

10 participants

@shuyingsunshine21 @pep8speaks @awaelchli @carmocca @tchaton @ananthsub @williamFalcon @Borda @ethanwharris @justusschock