-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Description
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