-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Fix] _init_dist_slurm can fail if scontrol gives a warning #1970
base: master
Are you sure you want to change the base?
Conversation
subprocess.getoutput also returns stderr. If scontrol issues a warning, the variable addr will include this warning
Thanks for your contribution.I add a ref link,which explain |
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.
LGTM.
maybe need to fix some lint @TimDarcet |
I change it anyway. |
unit tests failed |
Hi @TimDarcet !We are grateful for your efforts in helping improve mmcv open-source project during your personal time. |
Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.
Motivation
subprocess.getoutput also returns stderr. If scontrol issues a warning, the variable addr will include this warning.
Using subprocess.check_output fixes this.
Reference:
[1]:https://docs.python.org/2/library/subprocess.html#subprocess.check_output
Modification
Use subprocess.check_output instead of subprocess.getoutput in mmcv.runner._init_dist_slurm
Checklist
Before PR:
After PR: