Skip to content

Conversation

@williamFalcon
Copy link
Contributor

No description provided.

@mergify mergify bot requested a review from a team June 27, 2020 18:14
@Borda Borda added the bug Something isn't working label Jun 27, 2020
@Borda Borda added this to the 0.8.x milestone Jun 27, 2020
Comment on lines 413 to 415
if self.global_rank == 0:
for proc in self.interactive_ddp_procs:
subprocess.Popen.kill(proc)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it go into the run_training_teardown method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no because you still want certain things to happen with every process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wait, i see what you meant. i changed it. better now?

Copy link
Contributor

@awaelchli awaelchli Jun 28, 2020

Choose a reason for hiding this comment

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

not sure actually. I guess the "killing" should be the very last thing, after teardown has fully completed?
anyway not an expert here, so whatever works for you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, i see now. That indeed should be the last thing since it will interrupt every other processes teardown. makes sense!

@mergify mergify bot requested a review from a team June 27, 2020 19:45
@codecov
Copy link

codecov bot commented Jun 27, 2020

Codecov Report

Merging #2389 into master will increase coverage by 0%.
The diff coverage is 83%.

@@          Coverage Diff           @@
##           master   #2389   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          69      69           
  Lines        5452    5454    +2     
======================================
+ Hits         4818    4820    +2     
  Misses        634     634           

@williamFalcon
Copy link
Contributor Author

@awaelchli good to go with this?

@williamFalcon williamFalcon merged commit 66ffbad into master Jun 28, 2020
@Borda Borda deleted the ck branch June 28, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants