Skip to content

Improve the way RPC unit tests are skipped for windows #27129

@rohan-varma

Description

@rohan-varma

Feature / Bug

We don't support c10d in Windows. Therefore, It might be useful to add an @unittest.skipIf decorator to the relevant test class in test/test_rpc.py. This would be another failsafe in addition to the current guard in test/test_rpc.py. I have a couple of suggestions on how we could do this:

from common_utils import IS_WINDOWS
...
@unittest.skipIf(IS_WINDOWS, 'c10d not supported on windows')

or

@unittest.skipIf(not dist.is_available())

(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:

try:
    import [imports here]
except ImportError as e:
    unittest.skip('c10d not supported on windows')

... # proceed with normal test file

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

Metadata

Metadata

Assignees

Labels

better-engineeringRelatively self-contained tasks for better engineering contributorsmodule: windowsWindows support for PyTorchoncall: distributedAdd this issue/PR to distributed oncall triage queuetriagedThis issue has been looked at a team member, and triaged and prioritized into an appropriate module

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions