Move imports to after is_available in test_rpc to fix windows builds #27126
Conversation
rohan-varma
changed the title
|
@pytorchbot merge this please |
pytorchbot
added
the
merge-this-please
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@rohan-varma has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@peterjc123, I created #27129 to discuss how we can improve/prevent this from happening. Feel free to chime in with your thoughts. |
|
|
||
| import sys | ||
| import unittest | ||
| from os import getenv |
There was a problem hiding this comment.
Why does getenv need to move?
There was a problem hiding this comment.
It doesn't, but I just wanted to replicate how it looked before. We may want to talk about refactoring this to avoid in the future, which I've opened a discussion about in #27129.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@rohan-varma has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@rohan-varma merged this pull request in 85abfc1. |
|
I think we should disable test_rpc for windows tests, since RPC component is built on Linux socket/network API. |
|
@xush6528 They are disabled on Windows -- but the file is still sourced, which caused this issue. |
…ytorch#27126) Summary: These imports need to go after the check to not break windows since rpc is not supported on windows. I ran the linter and that moved it to before the check in pytorch#26570. This wasn't caught by the windows tests in the PR since it was failing due to a JIT-related reason: (https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-win-ws2016-cuda9-cudnn7-py3-test2/49433/console). In the future, we should probably have a warning/a comment/some message about discouraging running linters? Thanks to peterjc123 for flagging this. Pull Request resolved: pytorch#27126 Differential Revision: D17682761 Pulled By: rohan-varma fbshipit-source-id: 300c74290d734eb8e5880104d2b76dd64217b696
…ytorch#27126) Summary: These imports need to go after the check to not break windows since rpc is not supported on windows. I ran the linter and that moved it to before the check in pytorch#26570. This wasn't caught by the windows tests in the PR since it was failing due to a JIT-related reason: (https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-win-ws2016-cuda9-cudnn7-py3-test2/49433/console). In the future, we should probably have a warning/a comment/some message about discouraging running linters? Thanks to peterjc123 for flagging this. Pull Request resolved: pytorch#27126 Differential Revision: D17682761 Pulled By: rohan-varma fbshipit-source-id: 300c74290d734eb8e5880104d2b76dd64217b696
These imports need to go after the check to not break windows since rpc is not supported on windows. I ran the linter and that moved it to before the check in #26570. This wasn't caught by the windows tests in the PR since it was failing due to a JIT-related reason: (https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-win-ws2016-cuda9-cudnn7-py3-test2/49433/console). In the future, we should probably have a warning/a comment/some message about discouraging running linters? Thanks to @peterjc123 for flagging this.