-
Notifications
You must be signed in to change notification settings - Fork 27.7k
Improve the way RPC unit tests are skipped for windows #27129
Copy link
Copy link
Closed
Labels
better-engineeringRelatively self-contained tasks for better engineering contributorsRelatively self-contained tasks for better engineering contributorsmodule: windowsWindows support for PyTorchWindows support for PyTorchoncall: distributedAdd this issue/PR to distributed oncall triage queueAdd this issue/PR to distributed oncall triage queuetriagedThis issue has been looked at a team member, and triaged and prioritized into an appropriate moduleThis issue has been looked at a team member, and triaged and prioritized into an appropriate module
Metadata
Metadata
Assignees
Labels
better-engineeringRelatively self-contained tasks for better engineering contributorsRelatively self-contained tasks for better engineering contributorsmodule: windowsWindows support for PyTorchWindows support for PyTorchoncall: distributedAdd this issue/PR to distributed oncall triage queueAdd this issue/PR to distributed oncall triage queuetriagedThis issue has been looked at a team member, and triaged and prioritized into an appropriate moduleThis issue has been looked at a team member, and triaged and prioritized into an appropriate module
Feature / Bug
We don't support c10d in Windows. Therefore, It might be useful to add an
@unittest.skipIfdecorator to the relevant test class intest/test_rpc.py. This would be another failsafe in addition to the current guard intest/test_rpc.py. I have a couple of suggestions on how we could do this:or
(which is basically what we do now, but a little cleaner perhaps)
While we're at it, it might be better to refactor the module so that the imports are checked in the following way:
This way, incorrect imports can't mess up tests, since they are caught. This seems to be a bit more idiomatic than the current setup, and it makes it more obvious to the reader under which circumstances we skip the tests. Finally, this reduces the risk of running the auto-linter messing things up, so that we can work towards making our code lint-compliant.
Curious as to what people think about this.
cc @pietern @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @peterjc123