process_group: wait for futher_thread join before creating new one #68
Conversation
|
Hi @dwancn! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
thanks a lot. fixed the lint issue. |
|
@dwancn lint is still failing -- are you using CONTRIBUTING.md has details on how to run this locally |
torchft/process_group_test.py
Outdated
| self.assertNotEqual(futher_queue_1, futher_queue_2) | ||
| self.assertNotEqual(p_1, p_2) | ||
|
|
||
| # pyre-ignore[16]: optional |
There was a problem hiding this comment.
FYI in the future the easier thing to do is to just assert that it's not None after you grab it above
i.e.
futher_thread_1 = ...
assert futher_thread_1 is not None
...
There was a problem hiding this comment.
Thanks for the suggestion! Updated the patch again with suggested changes. Fixed some typos as well.
There was a problem hiding this comment.
BTW, I found I was unable to run lintrunner -a directly as suggested by CONTRIBUTING.md. pyre check works for me.
There was a problem hiding this comment.
@dwancn can you open an issue for that? Good to know if there's any issues with it.
Not great to having to have multiple back and forths just for some small lint issues and lintrunner usually solves that pretty easily
This modified the configure behavior in ProcessGroupBaby so that we wait for the futher thread to be terminated before we create new ones. This fixed a race condition where futher queue might be closed while futher thread was still trying to get item from it.
Test plan:
Added test cases for reconfiguring ProcessGroupBaby and included assertions to make sure internal state is set as expected.