Move imports to after is_available in test_rpc to fix windows builds#27126
Closed
rohan-varma wants to merge 3 commits intomasterfrom
Closed
Move imports to after is_available in test_rpc to fix windows builds#27126rohan-varma wants to merge 3 commits intomasterfrom
rohan-varma wants to merge 3 commits intomasterfrom
Conversation
peterjc123
reviewed
Oct 1, 2019
peterjc123
approved these changes
Oct 1, 2019
Collaborator
|
@pytorchbot merge this please |
Contributor
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.
Contributor
Author
|
@peterjc123, I created #27129 to discuss how we can improve/prevent this from happening. Feel free to chime in with your thoughts. |
pietern
approved these changes
Oct 1, 2019
pietern
reviewed
Oct 1, 2019
|
|
||
| import sys | ||
| import unittest | ||
| from os import getenv |
Contributor
There was a problem hiding this comment.
Why does getenv need to move?
Contributor
Author
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.
Contributor
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.
Contributor
|
@rohan-varma merged this pull request in 85abfc1. |
Contributor
|
I think we should disable test_rpc for windows tests, since RPC component is built on Linux socket/network API. |
Contributor
|
@xush6528 They are disabled on Windows -- but the file is still sourced, which caused this issue. |
Contributor
pdlive215
pushed a commit
to pdlive215/pytorch
that referenced
this pull request
Nov 27, 2019
…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
thiagocrepaldi
pushed a commit
to thiagocrepaldi/pytorch
that referenced
this pull request
Feb 4, 2020
…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
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 24, 2026
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.