-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
Join Horovod workers at the end of trainer.fit() to prevent race conditions following training #1786
Conversation
This pull request is now in conflict... :( |
1 similar comment
This pull request is now in conflict... :( |
Codecov Report
@@ Coverage Diff @@
## master #1786 +/- ##
======================================
Coverage 88% 88%
======================================
Files 69 69
Lines 4315 4316 +1
======================================
+ Hits 3804 3805 +1
Misses 511 511 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was requested for review but can only accept with low confidence.
I don't understand why the order of warning/import would cause a race condition.
Generally I would add a test for every bug fix to ensure it does not get re-introduced in the future, but not sure what to suggest here.
otherwise looks ok to me :)
Great job! =) |
oh, I did not know it auto merges. Shouldn't there be a confirm button? |
so be careful when you approve anything lol, there is a rule to merge all if tests pass and collect three approvals... |
lol. i moved those importa up bc they were failing flake8. but now they are at the bottom and flake passes?? @Borda |
yes, there was added ignore, the case was with new |
Just to clarify, the race condition fix line is here: https://github.com/PyTorchLightning/pytorch-lightning/pull/1786/files#diff-d673a98c14aa5cd59610bcc447178852R631. The other lines were introduced to make tests pass. |
awesome haha. I was commenting on our over-zealous flake8 haha :) |
See: #1718 (comment)